mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[ez][codex-rs] Support approvals reviewer in app defaults (#27075)
[from codex] ## Summary - add `approvals_reviewer` support to `[apps._default]` - resolve connected-app reviewers in per-app, app-default, then global order - expose the setting through the v2 config API and regenerate schema fixtures ## Context PR #25167 added `apps.<connector_id>.approvals_reviewer`, but the shared app defaults table could not specify the reviewer. This extends the same behavior to `[apps._default]` while preserving per-app overrides. Managed `allowed_approvals_reviewers` requirements still constrain both default and per-app values. A disallowed app value falls back to the global reviewer, and non-app MCP servers continue using the global reviewer. ## Testing - `just write-config-schema` - `just write-app-server-schema` - `just fmt` - `just test -p codex-config` - `just test -p codex-core app_approvals_reviewer` - `just test -p codex-app-server-protocol` - `just test -p codex-app-server config_read_includes_apps`
This commit is contained in:
committed by
GitHub
Unverified
parent
5b8e3c6d40
commit
3cac2e0d3f
+10
@@ -6584,6 +6584,16 @@
|
||||
},
|
||||
"AppsDefaultConfig": {
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/v2/ApprovalsReviewer"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"destructive_enabled": {
|
||||
"default": true,
|
||||
"type": "boolean"
|
||||
|
||||
+10
@@ -853,6 +853,16 @@
|
||||
},
|
||||
"AppsDefaultConfig": {
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/ApprovalsReviewer"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"destructive_enabled": {
|
||||
"default": true,
|
||||
"type": "boolean"
|
||||
|
||||
@@ -133,6 +133,16 @@
|
||||
},
|
||||
"AppsDefaultConfig": {
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"anyOf": [
|
||||
{
|
||||
"$ref": "#/definitions/ApprovalsReviewer"
|
||||
},
|
||||
{
|
||||
"type": "null"
|
||||
}
|
||||
]
|
||||
},
|
||||
"destructive_enabled": {
|
||||
"default": true,
|
||||
"type": "boolean"
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
// GENERATED CODE! DO NOT MODIFY BY HAND!
|
||||
|
||||
// This file was generated by [ts-rs](https://github.com/Aleph-Alpha/ts-rs). Do not edit this file manually.
|
||||
import type { ApprovalsReviewer } from "./ApprovalsReviewer";
|
||||
|
||||
export type AppsDefaultConfig = { enabled: boolean, destructive_enabled: boolean, open_world_enabled: boolean, };
|
||||
export type AppsDefaultConfig = { enabled: boolean, approvals_reviewer: ApprovalsReviewer | null, destructive_enabled: boolean, open_world_enabled: boolean, };
|
||||
|
||||
@@ -172,6 +172,7 @@ pub enum AppToolApproval {
|
||||
pub struct AppsDefaultConfig {
|
||||
#[serde(default = "default_enabled")]
|
||||
pub enabled: bool,
|
||||
pub approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
#[serde(default = "default_enabled")]
|
||||
pub destructive_enabled: bool,
|
||||
#[serde(default = "default_enabled")]
|
||||
|
||||
@@ -1775,13 +1775,18 @@ 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:
|
||||
Use `apps._default.approvals_reviewer` to set the reviewer for all apps, and a
|
||||
per-app value to override that default. When both are omitted, the app inherits
|
||||
the top-level `approvals_reviewer` value:
|
||||
|
||||
```toml
|
||||
approvals_reviewer = "auto_review"
|
||||
|
||||
[apps.demo-app]
|
||||
[apps._default]
|
||||
approvals_reviewer = "user"
|
||||
|
||||
[apps.demo-app]
|
||||
approvals_reviewer = "auto_review"
|
||||
```
|
||||
|
||||
Setting the app value to `"user"` routes its approval prompts to the user
|
||||
|
||||
@@ -7,6 +7,7 @@ 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::AppsDefaultConfig;
|
||||
use codex_app_server_protocol::AskForApproval;
|
||||
use codex_app_server_protocol::ConfigBatchWriteParams;
|
||||
use codex_app_server_protocol::ConfigEdit;
|
||||
@@ -332,6 +333,9 @@ async fn config_read_includes_apps() -> Result<()> {
|
||||
write_config(
|
||||
&codex_home,
|
||||
r#"
|
||||
[apps._default]
|
||||
approvals_reviewer = "auto_review"
|
||||
|
||||
[apps.app1]
|
||||
enabled = false
|
||||
approvals_reviewer = "user"
|
||||
@@ -365,7 +369,12 @@ default_tools_approval_mode = "prompt"
|
||||
assert_eq!(
|
||||
config.apps,
|
||||
Some(AppsConfig {
|
||||
default: None,
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: Some(ApprovalsReviewer::AutoReview),
|
||||
destructive_enabled: true,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
apps: std::collections::HashMap::from([(
|
||||
"app1".to_string(),
|
||||
AppConfig {
|
||||
@@ -380,6 +389,16 @@ default_tools_approval_mode = "prompt"
|
||||
)]),
|
||||
})
|
||||
);
|
||||
assert_eq!(
|
||||
origins
|
||||
.get("apps._default.approvals_reviewer")
|
||||
.expect("origin")
|
||||
.name,
|
||||
ConfigLayerSource::User {
|
||||
file: user_file.clone(),
|
||||
profile: None,
|
||||
}
|
||||
);
|
||||
assert_eq!(
|
||||
origins.get("apps.app1.enabled").expect("origin").name,
|
||||
ConfigLayerSource::User {
|
||||
|
||||
@@ -380,6 +380,10 @@ pub struct AppsDefaultConfig {
|
||||
#[serde(default = "default_enabled")]
|
||||
pub enabled: bool,
|
||||
|
||||
/// Reviewer for approval prompts unless overridden by per-app settings.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub approvals_reviewer: Option<ApprovalsReviewer>,
|
||||
|
||||
/// Whether tools with `destructive_hint = true` are allowed by default.
|
||||
#[serde(
|
||||
default = "default_enabled",
|
||||
|
||||
@@ -210,6 +210,14 @@
|
||||
"additionalProperties": false,
|
||||
"description": "Default settings that apply to all apps.",
|
||||
"properties": {
|
||||
"approvals_reviewer": {
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/ApprovalsReviewer"
|
||||
}
|
||||
],
|
||||
"description": "Reviewer for approval prompts unless overridden by per-app settings."
|
||||
},
|
||||
"destructive_enabled": {
|
||||
"description": "Whether tools with `destructive_hint = true` are allowed by default.",
|
||||
"type": "boolean"
|
||||
|
||||
@@ -601,6 +601,11 @@ pub(crate) fn mcp_approvals_reviewer(
|
||||
connector_id
|
||||
.and_then(|connector_id| apps_config.apps.get(connector_id))
|
||||
.and_then(|app| app.approvals_reviewer)
|
||||
.or_else(|| {
|
||||
apps_config
|
||||
.default
|
||||
.and_then(|defaults| defaults.approvals_reviewer)
|
||||
})
|
||||
})
|
||||
} else {
|
||||
None
|
||||
|
||||
@@ -269,6 +269,7 @@ fn app_tool_policy_uses_global_defaults_for_destructive_hints() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: false,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
@@ -298,6 +299,7 @@ fn app_tool_policy_defaults_missing_destructive_hint_to_true() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: false,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
@@ -327,6 +329,7 @@ fn app_tool_policy_defaults_missing_open_world_hint_to_true() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: true,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: true,
|
||||
open_world_enabled: false,
|
||||
}),
|
||||
@@ -356,6 +359,7 @@ fn app_is_enabled_uses_default_for_unconfigured_apps() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: false,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: true,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
@@ -371,6 +375,7 @@ fn app_is_enabled_prefers_per_app_override_over_default() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: false,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: true,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
@@ -393,19 +398,23 @@ fn app_is_enabled_prefers_per_app_override_over_default() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_approvals_reviewer_overrides_global_reviewer() {
|
||||
for (global, app, expected_global, expected_app) in [
|
||||
async fn app_approvals_reviewer_uses_app_then_default_then_global() {
|
||||
for (global, app_default, app, expected_global, expected_default, expected_app) in [
|
||||
(
|
||||
"user",
|
||||
"auto_review",
|
||||
"user",
|
||||
ApprovalsReviewer::User,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
ApprovalsReviewer::User,
|
||||
),
|
||||
(
|
||||
"auto_review",
|
||||
"user",
|
||||
"auto_review",
|
||||
ApprovalsReviewer::AutoReview,
|
||||
ApprovalsReviewer::User,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
),
|
||||
] {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
@@ -415,6 +424,9 @@ async fn app_approvals_reviewer_overrides_global_reviewer() {
|
||||
r#"
|
||||
approvals_reviewer = "{global}"
|
||||
|
||||
[apps._default]
|
||||
approvals_reviewer = "{app_default}"
|
||||
|
||||
[apps.calendar]
|
||||
approvals_reviewer = "{app}"
|
||||
"#
|
||||
@@ -433,7 +445,15 @@ approvals_reviewer = "{app}"
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("drive")),
|
||||
expected_global
|
||||
expected_default
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(
|
||||
&config,
|
||||
CODEX_APPS_MCP_SERVER_NAME,
|
||||
/*connector_id*/ None
|
||||
),
|
||||
expected_default
|
||||
);
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, "custom_server", Some("calendar")),
|
||||
@@ -442,6 +462,36 @@ approvals_reviewer = "{app}"
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn default_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._default]
|
||||
approvals_reviewer = "user"
|
||||
"#,
|
||||
)
|
||||
.expect("write config");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home.path().to_path_buf())
|
||||
.cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"allowed_approvals_reviewers = ["auto_review"]"#,
|
||||
),
|
||||
)
|
||||
.build()
|
||||
.await
|
||||
.expect("config should build");
|
||||
|
||||
assert_eq!(
|
||||
mcp_approvals_reviewer(&config, CODEX_APPS_MCP_SERVER_NAME, Some("calendar")),
|
||||
ApprovalsReviewer::AutoReview
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn app_approvals_reviewer_respects_global_reviewer_requirements() {
|
||||
let codex_home = tempdir().expect("tempdir should succeed");
|
||||
@@ -756,6 +806,7 @@ fn app_tool_policy_honors_default_app_enabled_false() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: false,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: true,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
@@ -987,6 +1038,7 @@ fn app_tool_policy_allows_per_app_enable_when_default_is_disabled() {
|
||||
let apps_config = AppsConfigToml {
|
||||
default: Some(AppsDefaultConfig {
|
||||
enabled: false,
|
||||
approvals_reviewer: None,
|
||||
destructive_enabled: true,
|
||||
open_world_enabled: true,
|
||||
}),
|
||||
|
||||
@@ -5,6 +5,7 @@ use anyhow::Result;
|
||||
use codex_config::types::AppToolApproval;
|
||||
use codex_core::config::Config;
|
||||
use codex_features::Feature;
|
||||
use codex_protocol::config_types::ApprovalsReviewer;
|
||||
use codex_protocol::config_types::CollaborationMode;
|
||||
use codex_protocol::config_types::ModeKind;
|
||||
use codex_protocol::config_types::Settings;
|
||||
@@ -59,6 +60,32 @@ default_tools_approval_mode = "{approval_mode}"
|
||||
.with_user_config(&user_config_path, user_config);
|
||||
}
|
||||
|
||||
fn set_calendar_approval_mode_and_default_reviewer(
|
||||
config: &mut Config,
|
||||
approval_mode: AppToolApproval,
|
||||
default_approvals_reviewer: ApprovalsReviewer,
|
||||
) {
|
||||
let approval_mode = match approval_mode {
|
||||
AppToolApproval::Auto => "auto",
|
||||
AppToolApproval::Prompt => "prompt",
|
||||
AppToolApproval::Approve => "approve",
|
||||
};
|
||||
let user_config_path = config.codex_home.join("config.toml").abs();
|
||||
let user_config = toml::from_str(&format!(
|
||||
r#"
|
||||
[apps._default]
|
||||
approvals_reviewer = "{default_approvals_reviewer}"
|
||||
|
||||
[apps.calendar]
|
||||
default_tools_approval_mode = "{approval_mode}"
|
||||
"#
|
||||
))
|
||||
.expect("apps config should parse");
|
||||
config.config_layer_stack = config
|
||||
.config_layer_stack
|
||||
.with_user_config(&user_config_path, user_config);
|
||||
}
|
||||
|
||||
async fn submit_user_turn(
|
||||
test: &TestCodex,
|
||||
text: &str,
|
||||
@@ -134,11 +161,17 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
|
||||
|
||||
let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone())
|
||||
.with_config(|config| {
|
||||
// Use the opposite global reviewer so this route must come from apps._default.
|
||||
config.approvals_reviewer = ApprovalsReviewer::AutoReview;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::ToolCallMcpElicitation)
|
||||
.expect("test config should allow feature update");
|
||||
set_calendar_approval_mode(config, AppToolApproval::Prompt);
|
||||
set_calendar_approval_mode_and_default_reviewer(
|
||||
config,
|
||||
AppToolApproval::Prompt,
|
||||
ApprovalsReviewer::User,
|
||||
);
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
@@ -160,11 +193,14 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
|
||||
assert_eq!(begin.call_id, call_id);
|
||||
|
||||
let EventMsg::ElicitationRequest(request) = wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::ElicitationRequest(_))
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await
|
||||
else {
|
||||
unreachable!("event guard guarantees ElicitationRequest");
|
||||
panic!("expected apps._default user to route the app approval to the user");
|
||||
};
|
||||
|
||||
test.codex
|
||||
@@ -194,6 +230,110 @@ async fn approved_mcp_tool_call_metadata_records_prior_user_input_request() -> R
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn apps_default_auto_review_routes_actual_mcp_approval_to_guardian() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let call_id = "calendar-default-auto-review";
|
||||
let calendar_args = serde_json::to_string(&json!({
|
||||
"title": "Lunch",
|
||||
"starts_at": "2026-03-10T12:00:00Z"
|
||||
}))?;
|
||||
let responses = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-tool"),
|
||||
ev_function_call_with_namespace(
|
||||
call_id,
|
||||
SEARCH_CALENDAR_NAMESPACE,
|
||||
SEARCH_CALENDAR_CREATE_TOOL,
|
||||
&calendar_args,
|
||||
),
|
||||
ev_completed("resp-parent-tool"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-guardian-review"),
|
||||
ev_assistant_message(
|
||||
"msg-guardian-review",
|
||||
&json!({
|
||||
"risk_level": "low",
|
||||
"user_authorization": "high",
|
||||
"outcome": "allow",
|
||||
"rationale": "Creating this calendar event is low risk.",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp-guardian-review"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp-parent-done"),
|
||||
ev_assistant_message("msg-parent-done", "done"),
|
||||
ev_completed("resp-parent-done"),
|
||||
]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = search_capable_apps_builder(apps_server.chatgpt_base_url.clone())
|
||||
.with_config(|config| {
|
||||
// Use the opposite global reviewer so this route must come from apps._default.
|
||||
config.approvals_reviewer = ApprovalsReviewer::User;
|
||||
config
|
||||
.features
|
||||
.enable(Feature::ToolCallMcpElicitation)
|
||||
.expect("test config should allow feature update");
|
||||
set_calendar_approval_mode_and_default_reviewer(
|
||||
config,
|
||||
AppToolApproval::Prompt,
|
||||
ApprovalsReviewer::AutoReview,
|
||||
);
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
submit_user_turn(
|
||||
&test,
|
||||
"Use [$calendar](app://calendar) to create a calendar event.",
|
||||
AskForApproval::OnRequest,
|
||||
/*collaboration_mode*/ None,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let route_event = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::ElicitationRequest(_) | EventMsg::TurnComplete(_)
|
||||
)
|
||||
})
|
||||
.await;
|
||||
assert!(
|
||||
matches!(route_event, EventMsg::TurnComplete(_)),
|
||||
"expected apps._default auto_review to route the app approval to Guardian"
|
||||
);
|
||||
|
||||
let guardian_request = responses
|
||||
.requests()
|
||||
.into_iter()
|
||||
.find(|request| {
|
||||
request
|
||||
.instructions_text()
|
||||
.starts_with("You are judging one planned coding-agent action.")
|
||||
})
|
||||
.expect("expected a Guardian request for the app MCP approval");
|
||||
assert!(guardian_request.body_contains_text("calendar_create_event"));
|
||||
assert!(guardian_request.body_contains_text("Lunch"));
|
||||
|
||||
let apps_tool_call = recorded_apps_tool_call_by_call_id(&server, call_id).await;
|
||||
assert_eq!(
|
||||
apps_tool_call.pointer("/params/arguments/title"),
|
||||
Some(&json!("Lunch"))
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn mcp_tool_call_metadata_records_prior_request_user_input_tool() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
Reference in New Issue
Block a user