mirror of
https://github.com/openai/codex.git
synced 2026-11-13 14:46:14 +00:00
Honor app link settings for MCP tool approvals (#42056)
## What changed - Pass the resolved app `link_id` into MCP tool approval policy evaluation. - Apply link-specific `default_tools_approval_mode` and `approvals_reviewer` settings before app and global defaults, while preserving per-tool and managed policy precedence. ## Testing - Cover link-specific approval prompts, selected account links, reviewer routing, fallback behavior, and managed-policy overrides. GitOrigin-RevId: caa07ec3e73c87aad9522bd69808cd1c0b1686f5
This commit is contained in:
1 parent
0e37d834d4
commit
3a04482645
11 files changed
+429
-39
No files matched your search
@@ -286,6 +286,7 @@ impl ResourceOrigin {
|
|||||||
if !AppToolPolicyEvaluator::new(&binding.config().config_layer_stack)
|
if !AppToolPolicyEvaluator::new(&binding.config().config_layer_stack)
|
||||||
.policy(AppToolPolicyInput {
|
.policy(AppToolPolicyInput {
|
||||||
connector_id: Some(&self.connector_id),
|
connector_id: Some(&self.connector_id),
|
||||||
|
link_id: None,
|
||||||
tool_name: tool_info.tool.name.as_ref(),
|
tool_name: tool_info.tool.name.as_ref(),
|
||||||
tool_title: tool_info.tool.title.as_deref(),
|
tool_title: tool_info.tool.title.as_deref(),
|
||||||
destructive_hint: annotations.and_then(|value| value.destructive_hint),
|
destructive_hint: annotations.and_then(|value| value.destructive_hint),
|
||||||
|
|||||||
@@ -22,10 +22,11 @@ impl Default for AppToolPolicy {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
/// Connector-owned metadata used to evaluate one app tool.
|
/// App and account metadata used to evaluate one tool.
|
||||||
#[derive(Debug, Clone, Copy)]
|
#[derive(Debug, Clone, Copy)]
|
||||||
pub struct AppToolPolicyInput<'a> {
|
pub struct AppToolPolicyInput<'a> {
|
||||||
pub connector_id: Option<&'a str>,
|
pub connector_id: Option<&'a str>,
|
||||||
|
pub link_id: Option<&'a str>,
|
||||||
pub tool_name: &'a str,
|
pub tool_name: &'a str,
|
||||||
pub tool_title: Option<&'a str>,
|
pub tool_title: Option<&'a str>,
|
||||||
pub destructive_hint: Option<bool>,
|
pub destructive_hint: Option<bool>,
|
||||||
@@ -186,6 +187,12 @@ fn app_tool_policy_from_apps_config(
|
|||||||
});
|
});
|
||||||
let approval = managed_approval
|
let approval = managed_approval
|
||||||
.or_else(|| tool_config.and_then(|tool| tool.approval_mode))
|
.or_else(|| tool_config.and_then(|tool| tool.approval_mode))
|
||||||
|
.or_else(|| {
|
||||||
|
input
|
||||||
|
.link_id
|
||||||
|
.and_then(|link_id| app?.links.as_ref()?.links.get(link_id))
|
||||||
|
.and_then(|link| link.default_tools_approval_mode)
|
||||||
|
})
|
||||||
.or_else(|| app.and_then(|app| app.default_tools_approval_mode))
|
.or_else(|| app.and_then(|app| app.default_tools_approval_mode))
|
||||||
.or_else(|| {
|
.or_else(|| {
|
||||||
input
|
input
|
||||||
|
|||||||
@@ -488,6 +488,112 @@ fn managed_approval_overrides_user_tool_approval() {
|
|||||||
);
|
);
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn link_privacy_mode_overrides_app_default_and_preserves_tool_settings() {
|
||||||
|
let apps_config = serde_json::from_value(serde_json::json!({
|
||||||
|
"calendar": {
|
||||||
|
"default_tools_approval_mode": "auto",
|
||||||
|
"tools": { "events/create": { "approval_mode": "writes" } },
|
||||||
|
"links": {
|
||||||
|
"link_calendar": { "default_tools_approval_mode": "approve" },
|
||||||
|
"link_other": { "default_tools_approval_mode": "prompt" },
|
||||||
|
"link_without_privacy": {},
|
||||||
|
},
|
||||||
|
},
|
||||||
|
"drive": {
|
||||||
|
"links": { "link_drive": { "default_tools_approval_mode": "prompt" } },
|
||||||
|
},
|
||||||
|
"without_links": {
|
||||||
|
"default_tools_approval_mode": "writes",
|
||||||
|
},
|
||||||
|
"empty_links": {
|
||||||
|
"default_tools_approval_mode": "prompt",
|
||||||
|
"links": {},
|
||||||
|
},
|
||||||
|
}))
|
||||||
|
.expect("apps config");
|
||||||
|
let evaluator = AppToolPolicyEvaluator::from_parts(
|
||||||
|
Some(apps_config),
|
||||||
|
/*requirements_apps_config*/ None,
|
||||||
|
);
|
||||||
|
|
||||||
|
for (link_id, approval) in [
|
||||||
|
(Some("link_calendar"), AppToolApproval::Approve),
|
||||||
|
(Some("link_other"), AppToolApproval::Prompt),
|
||||||
|
(Some("link_without_privacy"), AppToolApproval::Auto),
|
||||||
|
(Some("link_drive"), AppToolApproval::Auto),
|
||||||
|
(None, AppToolApproval::Auto),
|
||||||
|
] {
|
||||||
|
assert_eq!(
|
||||||
|
evaluator.policy(AppToolPolicyInput {
|
||||||
|
link_id,
|
||||||
|
..input("events/list", /*tool_title*/ None)
|
||||||
|
}),
|
||||||
|
AppToolPolicy {
|
||||||
|
enabled: true,
|
||||||
|
approval,
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
evaluator.policy(AppToolPolicyInput {
|
||||||
|
link_id: Some("link_calendar"),
|
||||||
|
..input("events/create", /*tool_title*/ None)
|
||||||
|
}),
|
||||||
|
AppToolPolicy {
|
||||||
|
enabled: true,
|
||||||
|
approval: AppToolApproval::Writes,
|
||||||
|
}
|
||||||
|
);
|
||||||
|
|
||||||
|
for (connector_id, approval) in [
|
||||||
|
("without_links", AppToolApproval::Writes),
|
||||||
|
("empty_links", AppToolApproval::Prompt),
|
||||||
|
] {
|
||||||
|
assert_eq!(
|
||||||
|
evaluator.policy(AppToolPolicyInput {
|
||||||
|
connector_id: Some(connector_id),
|
||||||
|
link_id: Some("link_calendar"),
|
||||||
|
..input("events/list", /*tool_title*/ None)
|
||||||
|
}),
|
||||||
|
AppToolPolicy {
|
||||||
|
enabled: true,
|
||||||
|
approval,
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
|
#[test]
|
||||||
|
fn link_privacy_mode_preserves_managed_connector_requirements() {
|
||||||
|
let apps_config = serde_json::from_value(serde_json::json!({
|
||||||
|
"calendar": {
|
||||||
|
"links": { "link_calendar": { "default_tools_approval_mode": "approve" } },
|
||||||
|
},
|
||||||
|
}))
|
||||||
|
.expect("apps config");
|
||||||
|
let mut requirements =
|
||||||
|
app_tool_requirements("calendar", "events/create", AppToolApproval::Prompt);
|
||||||
|
requirements
|
||||||
|
.apps
|
||||||
|
.get_mut("calendar")
|
||||||
|
.expect("calendar requirement")
|
||||||
|
.enabled = Some(false);
|
||||||
|
let evaluator = AppToolPolicyEvaluator::from_parts(Some(apps_config), Some(&requirements));
|
||||||
|
|
||||||
|
assert_eq!(
|
||||||
|
evaluator.policy(AppToolPolicyInput {
|
||||||
|
link_id: Some("link_calendar"),
|
||||||
|
..input("events/create", /*tool_title*/ None)
|
||||||
|
}),
|
||||||
|
AppToolPolicy {
|
||||||
|
enabled: false,
|
||||||
|
approval: AppToolApproval::Prompt,
|
||||||
|
}
|
||||||
|
);
|
||||||
|
}
|
||||||
|
|
||||||
#[test]
|
#[test]
|
||||||
fn per_tool_enable_overrides_app_level_hints() {
|
fn per_tool_enable_overrides_app_level_hints() {
|
||||||
let apps_config = AppsConfigToml {
|
let apps_config = AppsConfigToml {
|
||||||
@@ -705,6 +811,7 @@ fn evaluator_matches_tool_title_for_user_config() {
|
|||||||
fn input<'a>(tool_name: &'a str, tool_title: Option<&'a str>) -> AppToolPolicyInput<'a> {
|
fn input<'a>(tool_name: &'a str, tool_title: Option<&'a str>) -> AppToolPolicyInput<'a> {
|
||||||
AppToolPolicyInput {
|
AppToolPolicyInput {
|
||||||
connector_id: Some("calendar"),
|
connector_id: Some("calendar"),
|
||||||
|
link_id: None,
|
||||||
tool_name,
|
tool_name,
|
||||||
tool_title,
|
tool_title,
|
||||||
destructive_hint: Some(true),
|
destructive_hint: Some(true),
|
||||||
@@ -795,6 +902,7 @@ fn policy_from_config_parts(
|
|||||||
};
|
};
|
||||||
AppToolPolicyEvaluator::new(&config_layer_stack).policy(AppToolPolicyInput {
|
AppToolPolicyEvaluator::new(&config_layer_stack).policy(AppToolPolicyInput {
|
||||||
connector_id,
|
connector_id,
|
||||||
|
link_id: None,
|
||||||
tool_name,
|
tool_name,
|
||||||
tool_title,
|
tool_title,
|
||||||
destructive_hint,
|
destructive_hint,
|
||||||
|
|||||||
@@ -69,6 +69,7 @@ pub fn installed_connector_runtime<'a>(
|
|||||||
let policy_allows_tool = policy
|
let policy_allows_tool = policy
|
||||||
.policy(AppToolPolicyInput {
|
.policy(AppToolPolicyInput {
|
||||||
connector_id: Some(connector_id),
|
connector_id: Some(connector_id),
|
||||||
|
link_id: None,
|
||||||
tool_name: tool.tool_name,
|
tool_name: tool.tool_name,
|
||||||
tool_title: tool.tool_title,
|
tool_title: tool.tool_title,
|
||||||
destructive_hint: tool.destructive_hint,
|
destructive_hint: tool.destructive_hint,
|
||||||
|
|||||||
@@ -517,6 +517,7 @@ pub(crate) fn mcp_approvals_reviewer_from_layers(
|
|||||||
model: Option<&str>,
|
model: Option<&str>,
|
||||||
server_name: &str,
|
server_name: &str,
|
||||||
connector_id: Option<&str>,
|
connector_id: Option<&str>,
|
||||||
|
link_id: Option<&str>,
|
||||||
) -> ApprovalsReviewer {
|
) -> ApprovalsReviewer {
|
||||||
let requirements = config_layer_stack.requirements();
|
let requirements = config_layer_stack.requirements();
|
||||||
if model.is_some_and(|model| requirements.auto_review_required_for_model(model)) {
|
if model.is_some_and(|model| requirements.auto_review_required_for_model(model)) {
|
||||||
@@ -525,9 +526,11 @@ pub(crate) fn mcp_approvals_reviewer_from_layers(
|
|||||||
|
|
||||||
let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME {
|
let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME {
|
||||||
apps_config_from_layer_stack(config_layer_stack).and_then(|apps_config| {
|
apps_config_from_layer_stack(config_layer_stack).and_then(|apps_config| {
|
||||||
connector_id
|
let app = connector_id.and_then(|connector_id| apps_config.apps.get(connector_id));
|
||||||
.and_then(|connector_id| apps_config.apps.get(connector_id))
|
link_id
|
||||||
.and_then(|app| app.approvals_reviewer)
|
.and_then(|link_id| app?.links.as_ref()?.links.get(link_id))
|
||||||
|
.and_then(|link| link.approvals_reviewer)
|
||||||
|
.or_else(|| app.and_then(|app| app.approvals_reviewer))
|
||||||
.or_else(|| {
|
.or_else(|| {
|
||||||
apps_config
|
apps_config
|
||||||
.default
|
.default
|
||||||
|
|||||||
@@ -304,7 +304,7 @@ fn accessible_connectors_from_mcp_tools_preserves_description() {
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
async fn app_approvals_reviewer_uses_app_then_default_then_global() {
|
async fn app_approvals_reviewer_uses_link_then_app_then_default_then_global() {
|
||||||
for (global, app_default, app, expected_global, expected_default, expected_app) in [
|
for (global, app_default, app, expected_global, expected_default, expected_app) in [
|
||||||
(
|
(
|
||||||
"user",
|
"user",
|
||||||
@@ -335,6 +335,22 @@ approvals_reviewer = "{app_default}"
|
|||||||
|
|
||||||
[apps.calendar]
|
[apps.calendar]
|
||||||
approvals_reviewer = "{app}"
|
approvals_reviewer = "{app}"
|
||||||
|
|
||||||
|
[apps.calendar.links.link_calendar]
|
||||||
|
approvals_reviewer = "{app_default}"
|
||||||
|
|
||||||
|
[apps.calendar.links.link_without_privacy]
|
||||||
|
|
||||||
|
[apps.drive.links.link_drive]
|
||||||
|
approvals_reviewer = "{app_default}"
|
||||||
|
|
||||||
|
[apps.without_links]
|
||||||
|
approvals_reviewer = "{app}"
|
||||||
|
|
||||||
|
[apps.empty_links]
|
||||||
|
approvals_reviewer = "{app}"
|
||||||
|
|
||||||
|
[apps.empty_links.links]
|
||||||
"#
|
"#
|
||||||
),
|
),
|
||||||
)
|
)
|
||||||
@@ -345,23 +361,45 @@ approvals_reviewer = "{app}"
|
|||||||
.await
|
.await
|
||||||
.expect("config should build");
|
.expect("config should build");
|
||||||
|
|
||||||
|
for (link_id, expected) in [
|
||||||
|
(Some("link_calendar"), expected_default),
|
||||||
|
(Some("link_without_privacy"), expected_app),
|
||||||
|
(Some("link_drive"), expected_app),
|
||||||
|
(None, expected_app),
|
||||||
|
] {
|
||||||
|
assert_eq!(
|
||||||
|
mcp_approvals_reviewer_from_layers(
|
||||||
|
&config.config_layer_stack,
|
||||||
|
config.approvals_reviewer,
|
||||||
|
config.model.as_deref(),
|
||||||
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
|
Some("calendar"),
|
||||||
|
link_id,
|
||||||
|
),
|
||||||
|
expected
|
||||||
|
);
|
||||||
|
}
|
||||||
|
for connector_id in ["without_links", "empty_links"] {
|
||||||
|
assert_eq!(
|
||||||
|
mcp_approvals_reviewer_from_layers(
|
||||||
|
&config.config_layer_stack,
|
||||||
|
config.approvals_reviewer,
|
||||||
|
config.model.as_deref(),
|
||||||
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
|
Some(connector_id),
|
||||||
|
Some("link_calendar"),
|
||||||
|
),
|
||||||
|
expected_app
|
||||||
|
);
|
||||||
|
}
|
||||||
assert_eq!(
|
assert_eq!(
|
||||||
mcp_approvals_reviewer_from_layers(
|
mcp_approvals_reviewer_from_layers(
|
||||||
&config.config_layer_stack,
|
&config.config_layer_stack,
|
||||||
config.approvals_reviewer,
|
config.approvals_reviewer,
|
||||||
config.model.as_deref(),
|
config.model.as_deref(),
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
Some("calendar")
|
Some("drive"),
|
||||||
),
|
/*link_id*/ None,
|
||||||
expected_app
|
|
||||||
);
|
|
||||||
assert_eq!(
|
|
||||||
mcp_approvals_reviewer_from_layers(
|
|
||||||
&config.config_layer_stack,
|
|
||||||
config.approvals_reviewer,
|
|
||||||
config.model.as_deref(),
|
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
|
||||||
Some("drive")
|
|
||||||
),
|
),
|
||||||
expected_default
|
expected_default
|
||||||
);
|
);
|
||||||
@@ -371,7 +409,8 @@ approvals_reviewer = "{app}"
|
|||||||
config.approvals_reviewer,
|
config.approvals_reviewer,
|
||||||
config.model.as_deref(),
|
config.model.as_deref(),
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
/*connector_id*/ None
|
/*connector_id*/ None,
|
||||||
|
/*link_id*/ None,
|
||||||
),
|
),
|
||||||
expected_default
|
expected_default
|
||||||
);
|
);
|
||||||
@@ -381,7 +420,8 @@ approvals_reviewer = "{app}"
|
|||||||
config.approvals_reviewer,
|
config.approvals_reviewer,
|
||||||
config.model.as_deref(),
|
config.model.as_deref(),
|
||||||
"custom_server",
|
"custom_server",
|
||||||
Some("calendar")
|
Some("calendar"),
|
||||||
|
Some("link_calendar"),
|
||||||
),
|
),
|
||||||
expected_global
|
expected_global
|
||||||
);
|
);
|
||||||
@@ -418,7 +458,8 @@ approvals_reviewer = "user"
|
|||||||
config.approvals_reviewer,
|
config.approvals_reviewer,
|
||||||
config.model.as_deref(),
|
config.model.as_deref(),
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
Some("calendar")
|
Some("calendar"),
|
||||||
|
/*link_id*/ None,
|
||||||
),
|
),
|
||||||
ApprovalsReviewer::AutoReview
|
ApprovalsReviewer::AutoReview
|
||||||
);
|
);
|
||||||
@@ -434,6 +475,9 @@ approvals_reviewer = "auto_review"
|
|||||||
|
|
||||||
[apps.calendar]
|
[apps.calendar]
|
||||||
approvals_reviewer = "user"
|
approvals_reviewer = "user"
|
||||||
|
|
||||||
|
[apps.calendar.links.link_calendar]
|
||||||
|
approvals_reviewer = "user"
|
||||||
"#,
|
"#,
|
||||||
)
|
)
|
||||||
.expect("write config");
|
.expect("write config");
|
||||||
@@ -448,16 +492,19 @@ approvals_reviewer = "user"
|
|||||||
.await
|
.await
|
||||||
.expect("config should build");
|
.expect("config should build");
|
||||||
|
|
||||||
assert_eq!(
|
for link_id in [None, Some("link_calendar")] {
|
||||||
mcp_approvals_reviewer_from_layers(
|
assert_eq!(
|
||||||
&config.config_layer_stack,
|
mcp_approvals_reviewer_from_layers(
|
||||||
config.approvals_reviewer,
|
&config.config_layer_stack,
|
||||||
config.model.as_deref(),
|
config.approvals_reviewer,
|
||||||
CODEX_APPS_MCP_SERVER_NAME,
|
config.model.as_deref(),
|
||||||
Some("calendar")
|
CODEX_APPS_MCP_SERVER_NAME,
|
||||||
),
|
Some("calendar"),
|
||||||
ApprovalsReviewer::AutoReview
|
link_id,
|
||||||
);
|
),
|
||||||
|
ApprovalsReviewer::AutoReview
|
||||||
|
);
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test]
|
#[tokio::test]
|
||||||
|
|||||||
@@ -334,6 +334,7 @@ fn executor_hook_sources_for_step(step_context: &StepContext) -> Vec<ExecutorPlu
|
|||||||
app_tool_policy
|
app_tool_policy
|
||||||
.policy(AppToolPolicyInput {
|
.policy(AppToolPolicyInput {
|
||||||
connector_id: tool_info.connector_id.as_deref(),
|
connector_id: tool_info.connector_id.as_deref(),
|
||||||
|
link_id: None,
|
||||||
tool_name: &tool_info.tool.name,
|
tool_name: &tool_info.tool.name,
|
||||||
tool_title: tool_info.tool.title.as_deref(),
|
tool_title: tool_info.tool.title.as_deref(),
|
||||||
destructive_hint: annotations
|
destructive_hint: annotations
|
||||||
|
|||||||
@@ -207,6 +207,7 @@ pub(crate) async fn handle_mcp_tool_call(
|
|||||||
let annotations = metadata.annotations.as_ref();
|
let annotations = metadata.annotations.as_ref();
|
||||||
AppToolPolicyEvaluator::new(&runtime_config.config_layer_stack).policy(AppToolPolicyInput {
|
AppToolPolicyEvaluator::new(&runtime_config.config_layer_stack).policy(AppToolPolicyInput {
|
||||||
connector_id: metadata.connector_id.as_deref(),
|
connector_id: metadata.connector_id.as_deref(),
|
||||||
|
link_id: metadata.link_id.as_deref(),
|
||||||
tool_name: &tool_name,
|
tool_name: &tool_name,
|
||||||
tool_title: metadata.tool_title.as_deref(),
|
tool_title: metadata.tool_title.as_deref(),
|
||||||
destructive_hint: annotations.and_then(|annotations| annotations.destructive_hint),
|
destructive_hint: annotations.and_then(|annotations| annotations.destructive_hint),
|
||||||
@@ -1415,6 +1416,7 @@ async fn maybe_request_mcp_tool_approval(
|
|||||||
Some(turn_context.model_info().slug.as_str()),
|
Some(turn_context.model_info().slug.as_str()),
|
||||||
&invocation.server,
|
&invocation.server,
|
||||||
metadata.connector_id.as_deref(),
|
metadata.connector_id.as_deref(),
|
||||||
|
metadata.link_id.as_deref(),
|
||||||
);
|
);
|
||||||
if !strict_auto_review
|
if !strict_auto_review
|
||||||
&& mcp_permission_prompt_is_auto_approved(
|
&& mcp_permission_prompt_is_auto_approved(
|
||||||
|
|||||||
@@ -174,6 +174,7 @@ fn filter_codex_apps_mcp_tools<'a>(
|
|||||||
app_tool_policy
|
app_tool_policy
|
||||||
.policy(AppToolPolicyInput {
|
.policy(AppToolPolicyInput {
|
||||||
connector_id: Some(connector_id),
|
connector_id: Some(connector_id),
|
||||||
|
link_id: None,
|
||||||
tool_name: &tool.tool.name,
|
tool_name: &tool.tool.name,
|
||||||
tool_title: tool.tool.title.as_deref(),
|
tool_title: tool.tool.title.as_deref(),
|
||||||
destructive_hint: annotations.and_then(|annotations| annotations.destructive_hint),
|
destructive_hint: annotations.and_then(|annotations| annotations.destructive_hint),
|
||||||
|
|||||||
@@ -793,6 +793,7 @@ async fn review_guardian_mcp_elicitation(
|
|||||||
Some(turn_context.model_info().slug.as_str()),
|
Some(turn_context.model_info().slug.as_str()),
|
||||||
request.server_name.as_str(),
|
request.server_name.as_str(),
|
||||||
connector_id,
|
connector_id,
|
||||||
|
/*link_id*/ None,
|
||||||
) != ApprovalsReviewer::AutoReview
|
) != ApprovalsReviewer::AutoReview
|
||||||
|| request
|
|| request
|
||||||
.elicitation
|
.elicitation
|
||||||
@@ -877,6 +878,7 @@ async fn review_guardian_mcp_elicitation(
|
|||||||
Some(turn_context.model_info().slug.as_str()),
|
Some(turn_context.model_info().slug.as_str()),
|
||||||
request.server_name.as_str(),
|
request.server_name.as_str(),
|
||||||
elicitation_connector_id(&request.elicitation),
|
elicitation_connector_id(&request.elicitation),
|
||||||
|
/*link_id*/ None,
|
||||||
);
|
);
|
||||||
if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) {
|
if !crate::guardian::routes_approval_policy_to_guardian(approval_policy, approvals_reviewer) {
|
||||||
return Ok(None);
|
return Ok(None);
|
||||||
|
|||||||
@@ -31,6 +31,7 @@ use codex_protocol::request_user_input::RequestUserInputResponse;
|
|||||||
use codex_protocol::user_input::UserInput;
|
use codex_protocol::user_input::UserInput;
|
||||||
use core_test_support::PathExt;
|
use core_test_support::PathExt;
|
||||||
use core_test_support::apps_test_server::AppsTestServer;
|
use core_test_support::apps_test_server::AppsTestServer;
|
||||||
|
use core_test_support::apps_test_server::LINK_ID;
|
||||||
use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL;
|
use core_test_support::apps_test_server::SEARCH_CALENDAR_CREATE_TOOL;
|
||||||
use core_test_support::apps_test_server::SEARCH_CALENDAR_LIST_TOOL;
|
use core_test_support::apps_test_server::SEARCH_CALENDAR_LIST_TOOL;
|
||||||
use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE;
|
use core_test_support::apps_test_server::SEARCH_CALENDAR_NAMESPACE;
|
||||||
@@ -379,10 +380,12 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request(
|
|||||||
}
|
}
|
||||||
|
|
||||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
#[test_case(false; "unmanaged model")]
|
#[test_case(false, false; "unmanaged model")]
|
||||||
#[test_case(true; "protected model")]
|
#[test_case(true, false; "protected model")]
|
||||||
async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian(
|
#[test_case(false, true; "work mode link reviewer")]
|
||||||
|
async fn apps_prompt_with_auto_review_routes_actual_mcp_approval_to_guardian(
|
||||||
protected_model: bool,
|
protected_model: bool,
|
||||||
|
work_link_reviewer: bool,
|
||||||
) -> Result<()> {
|
) -> Result<()> {
|
||||||
skip_if_no_network!(Ok(()));
|
skip_if_no_network!(Ok(()));
|
||||||
|
|
||||||
@@ -441,11 +444,30 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar
|
|||||||
.features
|
.features
|
||||||
.enable(Feature::ToolCallMcpElicitation)
|
.enable(Feature::ToolCallMcpElicitation)
|
||||||
.expect("test config should allow feature update");
|
.expect("test config should allow feature update");
|
||||||
set_default_app_approval_mode_and_reviewer(
|
if work_link_reviewer {
|
||||||
config,
|
config.apps_mcp_product_sku = Some("tpp".to_string());
|
||||||
AppToolApproval::Prompt,
|
let user_config = toml::from_str(&format!(
|
||||||
app_reviewer,
|
r#"
|
||||||
);
|
[apps._default]
|
||||||
|
default_tools_approval_mode = "prompt"
|
||||||
|
approvals_reviewer = "user"
|
||||||
|
[apps.calendar.links.{LINK_ID}]
|
||||||
|
default_tools_approval_mode = "prompt"
|
||||||
|
approvals_reviewer = "auto_review"
|
||||||
|
"#
|
||||||
|
))
|
||||||
|
.expect("apps config should parse");
|
||||||
|
config.config_layer_stack = config
|
||||||
|
.config_layer_stack
|
||||||
|
.with_user_config(&config.codex_home.join("config.toml").abs(), user_config)
|
||||||
|
.expect("apps user config should be valid");
|
||||||
|
} else {
|
||||||
|
set_default_app_approval_mode_and_reviewer(
|
||||||
|
config,
|
||||||
|
AppToolApproval::Prompt,
|
||||||
|
app_reviewer,
|
||||||
|
);
|
||||||
|
}
|
||||||
});
|
});
|
||||||
if protected_model {
|
if protected_model {
|
||||||
builder = builder.with_model("gpt-5.4").with_cloud_config_bundle(
|
builder = builder.with_model("gpt-5.4").with_cloud_config_bundle(
|
||||||
@@ -454,7 +476,7 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar
|
|||||||
),
|
),
|
||||||
);
|
);
|
||||||
}
|
}
|
||||||
let test = builder.build(&server).await?;
|
let test = builder.build_with_auto_env(&server).await?;
|
||||||
|
|
||||||
submit_user_turn(
|
submit_user_turn(
|
||||||
&test,
|
&test,
|
||||||
@@ -478,7 +500,7 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar
|
|||||||
.await;
|
.await;
|
||||||
assert!(
|
assert!(
|
||||||
matches!(route_event, EventMsg::TurnComplete(_)),
|
matches!(route_event, EventMsg::TurnComplete(_)),
|
||||||
"expected apps._default auto_review to route the app approval to Guardian"
|
"expected the app's auto_review setting to route the approval to Guardian"
|
||||||
);
|
);
|
||||||
|
|
||||||
let guardian_request = responses
|
let guardian_request = responses
|
||||||
@@ -503,6 +525,201 @@ async fn apps_default_prompt_with_auto_review_routes_actual_mcp_approval_to_guar
|
|||||||
Ok(())
|
Ok(())
|
||||||
}
|
}
|
||||||
|
|
||||||
|
#[test_case("approve", "prompt", None, false, true; "work_link_requires_prompt")]
|
||||||
|
#[test_case("prompt", "approve", None, false, false; "work_link_skips_prompt")]
|
||||||
|
#[test_case("approve", "approve", Some("selected_calendar_link"), true, true; "required_selector_uses_selected_link")]
|
||||||
|
#[test_case("prompt", "prompt", Some("selected_calendar_link"), false, true; "ignores_non_selector_link_argument")]
|
||||||
|
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||||
|
async fn apps_link_policy_controls_elicitation(
|
||||||
|
connector_mode: &str,
|
||||||
|
link_mode: &str,
|
||||||
|
selected_link_id: Option<&'static str>,
|
||||||
|
requires_explicit_link_id: bool,
|
||||||
|
expected_prompt: bool,
|
||||||
|
) -> Result<()> {
|
||||||
|
skip_if_no_network!(Ok(()));
|
||||||
|
|
||||||
|
let server = start_mock_server().await;
|
||||||
|
let apps_server = AppsTestServer::mount(&server).await?;
|
||||||
|
let call_id = "calendar-link-policy";
|
||||||
|
let mut calendar_args = json!({
|
||||||
|
"title": "Lunch",
|
||||||
|
"starts_at": "2026-03-10T12:00:00Z"
|
||||||
|
});
|
||||||
|
let mut apps_config = format!(
|
||||||
|
r#"
|
||||||
|
[apps.calendar]
|
||||||
|
default_tools_approval_mode = "{connector_mode}"
|
||||||
|
|
||||||
|
[apps.calendar.links.{LINK_ID}]
|
||||||
|
default_tools_approval_mode = "{link_mode}"
|
||||||
|
"#
|
||||||
|
);
|
||||||
|
if let Some(selected_link_id) = selected_link_id {
|
||||||
|
calendar_args["link_id"] = json!(selected_link_id);
|
||||||
|
let selected_link_mode = if requires_explicit_link_id {
|
||||||
|
"prompt"
|
||||||
|
} else {
|
||||||
|
"approve"
|
||||||
|
};
|
||||||
|
apps_config.push_str(&format!(
|
||||||
|
r#"
|
||||||
|
[apps.calendar.links.{selected_link_id}]
|
||||||
|
default_tools_approval_mode = "{selected_link_mode}"
|
||||||
|
"#
|
||||||
|
));
|
||||||
|
let mut required = vec!["title", "starts_at"];
|
||||||
|
let link_description = if requires_explicit_link_id {
|
||||||
|
required.push("link_id");
|
||||||
|
format!(
|
||||||
|
"Link ID for the account this call should use. Supply link_id using a link_id value below. Select only from the accounts below.\n{}",
|
||||||
|
serde_json::to_string_pretty(&json!([
|
||||||
|
{ "link_id": LINK_ID, "link_name": "Default calendar" },
|
||||||
|
{ "link_id": selected_link_id, "link_name": "Selected calendar" }
|
||||||
|
]))?
|
||||||
|
)
|
||||||
|
} else {
|
||||||
|
"Link identifier for the calendar resource.".to_string()
|
||||||
|
};
|
||||||
|
let tool = json!({
|
||||||
|
"name": "calendar_create_event",
|
||||||
|
"description": "Create a calendar event.",
|
||||||
|
"annotations": { "readOnlyHint": false },
|
||||||
|
"inputSchema": {
|
||||||
|
"type": "object",
|
||||||
|
"properties": {
|
||||||
|
"title": { "type": "string" },
|
||||||
|
"starts_at": { "type": "string" },
|
||||||
|
"link_id": { "type": "string", "description": link_description }
|
||||||
|
},
|
||||||
|
"required": required,
|
||||||
|
"additionalProperties": false
|
||||||
|
},
|
||||||
|
"_meta": {
|
||||||
|
"connector_id": "calendar",
|
||||||
|
"link_id": LINK_ID,
|
||||||
|
"connector_name": "Calendar",
|
||||||
|
"_codex_apps": {
|
||||||
|
"connector_id": "calendar",
|
||||||
|
"resource_uri": "connector://calendar/tools/calendar_create_event",
|
||||||
|
"contains_mcp_source": true,
|
||||||
|
"requires_explicit_link_id": requires_explicit_link_id
|
||||||
|
}
|
||||||
|
}
|
||||||
|
});
|
||||||
|
Mock::given(method("POST"))
|
||||||
|
.and(path_regex("^/api/codex/ps/mcp/?$"))
|
||||||
|
.and(body_partial_json(json!({ "method": "tools/list" })))
|
||||||
|
.respond_with(move |request: &Request| {
|
||||||
|
let body: Value = serde_json::from_slice(&request.body)
|
||||||
|
.expect("Apps tools/list should be a valid JSON-RPC request");
|
||||||
|
ResponseTemplate::new(200).set_body_json(json!({
|
||||||
|
"jsonrpc": "2.0",
|
||||||
|
"id": body["id"],
|
||||||
|
"result": { "tools": [tool] }
|
||||||
|
}))
|
||||||
|
})
|
||||||
|
.with_priority(1)
|
||||||
|
.mount(&server)
|
||||||
|
.await;
|
||||||
|
}
|
||||||
|
let user_config = toml::from_str(&apps_config)?;
|
||||||
|
let mock = mount_sse_sequence(
|
||||||
|
&server,
|
||||||
|
vec![
|
||||||
|
sse(vec![
|
||||||
|
ev_response_created("resp-calendar"),
|
||||||
|
ev_function_call_with_namespace(
|
||||||
|
call_id,
|
||||||
|
SEARCH_CALENDAR_NAMESPACE,
|
||||||
|
SEARCH_CALENDAR_CREATE_TOOL,
|
||||||
|
&calendar_args.to_string(),
|
||||||
|
),
|
||||||
|
ev_completed("resp-calendar"),
|
||||||
|
]),
|
||||||
|
sse(vec![
|
||||||
|
ev_response_created("resp-done"),
|
||||||
|
ev_assistant_message("msg-done", "done"),
|
||||||
|
ev_completed("resp-done"),
|
||||||
|
]),
|
||||||
|
],
|
||||||
|
)
|
||||||
|
.await;
|
||||||
|
let mut builder =
|
||||||
|
search_capable_apps_builder(apps_server.chatgpt_base_url).with_config(move |config| {
|
||||||
|
config.apps_mcp_product_sku = Some("tpp".to_string());
|
||||||
|
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||||
|
config
|
||||||
|
.features
|
||||||
|
.enable(Feature::ToolCallMcpElicitation)
|
||||||
|
.expect("test config should allow feature update");
|
||||||
|
config.config_layer_stack = config
|
||||||
|
.config_layer_stack
|
||||||
|
.with_user_config(&config.codex_home.join("config.toml").abs(), user_config)
|
||||||
|
.expect("apps user config should be valid");
|
||||||
|
});
|
||||||
|
let test = builder.build_with_auto_env(&server).await?;
|
||||||
|
|
||||||
|
submit_user_turn(
|
||||||
|
&test,
|
||||||
|
"Use [$calendar](app://calendar) to create a calendar event.",
|
||||||
|
AskForApproval::OnRequest,
|
||||||
|
PermissionProfile::Disabled,
|
||||||
|
/*collaboration_mode*/ None,
|
||||||
|
)
|
||||||
|
.await?;
|
||||||
|
|
||||||
|
let mut completed_links = Vec::new();
|
||||||
|
let mut record_link = |event: &EventMsg| {
|
||||||
|
if let EventMsg::ItemCompleted(event) = event
|
||||||
|
&& let TurnItem::McpToolCall(item) = &event.item
|
||||||
|
&& item.id == call_id
|
||||||
|
{
|
||||||
|
completed_links.push(item.link_id.clone());
|
||||||
|
}
|
||||||
|
};
|
||||||
|
let event = wait_for_event(&test.codex, |event| {
|
||||||
|
record_link(event);
|
||||||
|
matches!(
|
||||||
|
event,
|
||||||
|
EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_)
|
||||||
|
)
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
assert_eq!(
|
||||||
|
matches!(&event, EventMsg::ElicitationRequest(_)),
|
||||||
|
expected_prompt
|
||||||
|
);
|
||||||
|
if let EventMsg::ElicitationRequest(request) = event {
|
||||||
|
test.codex
|
||||||
|
.submit(Op::ResolveElicitation {
|
||||||
|
server_name: request.server_name,
|
||||||
|
request_id: request.id,
|
||||||
|
decision: ElicitationAction::Accept,
|
||||||
|
content: None,
|
||||||
|
meta: None,
|
||||||
|
})
|
||||||
|
.await?;
|
||||||
|
wait_for_event(&test.codex, |event| {
|
||||||
|
record_link(event);
|
||||||
|
matches!(event, EventMsg::TurnComplete(_))
|
||||||
|
})
|
||||||
|
.await;
|
||||||
|
}
|
||||||
|
assert_eq!(mock.requests().len(), 2);
|
||||||
|
assert_eq!(
|
||||||
|
recorded_apps_tool_call_by_call_id(&server, call_id).await["params"]["arguments"],
|
||||||
|
calendar_args
|
||||||
|
);
|
||||||
|
let expected_link_id = if requires_explicit_link_id {
|
||||||
|
selected_link_id
|
||||||
|
} else {
|
||||||
|
Some(LINK_ID)
|
||||||
|
};
|
||||||
|
assert_eq!(completed_links, vec![expected_link_id.map(str::to_string)]);
|
||||||
|
Ok(())
|
||||||
|
}
|
||||||
|
|
||||||
const INVALID_APP_SELECTOR_ERROR: &str =
|
const INVALID_APP_SELECTOR_ERROR: &str =
|
||||||
"This app tool requires a non-empty string link_id argument";
|
"This app tool requires a non-empty string link_id argument";
|
||||||
|
|
||||||
|
|||||||
Reference in new issue
Block a user