Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 1 addition & 0 deletions codex-rs/app-server-protocol/src/protocol/v2/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
14 changes: 14 additions & 0 deletions codex-rs/app-server/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Expand Down
1 change: 1 addition & 0 deletions codex-rs/app-server/src/config_manager_service_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand Down
13 changes: 13 additions & 0 deletions codex-rs/app-server/tests/suite/v2/config_rpc.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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"
"#,
Expand Down Expand Up @@ -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),
Expand All @@ -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")
Expand Down
2 changes: 0 additions & 2 deletions codex-rs/codex-mcp/src/mcp/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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>,
}

Expand Down
4 changes: 0 additions & 4 deletions codex-rs/codex-mcp/src/mcp/mod_tests.rs
Original file line number Diff line number Diff line change
@@ -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;
Expand Down Expand Up @@ -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),
},
));
Expand All @@ -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),
},
));
Expand All @@ -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),
},
));
Expand Down
4 changes: 4 additions & 0 deletions codex-rs/config/src/types.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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>,
Expand Down
8 changes: 8 additions & 0 deletions codex-rs/core/config.schema.json
Original file line number Diff line number Diff line change
Expand Up @@ -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": [
{
Expand Down
24 changes: 15 additions & 9 deletions codex-rs/core/src/codex_delegate.rs
Original file line number Diff line number Diff line change
Expand Up @@ -34,13 +34,15 @@ 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;
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;
Expand Down Expand Up @@ -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;
Expand Down Expand Up @@ -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),
Expand Down
70 changes: 70 additions & 0 deletions codex-rs/core/src/codex_delegate_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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
);
}
30 changes: 30 additions & 0 deletions codex-rs/core/src/connectors.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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();
Expand Down
Loading
Loading