mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[app-server][core] Add connector-level Guardian reviewer overrides (#25167)
Context: https://openai.slack.com/archives/C0B4JAF0Q2C/p1779912328647229 ``` approvals_reviewer = "auto_review" [apps.connector_5f3c8c41a1e54ad7a76272c89e2554fa] enabled = true approvals_reviewer = "user" default_tools_approval_mode = "prompt" ``` <img width="230" height="84" alt="Screenshot 2026-05-31 at 11 56 34 AM" src="https://github.com/user-attachments/assets/e319f8f7-0983-42a7-98cd-3302732fa406" /> <img width="841" height="233" alt="Screenshot 2026-05-31 at 11 52 42 AM" src="https://github.com/user-attachments/assets/7ac76645-4e90-4d00-8242-f031146a22a5" /> ------- ``` approvals_reviewer = "user" [apps.connector_5f3c8c41a1e54ad7a76272c89e2554fa] enabled = true approvals_reviewer = "auto_review" default_tools_approval_mode = "prompt" ``` <img width="195" height="83" alt="Screenshot 2026-05-31 at 12 02 27 PM" src="https://github.com/user-attachments/assets/3d374dc8-8aa2-466f-a13f-e4ed8567aa2e" /> <img width="771" height="207" alt="Screenshot 2026-05-31 at 12 05 42 PM" src="https://github.com/user-attachments/assets/105c2575-68d6-4ca6-8e69-dc8c82da36a2" /> ## Summary - add `apps.<connector_id>.approvals_reviewer` to override Guardian or user review routing per connected app - apply overrides across direct app MCP calls, delegated MCP prompts, and app-server MCP elicitation review while preserving global behavior for non-app MCP servers - expose and document the config through app-server v2 and generated schemas, while honoring global managed reviewer requirements --------- Co-authored-by: jif-oai <jif@openai.com>
This commit is contained in:
committed by
GitHub
Unverified
parent
c097ad3e9e
commit
4d80d808b4
+10
@@ -5952,6 +5952,16 @@
|
||||
},
|
||||
"AppConfig": {
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/v2/ApprovalsReviewer"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"default_tools_approval_mode": {
|
||||
"anyOf": [
|
||||
{
|
||||
|
||||
+10
@@ -324,6 +324,16 @@
|
||||
},
|
||||
"AppConfig": {
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/ApprovalsReviewer"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"default_tools_approval_mode": {
|
||||
"anyOf": [
|
||||
{
|
||||
|
||||
@@ -19,6 +19,16 @@
|
||||
},
|
||||
"AppConfig": {
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/ApprovalsReviewer"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"default_tools_approval_mode": {
|
||||
"anyOf": [
|
||||
{
|
||||
|
||||
@@ -3,6 +3,7 @@
|
||||
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
|
||||
import type { AppToolApproval } from "./AppToolApproval";
|
||||
import type { AppToolsConfig } from "./AppToolsConfig";
|
||||
import type { ApprovalsReviewer } from "./ApprovalsReviewer";
|
||||
import type { AppsDefaultConfig } from "./AppsDefaultConfig";
|
||||
|
||||
export type AppsConfig = { _default: AppsDefaultConfig | null, } & ({ [key in string]?: { enabled: boolean, destructive_enabled: boolean | null, open_world_enabled: boolean | null, default_tools_approval_mode: AppToolApproval | null, default_tools_enabled: boolean | null, tools: AppToolsConfig | null, } });
|
||||
export type AppsConfig = { _default: AppsDefaultConfig | null, } & ({ [key in string]?: { enabled: boolean, approvals_reviewer: ApprovalsReviewer | null, destructive_enabled: boolean | null, open_world_enabled: boolean | null, default_tools_approval_mode: AppToolApproval | null, default_tools_enabled: boolean | null, tools: AppToolsConfig | null, } });
|
||||
|
||||
@@ -200,6 +200,7 @@ pub struct AppToolsConfig {
|
||||
pub struct AppConfig {
|
||||
#[serde(default = "default_enabled")]
|
||||
pub enabled: bool,
|
||||
pub approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
pub destructive_enabled: Option<bool>,
|
||||
pub open_world_enabled: Option<bool>,
|
||||
pub default_tools_approval_mode: Option<AppToolApproval>,
|
||||
|
||||
@@ -1716,6 +1716,20 @@ The server also emits `app/list/updated` notifications whenever either source (a
|
||||
}
|
||||
```
|
||||
|
||||
Connected apps may override the thread's approval reviewer in `config.toml`.
|
||||
When omitted, the app inherits the top-level `approvals_reviewer` value:
|
||||
|
||||
```toml
|
||||
approvals_reviewer = "auto_review"
|
||||
|
||||
[apps.demo-app]
|
||||
approvals_reviewer = "user"
|
||||
```
|
||||
|
||||
Setting the app value to `"user"` routes its approval prompts to the user
|
||||
instead of Guardian; setting it to `"auto_review"` opts that app into Guardian
|
||||
review when allowed by configuration requirements.
|
||||
|
||||
Invoke an app by inserting `$<app-slug>` in the text input. The slug is derived from the app name and lowercased with non-alphanumeric characters replaced by `-` (for example, "Demo App" becomes `$demo-app`). Add a `mention` input item (recommended) so the server uses the exact `app://<connector-id>` path rather than guessing by name. Plugins use the same `mention` item shape, but with `plugin://<plugin-name>@<marketplace-name>` paths from `plugin/installed` or `plugin/list`.
|
||||
|
||||
Example:
|
||||
|
||||
@@ -284,6 +284,7 @@ async fn write_value_supports_nested_app_paths() -> Result<()> {
|
||||
"app1".to_string(),
|
||||
AppConfig {
|
||||
enabled: false,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: None,
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: Some(AppToolApproval::Prompt),
|
||||
|
||||
@@ -5,6 +5,7 @@ use app_test_support::test_tmp_path_buf;
|
||||
use app_test_support::to_response;
|
||||
use codex_app_server_protocol::AppConfig;
|
||||
use codex_app_server_protocol::AppToolApproval;
|
||||
use codex_app_server_protocol::ApprovalsReviewer;
|
||||
use codex_app_server_protocol::AppsConfig;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ConfigBatchWriteParams;
|
||||
@@ -333,6 +334,7 @@ async fn config_read_includes_apps() -> Result<()> {
|
||||
r#"
|
||||
[apps.app1]
|
||||
enabled = false
|
||||
approvals_reviewer = "user"
|
||||
destructive_enabled = false
|
||||
default_tools_approval_mode = "prompt"
|
||||
"#,
|
||||
@@ -368,6 +370,7 @@ default_tools_approval_mode = "prompt"
|
||||
"app1".to_string(),
|
||||
AppConfig {
|
||||
enabled: false,
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
destructive_enabled: Some(false),
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: Some(AppToolApproval::Prompt),
|
||||
@@ -384,6 +387,16 @@ default_tools_approval_mode = "prompt"
|
||||
profile: None,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
origins
|
||||
.get("apps.app1.approvals_reviewer")
|
||||
.expect("origin")
|
||||
.name,
|
||||
ConfigLayerSource::User {
|
||||
file: user_file.clone(),
|
||||
profile: None,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
origins
|
||||
.get("apps.app1.destructive_enabled")
|
||||
|
||||
@@ -21,7 +21,6 @@ use codex_config::Constrained;
|
||||
use codex_config::McpServerConfig;
|
||||
use codex_config::McpServerTransportConfig;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_config::types::OAuthCredentialsStoreMode;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_plugin::PluginCapabilitySummary;
|
||||
@@ -91,7 +90,6 @@ pub fn mcp_permission_prompt_is_auto_approved(
|
||||
|
||||
#[derive(Clone, Copy, Debug, Default, Eq, PartialEq)]
|
||||
pub struct McpPermissionPromptAutoApproveContext {
|
||||
pub approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
pub tool_approval_mode: Option<AppToolApproval>,
|
||||
}
|
||||
|
||||
|
||||
@@ -1,7 +1,6 @@
|
||||
use super::*;
|
||||
use codex_config::Constrained;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_plugin::AppConnectorId;
|
||||
use codex_plugin::PluginCapabilitySummary;
|
||||
@@ -96,7 +95,6 @@ fn mcp_prompt_auto_approval_honors_approved_tools_in_all_permission_modes() {
|
||||
approval_policy,
|
||||
&PermissionProfile::read_only(),
|
||||
McpPermissionPromptAutoApproveContext {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
tool_approval_mode: Some(AppToolApproval::Approve),
|
||||
},
|
||||
));
|
||||
@@ -106,7 +104,6 @@ fn mcp_prompt_auto_approval_honors_approved_tools_in_all_permission_modes() {
|
||||
AskForApproval::OnRequest,
|
||||
&PermissionProfile::read_only(),
|
||||
McpPermissionPromptAutoApproveContext {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
tool_approval_mode: Some(AppToolApproval::Auto),
|
||||
},
|
||||
));
|
||||
@@ -118,7 +115,6 @@ fn mcp_prompt_auto_approval_rejects_auto_mode_in_default_permission_mode() {
|
||||
AskForApproval::OnRequest,
|
||||
&PermissionProfile::read_only(),
|
||||
McpPermissionPromptAutoApproveContext {
|
||||
approvals_reviewer: Some(ApprovalsReviewer::User),
|
||||
tool_approval_mode: Some(AppToolApproval::Auto),
|
||||
},
|
||||
));
|
||||
|
||||
@@ -425,6 +425,10 @@ pub struct AppConfig {
|
||||
#[serde(default = "default_enabled")]
|
||||
pub enabled: bool,
|
||||
|
||||
/// Reviewer for approval prompts from this app, overriding the thread default.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
|
||||
/// Whether tools with `destructive_hint = true` are allowed for this app.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub destructive_enabled: Option<bool>,
|
||||
|
||||
@@ -102,6 +102,14 @@
|
||||
"additionalProperties": false,
|
||||
"description": "Config values for a single app/connector.",
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/ApprovalsReviewer"
|
||||
}
|
||||
],
|
||||
"description": "Reviewer for approval prompts from this app, overriding the thread default."
|
||||
},
|
||||
"default_tools_approval_mode": {
|
||||
"allOf": [
|
||||
{
|
||||
|
||||
@@ -34,6 +34,7 @@ use crate::config::Config;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::routes_approval_to_guardian_with_reviewer;
|
||||
use crate::guardian::spawn_approval_request_review;
|
||||
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT;
|
||||
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION;
|
||||
@@ -41,6 +42,7 @@ use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC;
|
||||
use crate::mcp_tool_call::build_guardian_mcp_tool_review_request;
|
||||
use crate::mcp_tool_call::is_mcp_tool_approval_question_id;
|
||||
use crate::mcp_tool_call::lookup_mcp_tool_metadata;
|
||||
use crate::mcp_tool_call::mcp_approvals_reviewer;
|
||||
use crate::session::Codex;
|
||||
use crate::session::CodexSpawnArgs;
|
||||
use crate::session::CodexSpawnOk;
|
||||
@@ -632,15 +634,14 @@ async fn handle_request_user_input(
|
||||
event: RequestUserInputEvent,
|
||||
cancel_token: &CancellationToken,
|
||||
) {
|
||||
if routes_approval_to_guardian(parent_ctx)
|
||||
&& let Some(response) = maybe_auto_review_mcp_request_user_input(
|
||||
parent_session,
|
||||
parent_ctx,
|
||||
pending_mcp_invocations,
|
||||
&event,
|
||||
cancel_token,
|
||||
)
|
||||
.await
|
||||
if let Some(response) = maybe_auto_review_mcp_request_user_input(
|
||||
parent_session,
|
||||
parent_ctx,
|
||||
pending_mcp_invocations,
|
||||
&event,
|
||||
cancel_token,
|
||||
)
|
||||
.await
|
||||
{
|
||||
let _ = codex.submit(Op::UserInputAnswer { id, response }).await;
|
||||
return;
|
||||
@@ -694,6 +695,11 @@ async fn maybe_auto_review_mcp_request_user_input(
|
||||
&invocation.tool,
|
||||
)
|
||||
.await;
|
||||
let approvals_reviewer =
|
||||
mcp_approvals_reviewer(parent_ctx, &invocation.server, metadata.as_ref());
|
||||
if !routes_approval_to_guardian_with_reviewer(parent_ctx, approvals_reviewer) {
|
||||
return None;
|
||||
}
|
||||
let review_cancel = cancel_token.child_token();
|
||||
let review_rx = spawn_approval_request_review(
|
||||
Arc::clone(parent_session),
|
||||
|
||||
@@ -2,6 +2,7 @@ use super::*;
|
||||
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_DECLINE_SYNTHETIC;
|
||||
use crate::mcp_tool_call::MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX;
|
||||
use async_channel::bounded;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::models::NetworkPermissions;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
@@ -445,3 +446,72 @@ async fn delegated_mcp_guardian_abort_returns_synthetic_decline_answer() {
|
||||
})
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn delegated_mcp_user_reviewer_waits_for_metadata_lookup() {
|
||||
let (parent_session, parent_ctx, _rx_events) =
|
||||
crate::session::tests::make_session_and_context_with_rx().await;
|
||||
let pending_mcp_invocations = Arc::new(Mutex::new(HashMap::from([(
|
||||
"call-1".to_string(),
|
||||
McpInvocation {
|
||||
server: CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
tool: "dangerous_tool".to_string(),
|
||||
arguments: None,
|
||||
},
|
||||
)])));
|
||||
let cancel_token = CancellationToken::new();
|
||||
let manager = Arc::clone(&parent_session.services.mcp_connection_manager);
|
||||
let (manager_locked_tx, manager_locked_rx) = std::sync::mpsc::sync_channel(0);
|
||||
let (release_manager_tx, release_manager_rx) = std::sync::mpsc::sync_channel(0);
|
||||
let manager_lock = tokio::task::spawn_blocking(move || {
|
||||
let _manager_guard = manager.blocking_write();
|
||||
manager_locked_tx
|
||||
.send(())
|
||||
.expect("manager lock receiver should remain open");
|
||||
release_manager_rx
|
||||
.recv()
|
||||
.expect("manager lock release sender should remain open");
|
||||
});
|
||||
manager_locked_rx
|
||||
.recv_timeout(Duration::from_secs(1))
|
||||
.expect("manager write lock should be acquired");
|
||||
|
||||
let event = RequestUserInputEvent {
|
||||
call_id: "call-1".to_string(),
|
||||
turn_id: "child-turn-1".to_string(),
|
||||
questions: vec![RequestUserInputQuestion {
|
||||
id: format!("{MCP_TOOL_APPROVAL_QUESTION_ID_PREFIX}_call-1"),
|
||||
header: "Approve app tool call?".to_string(),
|
||||
question: "Allow this app tool?".to_string(),
|
||||
is_other: false,
|
||||
is_secret: false,
|
||||
options: None,
|
||||
}],
|
||||
};
|
||||
let response = maybe_auto_review_mcp_request_user_input(
|
||||
&parent_session,
|
||||
&parent_ctx,
|
||||
&pending_mcp_invocations,
|
||||
&event,
|
||||
&cancel_token,
|
||||
);
|
||||
tokio::pin!(response);
|
||||
assert!(
|
||||
timeout(Duration::from_millis(100), &mut response)
|
||||
.await
|
||||
.is_err(),
|
||||
"manual reviewer should wait for MCP metadata"
|
||||
);
|
||||
release_manager_tx
|
||||
.send(())
|
||||
.expect("manager lock holder should remain open");
|
||||
manager_lock
|
||||
.await
|
||||
.expect("manager lock task should not panic");
|
||||
assert_eq!(
|
||||
timeout(Duration::from_secs(1), response)
|
||||
.await
|
||||
.expect("manual reviewer should finish after MCP metadata lookup"),
|
||||
None
|
||||
);
|
||||
}
|
||||
|
||||
@@ -26,6 +26,7 @@ use crate::plugins::list_tool_suggest_discoverable_plugins;
|
||||
use crate::session::INITIAL_SUBMIT_ID;
|
||||
use codex_config::AppsRequirementsToml;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_config::types::AppsConfigToml;
|
||||
use codex_config::types::ToolSuggestDiscoverableType;
|
||||
use codex_core_plugins::PluginsManager;
|
||||
@@ -571,6 +572,35 @@ pub(crate) fn codex_app_tool_is_enabled(config: &Config, tool_info: &ToolInfo) -
|
||||
.enabled
|
||||
}
|
||||
|
||||
pub(crate) fn mcp_approvals_reviewer(
|
||||
config: &Config,
|
||||
server_name: &str,
|
||||
connector_id: Option<&str>,
|
||||
) -> ApprovalsReviewer {
|
||||
let app_reviewer = if server_name == CODEX_APPS_MCP_SERVER_NAME {
|
||||
read_user_apps_config(config).and_then(|apps_config| {
|
||||
connector_id
|
||||
.and_then(|connector_id| apps_config.apps.get(connector_id))
|
||||
.and_then(|app| app.approvals_reviewer)
|
||||
})
|
||||
} else {
|
||||
None
|
||||
};
|
||||
|
||||
if let Some(reviewer) = app_reviewer
|
||||
&& config
|
||||
.config_layer_stack
|
||||
.requirements()
|
||||
.approvals_reviewer
|
||||
.can_set(&reviewer)
|
||||
.is_ok()
|
||||
{
|
||||
return reviewer;
|
||||
}
|
||||
|
||||
config.approvals_reviewer
|
||||
}
|
||||
|
||||
fn read_apps_config(config: &Config) -> Option<AppsConfigToml> {
|
||||
let apps_config = read_user_apps_config(config);
|
||||
let had_apps_config = apps_config.is_some();
|
||||
|
||||
@@ -12,6 +12,7 @@ use codex_config::ConfigRequirementsToml;
|
||||
use codex_config::types::AppConfig;
|
||||
use codex_config::types::AppToolConfig;
|
||||
use codex_config::types::AppToolsConfig;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_config::types::AppsDefaultConfig;
|
||||
use codex_connectors::merge::plugin_connector_to_app_info;
|
||||
use codex_connectors::metadata::connector_install_url;
|
||||
@@ -377,6 +378,7 @@ fn app_is_enabled_prefers_per_app_override_over_default() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: None,
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: None,
|
||||
@@ -390,6 +392,88 @@ fn app_is_enabled_prefers_per_app_override_over_default() {
|
||||
assert!(!app_is_enabled(&apps_config, Some("drive")));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_approvals_reviewer_overrides_global_reviewer() {
|
||||
for (global, app, expected_global, expected_app) in [
|
||||
(
|
||||
"user",
|
||||
"auto_review",
|
||||
ApprovalsReviewer::User,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
),
|
||||
(
|
||||
"auto_review",
|
||||
"user",
|
||||
ApprovalsReviewer::AutoReview,
|
||||
ApprovalsReviewer::User,
|
||||
),
|
||||
] {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
format!(
|
||||
r#"
|
||||
approvals_reviewer = "{global}"
|
||||
|
||||
[apps.calendar]
|
||||
approvals_reviewer = "{app}"
|
||||
"#
|
||||
),
|
||||
)
|
||||
.expect("write config");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("config should build");
|
||||
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
|
||||
expected_app
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("drive")),
|
||||
expected_global
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, "custom_server", Some("calendar")),
|
||||
expected_global
|
||||
);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_approvals_reviewer_respects_global_reviewer_requirements() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
std::fs::write(
|
||||
codex_home.path().join(CONFIG_TOML_FILE),
|
||||
r#"
|
||||
approvals_reviewer = "auto_review"
|
||||
|
||||
[apps.calendar]
|
||||
approvals_reviewer = "user"
|
||||
"#,
|
||||
)
|
||||
.expect("write config");
|
||||
let requirements = ConfigRequirementsToml {
|
||||
allowed_approvals_reviewers: Some(vec![ApprovalsReviewer::AutoReview]),
|
||||
..Default::default()
|
||||
};
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_requirements(CloudRequirementsLoader::new(async move {
|
||||
Ok(Some(requirements))
|
||||
}))
|
||||
.build()
|
||||
.await
|
||||
.expect("config should build");
|
||||
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
|
||||
ApprovalsReviewer::AutoReview
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn requirements_disabled_connector_overrides_enabled_connector() {
|
||||
let mut effective_apps = AppsConfigToml {
|
||||
@@ -932,6 +1016,7 @@ fn app_tool_policy_allows_per_app_enable_when_default_is_disabled() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: None,
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: None,
|
||||
@@ -969,6 +1054,7 @@ fn app_tool_policy_per_tool_enabled_true_overrides_app_level_disable_flags() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: Some(false),
|
||||
open_world_enabled: Some(false),
|
||||
default_tools_approval_mode: None,
|
||||
@@ -1012,6 +1098,7 @@ fn app_tool_policy_default_tools_enabled_true_overrides_app_level_tool_hints() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: Some(false),
|
||||
open_world_enabled: Some(false),
|
||||
default_tools_approval_mode: None,
|
||||
@@ -1047,6 +1134,7 @@ fn app_tool_policy_default_tools_enabled_false_overrides_app_level_tool_hints()
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: Some(true),
|
||||
open_world_enabled: Some(true),
|
||||
default_tools_approval_mode: Some(AppToolApproval::Approve),
|
||||
@@ -1084,6 +1172,7 @@ fn app_tool_policy_uses_default_tools_approval_mode() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: None,
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: Some(AppToolApproval::Prompt),
|
||||
@@ -1123,6 +1212,7 @@ fn app_tool_policy_matches_prefix_stripped_tool_name_for_tool_config() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: Some(false),
|
||||
open_world_enabled: Some(false),
|
||||
default_tools_approval_mode: Some(AppToolApproval::Auto),
|
||||
|
||||
@@ -39,6 +39,7 @@ pub(crate) use review::review_approval_request;
|
||||
#[cfg(test)]
|
||||
pub(crate) use review::review_approval_request_with_cancel;
|
||||
pub(crate) use review::routes_approval_to_guardian;
|
||||
pub(crate) use review::routes_approval_to_guardian_with_reviewer;
|
||||
pub(crate) use review::spawn_approval_request_review;
|
||||
pub(crate) use review_session::GuardianReviewSessionManager;
|
||||
pub(crate) use review_session::prompt_cache_key_override_for_review_session;
|
||||
|
||||
@@ -145,10 +145,18 @@ fn guardian_risk_level_str(level: GuardianRiskLevel) -> &'static str {
|
||||
/// reviewer instead of surfacing them to the user. ARC may still block actions
|
||||
/// earlier in the flow.
|
||||
pub(crate) fn routes_approval_to_guardian(turn: &TurnContext) -> bool {
|
||||
routes_approval_to_guardian_with_reviewer(turn, turn.config.approvals_reviewer)
|
||||
}
|
||||
|
||||
/// Whether an approval with its own reviewer selection should be routed through guardian.
|
||||
pub(crate) fn routes_approval_to_guardian_with_reviewer(
|
||||
turn: &TurnContext,
|
||||
approvals_reviewer: ApprovalsReviewer,
|
||||
) -> bool {
|
||||
matches!(
|
||||
turn.approval_policy.value(),
|
||||
AskForApproval::OnRequest | AskForApproval::Granular(_)
|
||||
) && turn.config.approvals_reviewer == ApprovalsReviewer::AutoReview
|
||||
) && approvals_reviewer == ApprovalsReviewer::AutoReview
|
||||
}
|
||||
|
||||
pub(crate) fn is_guardian_reviewer_source(
|
||||
|
||||
@@ -1099,6 +1099,20 @@ async fn routes_approval_to_guardian_requires_guardian_reviewer() {
|
||||
assert!(routes_approval_to_guardian(&turn));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn routes_approval_to_guardian_can_use_app_reviewer_override() {
|
||||
let (_session, turn) = crate::session::tests::make_session_and_context().await;
|
||||
|
||||
assert!(!routes_approval_to_guardian_with_reviewer(
|
||||
&turn,
|
||||
ApprovalsReviewer::User
|
||||
));
|
||||
assert!(routes_approval_to_guardian_with_reviewer(
|
||||
&turn,
|
||||
ApprovalsReviewer::AutoReview
|
||||
));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn routes_approval_to_guardian_allows_granular_review_policy() {
|
||||
let (_session, mut turn) = crate::session::tests::make_session_and_context().await;
|
||||
|
||||
@@ -13,7 +13,7 @@ use crate::guardian::guardian_rejection_message;
|
||||
use crate::guardian::guardian_timeout_message;
|
||||
use crate::guardian::new_guardian_review_id;
|
||||
use crate::guardian::review_approval_request;
|
||||
use crate::guardian::routes_approval_to_guardian;
|
||||
use crate::guardian::routes_approval_to_guardian_with_reviewer;
|
||||
use crate::hook_runtime::run_permission_request_hooks;
|
||||
use crate::mcp_openai_file::rewrite_mcp_tool_arguments_for_openai_files;
|
||||
use crate::mcp_tool_approval_templates::RenderedMcpToolApprovalParam;
|
||||
@@ -32,6 +32,7 @@ use codex_app_server_protocol::McpElicitationSchema;
|
||||
use codex_app_server_protocol::McpServerElicitationRequest;
|
||||
use codex_app_server_protocol::McpServerElicitationRequestParams;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_config::types::ApprovalsReviewer;
|
||||
use codex_features::Feature;
|
||||
use codex_hooks::PermissionRequestDecision;
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
@@ -1162,11 +1163,11 @@ async fn maybe_request_mcp_tool_approval(
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> Option<McpToolApprovalDecision> {
|
||||
let approvals_reviewer = mcp_approvals_reviewer(turn_context, &invocation.server, metadata);
|
||||
if mcp_permission_prompt_is_auto_approved(
|
||||
turn_context.approval_policy.value(),
|
||||
&turn_context.permission_profile(),
|
||||
McpPermissionPromptAutoApproveContext {
|
||||
approvals_reviewer: Some(turn_context.config.approvals_reviewer),
|
||||
tool_approval_mode: Some(approval_mode),
|
||||
},
|
||||
) {
|
||||
@@ -1218,7 +1219,7 @@ async fn maybe_request_mcp_tool_approval(
|
||||
.features
|
||||
.enabled(Feature::ToolCallMcpElicitation);
|
||||
|
||||
if routes_approval_to_guardian(turn_context) {
|
||||
if routes_approval_to_guardian_with_reviewer(turn_context, approvals_reviewer) {
|
||||
let review_id = new_guardian_review_id();
|
||||
let decision = review_approval_request(
|
||||
sess,
|
||||
@@ -1328,6 +1329,18 @@ async fn maybe_request_mcp_tool_approval(
|
||||
Some(decision)
|
||||
}
|
||||
|
||||
pub(crate) fn mcp_approvals_reviewer(
|
||||
turn_context: &TurnContext,
|
||||
server_name: &str,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
) -> ApprovalsReviewer {
|
||||
connectors::mcp_approvals_reviewer(
|
||||
turn_context.config.as_ref(),
|
||||
server_name,
|
||||
metadata.and_then(|metadata| metadata.connector_id.as_deref()),
|
||||
)
|
||||
}
|
||||
|
||||
fn session_mcp_tool_approval_key(
|
||||
invocation: &McpInvocation,
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
|
||||
@@ -1886,6 +1886,7 @@ async fn persist_codex_app_tool_approval_writes_tool_override() {
|
||||
"calendar".to_string(),
|
||||
AppConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: None,
|
||||
open_world_enabled: None,
|
||||
default_tools_approval_mode: None,
|
||||
|
||||
@@ -455,7 +455,15 @@ async fn review_guardian_mcp_elicitation(
|
||||
return Ok(None);
|
||||
};
|
||||
|
||||
if !crate::guardian::routes_approval_to_guardian(turn_context.as_ref()) {
|
||||
let approvals_reviewer = crate::connectors::mcp_approvals_reviewer(
|
||||
turn_context.config.as_ref(),
|
||||
request.server_name.as_str(),
|
||||
elicitation_connector_id(&request.elicitation),
|
||||
);
|
||||
if !crate::guardian::routes_approval_to_guardian_with_reviewer(
|
||||
turn_context.as_ref(),
|
||||
approvals_reviewer,
|
||||
) {
|
||||
return Ok(None);
|
||||
}
|
||||
|
||||
@@ -567,6 +575,15 @@ fn guardian_elicitation_review_request(
|
||||
))
|
||||
}
|
||||
|
||||
fn elicitation_connector_id(elicitation: &CreateElicitationRequestParams) -> Option<&str> {
|
||||
match elicitation {
|
||||
CreateElicitationRequestParams::FormElicitationParams { meta, .. }
|
||||
| CreateElicitationRequestParams::UrlElicitationParams { meta, .. } => meta
|
||||
.as_ref()
|
||||
.and_then(|meta| metadata_str(&meta.0, MCP_ELICITATION_CONNECTOR_ID_KEY)),
|
||||
}
|
||||
}
|
||||
|
||||
fn meta_requests_approval_request(meta: &Option<Meta>) -> bool {
|
||||
meta.as_ref()
|
||||
.and_then(|meta| metadata_str(&meta.0, MCP_ELICITATION_REQUEST_TYPE_KEY))
|
||||
|
||||
Reference in New Issue
Block a user