mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[mcp] Improve custom MCP elicitation (#15800)
- [x] Support don't ask again for custom MCP tool calls. - [x] Don't run arc in yolo mode. - [x] Run arc for custom MCP tools in always allow mode.
This commit is contained in:
committed by
GitHub
Unverified
parent
d7e35e56cf
commit
78799c1bcf
@@ -1012,6 +1012,11 @@ Order of messages:
|
||||
|
||||
`turnId` is best-effort. When the elicitation is correlated with an active turn, the request includes that turn id; otherwise it is `null`.
|
||||
|
||||
For MCP tool approval elicitations, form request `meta` includes
|
||||
`codex_approval_kind: "mcp_tool_call"` and may include `persist: "session"`,
|
||||
`persist: "always"`, or `persist: ["session", "always"]` to advertise whether
|
||||
the client can offer session-scoped and/or persistent approval choices.
|
||||
|
||||
### Permission requests
|
||||
|
||||
The built-in `request_permissions` tool sends an `item/permissions/requestApproval` JSON-RPC request to the client with the requested permission profile. This v2 payload mirrors the standalone tool's narrower permission shape, so it can request network access and additional filesystem access but does not include the broader `macos` branch used by command-execution `additionalPermissions`.
|
||||
|
||||
@@ -130,7 +130,7 @@ impl ConfigApi {
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
async fn load_latest_config(
|
||||
pub(crate) async fn load_latest_config(
|
||||
&self,
|
||||
fallback_cwd: Option<PathBuf>,
|
||||
) -> Result<Config, JSONRPCErrorError> {
|
||||
|
||||
@@ -20,6 +20,7 @@ use crate::outgoing_message::OutgoingMessageSender;
|
||||
use crate::outgoing_message::RequestContext;
|
||||
use crate::transport::AppServerTransport;
|
||||
use async_trait::async_trait;
|
||||
use codex_app_server_protocol::AppListUpdatedNotification;
|
||||
use codex_app_server_protocol::ChatgptAuthTokensRefreshParams;
|
||||
use codex_app_server_protocol::ChatgptAuthTokensRefreshReason;
|
||||
use codex_app_server_protocol::ChatgptAuthTokensRefreshResponse;
|
||||
@@ -53,6 +54,7 @@ use codex_app_server_protocol::ServerNotification;
|
||||
use codex_app_server_protocol::ServerRequestPayload;
|
||||
use codex_app_server_protocol::experimental_required_message;
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_chatgpt::connectors;
|
||||
use codex_core::AnalyticsEventsClient;
|
||||
use codex_core::AuthManager;
|
||||
use codex_core::ThreadManager;
|
||||
@@ -880,13 +882,87 @@ impl MessageProcessor {
|
||||
request_id: ConnectionRequestId,
|
||||
params: ExperimentalFeatureEnablementSetParams,
|
||||
) {
|
||||
self.handle_config_mutation_result(
|
||||
request_id,
|
||||
self.config_api
|
||||
.set_experimental_feature_enablement(params)
|
||||
.await,
|
||||
)
|
||||
.await;
|
||||
let should_refresh_apps_list = params.enablement.get("apps").copied() == Some(true);
|
||||
match self
|
||||
.config_api
|
||||
.set_experimental_feature_enablement(params)
|
||||
.await
|
||||
{
|
||||
Ok(response) => {
|
||||
self.codex_message_processor.clear_plugin_related_caches();
|
||||
self.codex_message_processor
|
||||
.maybe_start_plugin_startup_tasks_for_latest_config()
|
||||
.await;
|
||||
self.outgoing.send_response(request_id, response).await;
|
||||
if should_refresh_apps_list {
|
||||
self.refresh_apps_list_after_experimental_feature_enablement_set()
|
||||
.await;
|
||||
}
|
||||
}
|
||||
Err(error) => self.outgoing.send_error(request_id, error).await,
|
||||
}
|
||||
}
|
||||
|
||||
async fn refresh_apps_list_after_experimental_feature_enablement_set(&self) {
|
||||
let config = match self
|
||||
.config_api
|
||||
.load_latest_config(/*fallback_cwd*/ None)
|
||||
.await
|
||||
{
|
||||
Ok(config) => config,
|
||||
Err(error) => {
|
||||
tracing::warn!(
|
||||
"failed to load config for apps list refresh after experimental feature enablement: {}",
|
||||
error.message
|
||||
);
|
||||
return;
|
||||
}
|
||||
};
|
||||
if !config.features.apps_enabled(Some(&self.auth_manager)).await {
|
||||
return;
|
||||
}
|
||||
|
||||
let outgoing = Arc::clone(&self.outgoing);
|
||||
tokio::spawn(async move {
|
||||
let (all_connectors_result, accessible_connectors_result) = tokio::join!(
|
||||
connectors::list_all_connectors_with_options(&config, /*force_refetch*/ true),
|
||||
connectors::list_accessible_connectors_from_mcp_tools_with_options(
|
||||
&config, /*force_refetch*/ true,
|
||||
),
|
||||
);
|
||||
let all_connectors = match all_connectors_result {
|
||||
Ok(connectors) => connectors,
|
||||
Err(err) => {
|
||||
tracing::warn!(
|
||||
"failed to force-refresh directory apps after experimental feature enablement: {err:#}"
|
||||
);
|
||||
return;
|
||||
}
|
||||
};
|
||||
let accessible_connectors = match accessible_connectors_result {
|
||||
Ok(connectors) => connectors,
|
||||
Err(err) => {
|
||||
tracing::warn!(
|
||||
"failed to force-refresh accessible apps after experimental feature enablement: {err:#}"
|
||||
);
|
||||
return;
|
||||
}
|
||||
};
|
||||
|
||||
let data = connectors::with_app_enabled_state(
|
||||
connectors::merge_connectors_with_accessible(
|
||||
all_connectors,
|
||||
accessible_connectors,
|
||||
/*all_connectors_loaded*/ true,
|
||||
),
|
||||
&config,
|
||||
);
|
||||
outgoing
|
||||
.send_server_notification(ServerNotification::AppListUpdated(
|
||||
AppListUpdatedNotification { data },
|
||||
))
|
||||
.await;
|
||||
});
|
||||
}
|
||||
|
||||
async fn handle_config_mutation_result<T: serde::Serialize>(
|
||||
|
||||
@@ -1,4 +1,5 @@
|
||||
use std::borrow::Cow;
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
use std::sync::Mutex as StdMutex;
|
||||
@@ -27,6 +28,7 @@ use codex_app_server_protocol::AppScreenshot;
|
||||
use codex_app_server_protocol::AppsListParams;
|
||||
use codex_app_server_protocol::AppsListResponse;
|
||||
use codex_app_server_protocol::AuthMode;
|
||||
use codex_app_server_protocol::ExperimentalFeatureEnablementSetParams;
|
||||
use codex_app_server_protocol::JSONRPCError;
|
||||
use codex_app_server_protocol::JSONRPCResponse;
|
||||
use codex_app_server_protocol::RequestId;
|
||||
@@ -1201,6 +1203,108 @@ async fn list_apps_force_refetch_patches_updates_from_cached_snapshots() -> Resu
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn experimental_feature_enablement_set_refreshes_apps_list_when_apps_turn_on() -> Result<()> {
|
||||
let initial_connectors = vec![AppInfo {
|
||||
id: "alpha".to_string(),
|
||||
name: "Alpha".to_string(),
|
||||
description: Some("Alpha v1".to_string()),
|
||||
logo_url: None,
|
||||
logo_url_dark: None,
|
||||
distribution_channel: None,
|
||||
branding: None,
|
||||
app_metadata: None,
|
||||
labels: None,
|
||||
install_url: None,
|
||||
is_accessible: false,
|
||||
is_enabled: true,
|
||||
plugin_display_names: Vec::new(),
|
||||
}];
|
||||
let (server_url, server_handle, server_control) = start_apps_server_with_delays_and_control(
|
||||
initial_connectors,
|
||||
Vec::new(),
|
||||
Duration::ZERO,
|
||||
Duration::ZERO,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let codex_home = TempDir::new()?;
|
||||
write_connectors_config(codex_home.path(), &server_url)?;
|
||||
write_chatgpt_auth(
|
||||
codex_home.path(),
|
||||
ChatGptAuthFixture::new("chatgpt-token")
|
||||
.account_id("account-123")
|
||||
.chatgpt_user_id("user-enable-refresh")
|
||||
.chatgpt_account_id("account-123"),
|
||||
AuthCredentialsStoreMode::File,
|
||||
)?;
|
||||
|
||||
let mut mcp = McpProcess::new(codex_home.path()).await?;
|
||||
timeout(DEFAULT_TIMEOUT, mcp.initialize()).await??;
|
||||
|
||||
let disable_request = mcp
|
||||
.send_experimental_feature_enablement_set_request(ExperimentalFeatureEnablementSetParams {
|
||||
enablement: BTreeMap::from([("apps".to_string(), false)]),
|
||||
})
|
||||
.await?;
|
||||
let _disable_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(disable_request)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
server_control.set_connectors(vec![AppInfo {
|
||||
id: "alpha".to_string(),
|
||||
name: "Alpha".to_string(),
|
||||
description: Some("Alpha v2".to_string()),
|
||||
logo_url: None,
|
||||
logo_url_dark: None,
|
||||
distribution_channel: None,
|
||||
branding: None,
|
||||
app_metadata: None,
|
||||
labels: None,
|
||||
install_url: None,
|
||||
is_accessible: false,
|
||||
is_enabled: true,
|
||||
plugin_display_names: Vec::new(),
|
||||
}]);
|
||||
server_control.set_tools(vec![connector_tool("alpha", "Alpha App")?]);
|
||||
|
||||
let enable_request = mcp
|
||||
.send_experimental_feature_enablement_set_request(ExperimentalFeatureEnablementSetParams {
|
||||
enablement: BTreeMap::from([("apps".to_string(), true)]),
|
||||
})
|
||||
.await?;
|
||||
let _enable_response: JSONRPCResponse = timeout(
|
||||
DEFAULT_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(enable_request)),
|
||||
)
|
||||
.await??;
|
||||
|
||||
let update = read_app_list_updated_notification(&mut mcp).await?;
|
||||
assert_eq!(
|
||||
update.data,
|
||||
vec![AppInfo {
|
||||
id: "alpha".to_string(),
|
||||
name: "Alpha".to_string(),
|
||||
description: Some("Alpha v2".to_string()),
|
||||
logo_url: None,
|
||||
logo_url_dark: None,
|
||||
distribution_channel: None,
|
||||
branding: None,
|
||||
app_metadata: None,
|
||||
labels: None,
|
||||
install_url: Some("https://chatgpt.com/apps/alpha/alpha".to_string()),
|
||||
is_accessible: true,
|
||||
is_enabled: true,
|
||||
plugin_display_names: Vec::new(),
|
||||
}]
|
||||
);
|
||||
|
||||
server_handle.abort();
|
||||
Ok(())
|
||||
}
|
||||
|
||||
async fn read_app_list_updated_notification(
|
||||
mcp: &mut McpProcess,
|
||||
) -> Result<AppListUpdatedNotification> {
|
||||
|
||||
@@ -306,6 +306,7 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
|
||||
servers.insert(name.clone(), new_entry);
|
||||
|
||||
@@ -745,6 +745,21 @@
|
||||
}
|
||||
]
|
||||
},
|
||||
"McpServerToolConfig": {
|
||||
"additionalProperties": false,
|
||||
"description": "Per-tool approval settings for a single MCP server tool.",
|
||||
"properties": {
|
||||
"approval_mode": {
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/AppToolApproval"
|
||||
}
|
||||
],
|
||||
"description": "Approval mode for this tool."
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"MemoriesToml": {
|
||||
"additionalProperties": false,
|
||||
"description": "Memories settings loaded from config.toml.",
|
||||
@@ -1246,7 +1261,9 @@
|
||||
"type": "object"
|
||||
},
|
||||
"RawMcpServerConfig": {
|
||||
"additionalProperties": false,
|
||||
"additionalProperties": {
|
||||
"$ref": "#/definitions/McpServerToolConfig"
|
||||
},
|
||||
"properties": {
|
||||
"args": {
|
||||
"default": null,
|
||||
@@ -1313,6 +1330,11 @@
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"name": {
|
||||
"default": null,
|
||||
"description": "Legacy display-name field accepted for backward compatibility.",
|
||||
"type": "string"
|
||||
},
|
||||
"oauth_resource": {
|
||||
"default": null,
|
||||
"type": "string"
|
||||
@@ -1344,6 +1366,13 @@
|
||||
"format": "double",
|
||||
"type": "number"
|
||||
},
|
||||
"tools": {
|
||||
"additionalProperties": {
|
||||
"$ref": "#/definitions/McpServerToolConfig"
|
||||
},
|
||||
"default": null,
|
||||
"type": "object"
|
||||
},
|
||||
"url": {
|
||||
"type": "string"
|
||||
}
|
||||
|
||||
@@ -1,10 +1,12 @@
|
||||
use crate::config::edit::ConfigEdit;
|
||||
use crate::config::edit::ConfigEditsBuilder;
|
||||
use crate::config::edit::apply_blocking;
|
||||
use crate::config::types::AppToolApproval;
|
||||
use crate::config::types::ApprovalsReviewer;
|
||||
use crate::config::types::BundledSkillsConfig;
|
||||
use crate::config::types::FeedbackConfigToml;
|
||||
use crate::config::types::HistoryPersistence;
|
||||
use crate::config::types::McpServerToolConfig;
|
||||
use crate::config::types::McpServerTransportConfig;
|
||||
use crate::config::types::MemoriesConfig;
|
||||
use crate::config::types::MemoriesToml;
|
||||
@@ -57,6 +59,7 @@ fn stdio_mcp(command: &str) -> McpServerConfig {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -77,6 +80,7 @@ fn http_mcp(url: &str) -> McpServerConfig {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}
|
||||
}
|
||||
|
||||
@@ -1853,6 +1857,7 @@ async fn replace_mcp_servers_round_trips_entries() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -1958,6 +1963,85 @@ startup_timeout_ms = 2500
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_servers_toml_parses_per_tool_approval_overrides() {
|
||||
let config = toml::from_str::<ConfigToml>(
|
||||
r#"
|
||||
[mcp_servers.docs]
|
||||
command = "docs-server"
|
||||
name = "Docs"
|
||||
|
||||
[mcp_servers.docs.tools.search]
|
||||
approval_mode = "approve"
|
||||
"#,
|
||||
)
|
||||
.expect("TOML deserialization should succeed");
|
||||
let tool = config
|
||||
.mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.tools.get("search"))
|
||||
.expect("docs/search tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_servers_toml_parses_legacy_flattened_per_tool_approval_overrides() {
|
||||
let config = toml::from_str::<ConfigToml>(
|
||||
r#"
|
||||
[mcp_servers.docs]
|
||||
command = "docs-server"
|
||||
|
||||
[mcp_servers.docs.search]
|
||||
approval_mode = "approve"
|
||||
"#,
|
||||
)
|
||||
.expect("legacy TOML deserialization should succeed");
|
||||
let tool = config
|
||||
.mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.tools.get("search"))
|
||||
.expect("docs/search tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn mcp_servers_toml_parses_tool_approval_override_for_reserved_name() {
|
||||
let config = toml::from_str::<ConfigToml>(
|
||||
r#"
|
||||
[mcp_servers.docs]
|
||||
command = "docs-server"
|
||||
|
||||
[mcp_servers.docs.tools.command]
|
||||
approval_mode = "approve"
|
||||
"#,
|
||||
)
|
||||
.expect("TOML deserialization should succeed");
|
||||
let tool = config
|
||||
.mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.tools.get("command"))
|
||||
.expect("docs/command tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn load_global_mcp_servers_rejects_inline_bearer_token() -> anyhow::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
@@ -2009,6 +2093,7 @@ async fn replace_mcp_servers_serializes_env_sorted() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2081,6 +2166,7 @@ async fn replace_mcp_servers_serializes_env_vars() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2133,6 +2219,7 @@ async fn replace_mcp_servers_serializes_cwd() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2183,6 +2270,7 @@ async fn replace_mcp_servers_streamable_http_serializes_bearer_token() -> anyhow
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2249,6 +2337,7 @@ async fn replace_mcp_servers_streamable_http_serializes_custom_headers() -> anyh
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
apply_blocking(
|
||||
@@ -2327,6 +2416,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2358,6 +2448,7 @@ async fn replace_mcp_servers_streamable_http_removes_optional_sections() -> anyh
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
apply_blocking(
|
||||
@@ -2424,6 +2515,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers()
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
),
|
||||
(
|
||||
@@ -2445,6 +2537,7 @@ async fn replace_mcp_servers_streamable_http_isolates_headers_between_servers()
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
),
|
||||
]);
|
||||
@@ -2529,6 +2622,7 @@ async fn replace_mcp_servers_serializes_disabled_flag() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2575,6 +2669,7 @@ async fn replace_mcp_servers_serializes_required_flag() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2621,6 +2716,7 @@ async fn replace_mcp_servers_serializes_tool_filters() -> anyhow::Result<()> {
|
||||
disabled_tools: Some(vec!["blocked".to_string()]),
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -2671,6 +2767,7 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: Some("https://resource.example.com".to_string()),
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
|
||||
@@ -125,7 +125,9 @@ pub fn model_availability_nux_count_edits(shown_count: &HashMap<String, u32>) ->
|
||||
|
||||
// TODO(jif) move to a dedicated file
|
||||
mod document_helpers {
|
||||
use crate::config::types::AppToolApproval;
|
||||
use crate::config::types::McpServerConfig;
|
||||
use crate::config::types::McpServerToolConfig;
|
||||
use crate::config::types::McpServerTransportConfig;
|
||||
use toml_edit::Array as TomlArray;
|
||||
use toml_edit::InlineTable;
|
||||
@@ -248,10 +250,32 @@ mod document_helpers {
|
||||
{
|
||||
entry["oauth_resource"] = value(resource.clone());
|
||||
}
|
||||
if !config.tools.is_empty() {
|
||||
let mut tools = new_implicit_table();
|
||||
let mut tool_entries: Vec<_> = config.tools.iter().collect();
|
||||
tool_entries.sort_by(|(left, _), (right, _)| left.cmp(right));
|
||||
for (name, tool_config) in tool_entries {
|
||||
tools.insert(name, serialize_mcp_server_tool(tool_config));
|
||||
}
|
||||
entry.insert("tools", TomlItem::Table(tools));
|
||||
}
|
||||
|
||||
entry
|
||||
}
|
||||
|
||||
fn serialize_mcp_server_tool(config: &McpServerToolConfig) -> TomlItem {
|
||||
let mut entry = TomlTable::new();
|
||||
entry.set_implicit(false);
|
||||
if let Some(approval_mode) = config.approval_mode {
|
||||
entry["approval_mode"] = value(match approval_mode {
|
||||
AppToolApproval::Auto => "auto",
|
||||
AppToolApproval::Prompt => "prompt",
|
||||
AppToolApproval::Approve => "approve",
|
||||
});
|
||||
}
|
||||
TomlItem::Table(entry)
|
||||
}
|
||||
|
||||
pub(super) fn serialize_mcp_server(config: &McpServerConfig) -> TomlItem {
|
||||
TomlItem::Table(serialize_mcp_server_table(config))
|
||||
}
|
||||
|
||||
@@ -1,4 +1,6 @@
|
||||
use super::*;
|
||||
use crate::config::types::AppToolApproval;
|
||||
use crate::config::types::McpServerToolConfig;
|
||||
use crate::config::types::McpServerTransportConfig;
|
||||
use codex_protocol::openai_models::ReasoningEffort;
|
||||
use pretty_assertions::assert_eq;
|
||||
@@ -582,6 +584,7 @@ fn blocking_replace_mcp_servers_round_trips() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -607,6 +610,7 @@ fn blocking_replace_mcp_servers_round_trips() {
|
||||
disabled_tools: Some(vec!["forbidden".to_string()]),
|
||||
scopes: None,
|
||||
oauth_resource: Some("https://resource.example.com".to_string()),
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -643,6 +647,53 @@ B = \"2\"
|
||||
assert_eq!(raw, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn blocking_replace_mcp_servers_serializes_tool_approval_overrides() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
let codex_home = tmp.path();
|
||||
|
||||
let mut servers = BTreeMap::new();
|
||||
servers.insert(
|
||||
"docs".to_string(),
|
||||
McpServerConfig {
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: "docs-server".to_string(),
|
||||
args: Vec::new(),
|
||||
env: None,
|
||||
env_vars: Vec::new(),
|
||||
cwd: None,
|
||||
},
|
||||
enabled: true,
|
||||
required: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: None,
|
||||
tool_timeout_sec: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::from([(
|
||||
"search".to_string(),
|
||||
McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
},
|
||||
)]),
|
||||
},
|
||||
);
|
||||
|
||||
apply_blocking(codex_home, None, &[ConfigEdit::ReplaceMcpServers(servers)]).expect("persist");
|
||||
|
||||
let raw = std::fs::read_to_string(codex_home.join(CONFIG_TOML_FILE)).expect("read config");
|
||||
let expected = "\
|
||||
[mcp_servers.docs]
|
||||
command = \"docs-server\"
|
||||
|
||||
[mcp_servers.docs.tools.search]
|
||||
approval_mode = \"approve\"
|
||||
";
|
||||
assert_eq!(raw, expected);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn blocking_replace_mcp_servers_preserves_inline_comments() {
|
||||
let tmp = tempdir().expect("tmpdir");
|
||||
@@ -676,6 +727,7 @@ foo = { command = "cmd" }
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -721,6 +773,7 @@ foo = { command = "cmd" } # keep me
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -765,6 +818,7 @@ foo = { command = "cmd", args = ["--flag"] } # keep me
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
@@ -810,6 +864,7 @@ foo = { command = "cmd" }
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
|
||||
|
||||
@@ -108,6 +108,10 @@ pub struct McpServerConfig {
|
||||
/// Optional OAuth resource parameter to include during MCP login (RFC 8707).
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub oauth_resource: Option<String>,
|
||||
|
||||
/// Per-tool approval settings keyed by tool name.
|
||||
#[serde(default, skip_serializing_if = "HashMap::is_empty")]
|
||||
pub tools: HashMap<String, McpServerToolConfig>,
|
||||
}
|
||||
|
||||
// Raw MCP config shape used for deserialization and JSON Schema generation.
|
||||
@@ -154,6 +158,15 @@ pub(crate) struct RawMcpServerConfig {
|
||||
pub scopes: Option<Vec<String>>,
|
||||
#[serde(default)]
|
||||
pub oauth_resource: Option<String>,
|
||||
/// Legacy display-name field accepted for backward compatibility.
|
||||
#[serde(default, rename = "name")]
|
||||
pub _name: Option<String>,
|
||||
#[serde(default)]
|
||||
pub tools: Option<HashMap<String, McpServerToolConfig>>,
|
||||
/// Legacy flattened per-tool approval settings accepted for backward compatibility.
|
||||
#[serde(default)]
|
||||
#[serde(flatten)]
|
||||
pub legacy_tools: HashMap<String, McpServerToolConfig>,
|
||||
}
|
||||
|
||||
impl<'de> Deserialize<'de> for McpServerConfig {
|
||||
@@ -178,6 +191,10 @@ impl<'de> Deserialize<'de> for McpServerConfig {
|
||||
let disabled_tools = raw.disabled_tools.clone();
|
||||
let scopes = raw.scopes.clone();
|
||||
let oauth_resource = raw.oauth_resource.clone();
|
||||
let mut tools = raw.legacy_tools.clone();
|
||||
if let Some(nested_tools) = raw.tools.clone() {
|
||||
tools.extend(nested_tools);
|
||||
}
|
||||
|
||||
fn throw_if_set<E, T>(transport: &str, field: &str, value: Option<&T>) -> Result<(), E>
|
||||
where
|
||||
@@ -236,6 +253,7 @@ impl<'de> Deserialize<'de> for McpServerConfig {
|
||||
disabled_tools,
|
||||
scopes,
|
||||
oauth_resource,
|
||||
tools,
|
||||
})
|
||||
}
|
||||
}
|
||||
@@ -496,6 +514,15 @@ pub enum AppToolApproval {
|
||||
Approve,
|
||||
}
|
||||
|
||||
/// Per-tool approval settings for a single MCP server tool.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct McpServerToolConfig {
|
||||
/// Approval mode for this tool.
|
||||
#[serde(default, skip_serializing_if = "Option::is_none")]
|
||||
pub approval_mode: Option<AppToolApproval>,
|
||||
}
|
||||
|
||||
/// Default settings that apply to all apps.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Default, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
|
||||
@@ -176,6 +176,7 @@ fn codex_apps_mcp_server_config(config: &Config, auth: Option<&CodexAuth>) -> Mc
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -235,6 +235,7 @@ async fn effective_mcp_servers_include_plugins_without_overriding_user_config()
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
|
||||
@@ -428,6 +428,7 @@ fn mcp_dependency_to_server_config(
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
});
|
||||
}
|
||||
|
||||
@@ -453,6 +454,7 @@ fn mcp_dependency_to_server_config(
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
});
|
||||
}
|
||||
|
||||
|
||||
@@ -48,6 +48,7 @@ fn collect_missing_respects_canonical_installed_key() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
@@ -97,6 +98,7 @@ fn collect_missing_dedupes_by_canonical_key_but_preserves_original_name() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
|
||||
|
||||
@@ -542,6 +542,7 @@ fn mcp_init_error_display_prompts_for_github_pat() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
auth_status: McpAuthStatus::Unsupported,
|
||||
};
|
||||
@@ -590,6 +591,7 @@ fn mcp_init_error_display_reports_generic_errors() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
auth_status: McpAuthStatus::Unsupported,
|
||||
};
|
||||
|
||||
@@ -1,7 +1,10 @@
|
||||
use std::collections::BTreeMap;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
use codex_app_server_protocol::ConfigLayerSource;
|
||||
use codex_app_server_protocol::McpElicitationObjectType;
|
||||
use codex_app_server_protocol::McpElicitationSchema;
|
||||
use codex_app_server_protocol::McpServerElicitationRequest;
|
||||
@@ -12,8 +15,10 @@ use crate::arc_monitor::ArcMonitorOutcome;
|
||||
use crate::arc_monitor::monitor_action;
|
||||
use crate::codex::Session;
|
||||
use crate::codex::TurnContext;
|
||||
use crate::config::Config;
|
||||
use crate::config::edit::ConfigEdit;
|
||||
use crate::config::edit::ConfigEditsBuilder;
|
||||
use crate::config::load_global_mcp_servers;
|
||||
use crate::config::types::AppToolApproval;
|
||||
use crate::connectors;
|
||||
use crate::guardian::GuardianApprovalRequest;
|
||||
@@ -46,6 +51,7 @@ use codex_protocol::request_user_input::RequestUserInputResponse;
|
||||
use codex_rmcp_client::ElicitationAction;
|
||||
use codex_rmcp_client::ElicitationResponse;
|
||||
use rmcp::model::ToolAnnotations;
|
||||
use serde::Deserialize;
|
||||
use serde::Serialize;
|
||||
use std::path::Path;
|
||||
use std::sync::Arc;
|
||||
@@ -104,6 +110,11 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
} else {
|
||||
connectors::AppToolPolicy::default()
|
||||
};
|
||||
let approval_mode = if server == CODEX_APPS_MCP_SERVER_NAME {
|
||||
app_tool_policy.approval
|
||||
} else {
|
||||
custom_mcp_tool_approval_mode(turn_context.as_ref(), &server, &tool_name)
|
||||
};
|
||||
|
||||
if server == CODEX_APPS_MCP_SERVER_NAME && !app_tool_policy.enabled {
|
||||
let result = notify_mcp_tool_call_skip(
|
||||
@@ -151,7 +162,7 @@ pub(crate) async fn handle_mcp_tool_call(
|
||||
&call_id,
|
||||
&invocation,
|
||||
metadata.as_ref(),
|
||||
app_tool_policy.approval,
|
||||
approval_mode,
|
||||
)
|
||||
.await
|
||||
{
|
||||
@@ -491,6 +502,27 @@ pub(crate) struct McpToolApprovalMetadata {
|
||||
|
||||
const MCP_TOOL_CODEX_APPS_META_KEY: &str = "_codex_apps";
|
||||
|
||||
fn custom_mcp_tool_approval_mode(
|
||||
turn_context: &TurnContext,
|
||||
server: &str,
|
||||
tool_name: &str,
|
||||
) -> AppToolApproval {
|
||||
turn_context
|
||||
.config
|
||||
.config_layer_stack
|
||||
.effective_config()
|
||||
.as_table()
|
||||
.and_then(|table| table.get("mcp_servers"))
|
||||
.cloned()
|
||||
.and_then(|value| {
|
||||
HashMap::<String, crate::config::types::McpServerConfig>::deserialize(value).ok()
|
||||
})
|
||||
.and_then(|servers| servers.get(server).cloned())
|
||||
.and_then(|server| server.tools.get(tool_name).cloned())
|
||||
.and_then(|tool| tool.approval_mode)
|
||||
.unwrap_or_default()
|
||||
}
|
||||
|
||||
fn build_mcp_tool_call_request_meta(
|
||||
turn_context: &TurnContext,
|
||||
server: &str,
|
||||
@@ -560,7 +592,6 @@ const MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: &str = "tool_params";
|
||||
const MCP_TOOL_APPROVAL_TOOL_PARAMS_DISPLAY_KEY: &str = "tool_params_display";
|
||||
const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_DEFAULT: &str = "mcp_tool_call__default";
|
||||
const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_ALWAYS_ALLOW: &str = "mcp_tool_call__always_allow";
|
||||
const MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_FULL_ACCESS: &str = "mcp_tool_call__full_access";
|
||||
|
||||
pub(crate) fn is_mcp_tool_approval_question_id(question_id: &str) -> bool {
|
||||
question_id
|
||||
@@ -595,11 +626,14 @@ async fn maybe_request_mcp_tool_approval(
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> Option<McpToolApprovalDecision> {
|
||||
if is_full_access_mode(turn_context) {
|
||||
return None;
|
||||
}
|
||||
|
||||
let annotations = metadata.and_then(|metadata| metadata.annotations.as_ref());
|
||||
let approval_required = requires_mcp_tool_approval(annotations);
|
||||
let mut monitor_reason = None;
|
||||
let auto_approved_by_policy = approval_mode == AppToolApproval::Approve
|
||||
|| (approval_mode == AppToolApproval::Auto && is_full_access_mode(turn_context));
|
||||
let auto_approved_by_policy = approval_mode == AppToolApproval::Approve;
|
||||
|
||||
if auto_approved_by_policy {
|
||||
if !approval_required {
|
||||
@@ -812,12 +846,7 @@ fn persistent_mcp_tool_approval_key(
|
||||
metadata: Option<&McpToolApprovalMetadata>,
|
||||
approval_mode: AppToolApproval,
|
||||
) -> Option<McpToolApprovalKey> {
|
||||
if invocation.server != CODEX_APPS_MCP_SERVER_NAME {
|
||||
return None;
|
||||
}
|
||||
|
||||
session_mcp_tool_approval_key(invocation, metadata, approval_mode)
|
||||
.filter(|key| key.connector_id.is_some())
|
||||
}
|
||||
|
||||
pub(crate) fn build_guardian_mcp_tool_review_request(
|
||||
@@ -865,16 +894,12 @@ fn is_full_access_mode(turn_context: &TurnContext) -> bool {
|
||||
|
||||
fn mcp_tool_approval_callsite_mode(
|
||||
approval_mode: AppToolApproval,
|
||||
turn_context: &TurnContext,
|
||||
_turn_context: &TurnContext,
|
||||
) -> &'static str {
|
||||
match approval_mode {
|
||||
AppToolApproval::Approve => MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_ALWAYS_ALLOW,
|
||||
AppToolApproval::Auto | AppToolApproval::Prompt => {
|
||||
if approval_mode == AppToolApproval::Auto && is_full_access_mode(turn_context) {
|
||||
MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_FULL_ACCESS
|
||||
} else {
|
||||
MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_DEFAULT
|
||||
}
|
||||
MCP_TOOL_CALL_ARC_MONITOR_CALLSITE_DEFAULT
|
||||
}
|
||||
}
|
||||
}
|
||||
@@ -1356,21 +1381,25 @@ async fn maybe_persist_mcp_tool_approval(
|
||||
turn_context: &TurnContext,
|
||||
key: McpToolApprovalKey,
|
||||
) {
|
||||
let Some(connector_id) = key.connector_id.clone() else {
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
return;
|
||||
};
|
||||
let tool_name = key.tool_name.clone();
|
||||
|
||||
if let Err(err) =
|
||||
let persist_result = if key.server == CODEX_APPS_MCP_SERVER_NAME {
|
||||
let Some(connector_id) = key.connector_id.clone() else {
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
return;
|
||||
};
|
||||
persist_codex_app_tool_approval(&turn_context.config.codex_home, &connector_id, &tool_name)
|
||||
.await
|
||||
{
|
||||
} else {
|
||||
persist_custom_mcp_tool_approval(&turn_context.config, &key.server, &tool_name).await
|
||||
};
|
||||
|
||||
if let Err(err) = persist_result {
|
||||
error!(
|
||||
error = %err,
|
||||
connector_id,
|
||||
server = key.server,
|
||||
tool_name,
|
||||
"failed to persist codex app tool approval"
|
||||
"failed to persist MCP tool approval"
|
||||
);
|
||||
remember_mcp_tool_approval(sess, key).await;
|
||||
return;
|
||||
@@ -1400,6 +1429,67 @@ async fn persist_codex_app_tool_approval(
|
||||
.await
|
||||
}
|
||||
|
||||
async fn persist_custom_mcp_tool_approval(
|
||||
config: &Config,
|
||||
server: &str,
|
||||
tool_name: &str,
|
||||
) -> anyhow::Result<()> {
|
||||
let config_folder = if let Some(project_config_folder) =
|
||||
project_mcp_tool_approval_config_folder(config, server)
|
||||
{
|
||||
project_config_folder
|
||||
} else {
|
||||
let servers = load_global_mcp_servers(&config.codex_home).await?;
|
||||
if !servers.contains_key(server) {
|
||||
anyhow::bail!("MCP server `{server}` is not configured in config.toml");
|
||||
}
|
||||
config.codex_home.clone()
|
||||
};
|
||||
|
||||
ConfigEditsBuilder::new(&config_folder)
|
||||
.with_edits([ConfigEdit::SetPath {
|
||||
segments: vec![
|
||||
"mcp_servers".to_string(),
|
||||
server.to_string(),
|
||||
"tools".to_string(),
|
||||
tool_name.to_string(),
|
||||
"approval_mode".to_string(),
|
||||
],
|
||||
value: value("approve"),
|
||||
}])
|
||||
.apply()
|
||||
.await
|
||||
}
|
||||
|
||||
fn project_mcp_tool_approval_config_folder(config: &Config, server: &str) -> Option<PathBuf> {
|
||||
config
|
||||
.config_layer_stack
|
||||
.layers_high_to_low()
|
||||
.into_iter()
|
||||
.find_map(|layer| {
|
||||
if !matches!(layer.name, ConfigLayerSource::Project { .. }) {
|
||||
return None;
|
||||
}
|
||||
|
||||
let servers = layer
|
||||
.config
|
||||
.as_table()
|
||||
.and_then(|table| table.get("mcp_servers"))
|
||||
.cloned()
|
||||
.and_then(|value| {
|
||||
HashMap::<String, crate::config::types::McpServerConfig>::deserialize(value)
|
||||
.ok()
|
||||
})?;
|
||||
if servers.contains_key(server) {
|
||||
layer
|
||||
.config_folder()
|
||||
.map(|folder| folder.as_path().to_path_buf())
|
||||
} else {
|
||||
None
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
fn requires_mcp_tool_approval(annotations: Option<&ToolAnnotations>) -> bool {
|
||||
let destructive_hint = annotations.and_then(|annotations| annotations.destructive_hint);
|
||||
if destructive_hint == Some(true) {
|
||||
|
||||
@@ -1,11 +1,14 @@
|
||||
use super::*;
|
||||
use crate::codex::make_session_and_context;
|
||||
use crate::config::ApprovalsReviewer;
|
||||
use crate::config::ConfigBuilder;
|
||||
use crate::config::ConfigToml;
|
||||
use crate::config::types::AppConfig;
|
||||
use crate::config::types::AppToolConfig;
|
||||
use crate::config::types::AppToolsConfig;
|
||||
use crate::config::types::AppsConfigToml;
|
||||
use crate::config::types::McpServerConfig;
|
||||
use crate::config::types::McpServerToolConfig;
|
||||
use codex_config::CONFIG_TOML_FILE;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
@@ -378,13 +381,13 @@ fn codex_apps_tool_question_without_elicitation_omits_always_allow() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_mcp_tool_question_offers_session_remember_without_always_allow() {
|
||||
fn custom_mcp_tool_question_offers_session_remember_and_always_allow() {
|
||||
let question = build_mcp_tool_approval_question(
|
||||
"q".to_string(),
|
||||
"custom_server",
|
||||
"run_action",
|
||||
None,
|
||||
prompt_options(true, false),
|
||||
prompt_options(true, true),
|
||||
None,
|
||||
);
|
||||
|
||||
@@ -398,13 +401,14 @@ fn custom_mcp_tool_question_offers_session_remember_without_always_allow() {
|
||||
vec![
|
||||
MCP_TOOL_APPROVAL_ACCEPT.to_string(),
|
||||
MCP_TOOL_APPROVAL_ACCEPT_FOR_SESSION.to_string(),
|
||||
MCP_TOOL_APPROVAL_ACCEPT_AND_REMEMBER.to_string(),
|
||||
MCP_TOOL_APPROVAL_CANCEL.to_string(),
|
||||
]
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn custom_servers_keep_session_remember_without_persistent_approval() {
|
||||
fn custom_servers_support_session_and_persistent_approval() {
|
||||
let invocation = McpInvocation {
|
||||
server: "custom_server".to_string(),
|
||||
tool: "run_action".to_string(),
|
||||
@@ -418,11 +422,11 @@ fn custom_servers_keep_session_remember_without_persistent_approval() {
|
||||
|
||||
assert_eq!(
|
||||
session_mcp_tool_approval_key(&invocation, None, AppToolApproval::Auto),
|
||||
Some(expected)
|
||||
Some(expected.clone())
|
||||
);
|
||||
assert_eq!(
|
||||
persistent_mcp_tool_approval_key(&invocation, None, AppToolApproval::Auto),
|
||||
None
|
||||
Some(expected)
|
||||
);
|
||||
}
|
||||
|
||||
@@ -612,7 +616,7 @@ fn approval_elicitation_meta_marks_tool_approvals() {
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn approval_elicitation_meta_keeps_session_persist_behavior_for_custom_servers() {
|
||||
fn approval_elicitation_meta_merges_session_and_always_persist_for_custom_servers() {
|
||||
assert_eq!(
|
||||
build_mcp_tool_approval_elicitation_meta(
|
||||
"custom_server",
|
||||
@@ -625,11 +629,14 @@ fn approval_elicitation_meta_keeps_session_persist_behavior_for_custom_servers()
|
||||
)),
|
||||
Some(&serde_json::json!({"id": 1})),
|
||||
None,
|
||||
prompt_options(true, false),
|
||||
prompt_options(true, true),
|
||||
),
|
||||
Some(serde_json::json!({
|
||||
MCP_TOOL_APPROVAL_KIND_KEY: MCP_TOOL_APPROVAL_KIND_MCP_TOOL_CALL,
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY: MCP_TOOL_APPROVAL_PERSIST_SESSION,
|
||||
MCP_TOOL_APPROVAL_PERSIST_KEY: [
|
||||
MCP_TOOL_APPROVAL_PERSIST_SESSION,
|
||||
MCP_TOOL_APPROVAL_PERSIST_ALWAYS,
|
||||
],
|
||||
MCP_TOOL_APPROVAL_TOOL_TITLE_KEY: "Run Action",
|
||||
MCP_TOOL_APPROVAL_TOOL_DESCRIPTION_KEY: "Runs the selected action.",
|
||||
MCP_TOOL_APPROVAL_TOOL_PARAMS_KEY: {
|
||||
@@ -843,8 +850,8 @@ fn approval_elicitation_meta_merges_session_and_always_persist_with_connector_so
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approval_callsite_mode_distinguishes_default_always_allow_and_full_access() {
|
||||
let (_session, mut turn_context) = make_session_and_context().await;
|
||||
async fn approval_callsite_mode_distinguishes_default_and_always_allow() {
|
||||
let (_session, turn_context) = make_session_and_context().await;
|
||||
|
||||
assert_eq!(
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Auto, &turn_context),
|
||||
@@ -858,20 +865,6 @@ async fn approval_callsite_mode_distinguishes_default_always_allow_and_full_acce
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Approve, &turn_context),
|
||||
"mcp_tool_call__always_allow"
|
||||
);
|
||||
|
||||
turn_context
|
||||
.approval_policy
|
||||
.set(AskForApproval::Never)
|
||||
.expect("test setup should allow updating approval policy");
|
||||
turn_context
|
||||
.sandbox_policy
|
||||
.set(SandboxPolicy::DangerFullAccess)
|
||||
.expect("test setup should allow updating sandbox policy");
|
||||
|
||||
assert_eq!(
|
||||
mcp_tool_approval_callsite_mode(AppToolApproval::Auto, &turn_context),
|
||||
"mcp_tool_call__full_access"
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
@@ -992,6 +985,41 @@ async fn persist_codex_app_tool_approval_writes_tool_override() {
|
||||
assert!(contents.contains("[apps.calendar.tools.\"calendar/list_events\"]"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn persist_custom_mcp_tool_approval_writes_tool_override() {
|
||||
let tmp = tempdir().expect("tempdir");
|
||||
std::fs::write(
|
||||
tmp.path().join(CONFIG_TOML_FILE),
|
||||
"[mcp_servers.docs]\ncommand = \"docs-server\"\n",
|
||||
)
|
||||
.expect("seed config");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(tmp.path().to_path_buf())
|
||||
.build()
|
||||
.await
|
||||
.expect("load config");
|
||||
|
||||
persist_custom_mcp_tool_approval(&config, "docs", "search")
|
||||
.await
|
||||
.expect("persist approval");
|
||||
|
||||
let contents = std::fs::read_to_string(tmp.path().join(CONFIG_TOML_FILE)).expect("read config");
|
||||
let parsed: ConfigToml = toml::from_str(&contents).expect("parse config");
|
||||
let tool = parsed
|
||||
.mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.tools.get("search"))
|
||||
.expect("docs/search tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
assert!(contents.contains("[mcp_servers.docs.tools.search]"));
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn maybe_persist_mcp_tool_approval_reloads_session_config() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
@@ -1031,6 +1059,104 @@ async fn maybe_persist_mcp_tool_approval_reloads_session_config() {
|
||||
assert_eq!(mcp_tool_approval_is_remembered(&session, &key).await, true);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn maybe_persist_mcp_tool_approval_reloads_session_config_for_custom_server() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
let codex_home = session.codex_home().await;
|
||||
std::fs::create_dir_all(&codex_home).expect("create codex home");
|
||||
std::fs::write(
|
||||
codex_home.join(CONFIG_TOML_FILE),
|
||||
"[mcp_servers.docs]\ncommand = \"docs-server\"\n",
|
||||
)
|
||||
.expect("seed config");
|
||||
let key = McpToolApprovalKey {
|
||||
server: "docs".to_string(),
|
||||
connector_id: None,
|
||||
tool_name: "search".to_string(),
|
||||
};
|
||||
|
||||
maybe_persist_mcp_tool_approval(&session, &turn_context, key.clone()).await;
|
||||
|
||||
let config = session.get_config().await;
|
||||
let mcp_servers_toml = config
|
||||
.config_layer_stack
|
||||
.effective_config()
|
||||
.as_table()
|
||||
.and_then(|table| table.get("mcp_servers"))
|
||||
.cloned()
|
||||
.expect("mcp_servers table");
|
||||
let mcp_servers = HashMap::<String, McpServerConfig>::deserialize(mcp_servers_toml)
|
||||
.expect("deserialize MCP servers");
|
||||
let tool = mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.tools.get("search"))
|
||||
.expect("docs/search tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
assert_eq!(mcp_tool_approval_is_remembered(&session, &key).await, true);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn maybe_persist_mcp_tool_approval_writes_project_config_for_project_server() {
|
||||
let (session, mut turn_context) = make_session_and_context().await;
|
||||
let codex_home = session.codex_home().await;
|
||||
let project_dir = tempdir().expect("tempdir");
|
||||
std::fs::write(project_dir.path().join(".git"), "gitdir: nowhere").expect("seed git marker");
|
||||
let project_codex_dir = project_dir.path().join(".codex");
|
||||
std::fs::create_dir_all(&project_codex_dir).expect("create project .codex dir");
|
||||
std::fs::write(
|
||||
project_codex_dir.join(CONFIG_TOML_FILE),
|
||||
"[mcp_servers.docs]\ncommand = \"docs-server\"\n",
|
||||
)
|
||||
.expect("seed project config");
|
||||
ConfigEditsBuilder::new(&codex_home)
|
||||
.set_project_trust_level(
|
||||
project_dir.path(),
|
||||
codex_protocol::config_types::TrustLevel::Trusted,
|
||||
)
|
||||
.apply()
|
||||
.await
|
||||
.expect("trust project");
|
||||
let config = ConfigBuilder::default()
|
||||
.codex_home(codex_home)
|
||||
.fallback_cwd(Some(project_dir.path().to_path_buf()))
|
||||
.build()
|
||||
.await
|
||||
.expect("load project config");
|
||||
turn_context.cwd = config.cwd.clone();
|
||||
turn_context.config = Arc::new(config);
|
||||
let key = McpToolApprovalKey {
|
||||
server: "docs".to_string(),
|
||||
connector_id: None,
|
||||
tool_name: "search".to_string(),
|
||||
};
|
||||
|
||||
maybe_persist_mcp_tool_approval(&session, &turn_context, key.clone()).await;
|
||||
|
||||
let contents = std::fs::read_to_string(project_codex_dir.join(CONFIG_TOML_FILE))
|
||||
.expect("read project config");
|
||||
let parsed: ConfigToml = toml::from_str(&contents).expect("parse project config");
|
||||
let tool = parsed
|
||||
.mcp_servers
|
||||
.get("docs")
|
||||
.and_then(|server| server.tools.get("search"))
|
||||
.expect("docs/search tool config exists");
|
||||
|
||||
assert_eq!(
|
||||
tool,
|
||||
&McpServerToolConfig {
|
||||
approval_mode: Some(AppToolApproval::Approve),
|
||||
}
|
||||
);
|
||||
assert!(contents.contains("[mcp_servers.docs.tools.search]"));
|
||||
assert_eq!(mcp_tool_approval_is_remembered(&session, &key).await, true);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approve_mode_skips_when_annotations_do_not_require_approval() {
|
||||
let (session, turn_context) = make_session_and_context().await;
|
||||
@@ -1133,6 +1259,75 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_for_model() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn custom_approve_mode_blocks_when_arc_returns_interrupt_for_model() {
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
let server = MockServer::start().await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/codex/safety/arc"))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(serde_json::json!({
|
||||
"outcome": "steer-model",
|
||||
"short_reason": "needs approval",
|
||||
"rationale": "high-risk action",
|
||||
"risk_score": 96,
|
||||
"risk_level": "critical",
|
||||
"evidence": [{
|
||||
"message": "dangerous_tool",
|
||||
"why": "high-risk action",
|
||||
}],
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let (session, mut turn_context) = make_session_and_context().await;
|
||||
turn_context.auth_manager = Some(crate::test_support::auth_manager_from_auth(
|
||||
crate::CodexAuth::create_dummy_chatgpt_auth_for_testing(),
|
||||
));
|
||||
let mut config = (*turn_context.config).clone();
|
||||
config.chatgpt_base_url = server.uri();
|
||||
turn_context.config = Arc::new(config);
|
||||
|
||||
let session = Arc::new(session);
|
||||
let turn_context = Arc::new(turn_context);
|
||||
let invocation = McpInvocation {
|
||||
server: "docs".to_string(),
|
||||
tool: "dangerous_tool".to_string(),
|
||||
arguments: Some(serde_json::json!({ "id": 1 })),
|
||||
};
|
||||
let metadata = McpToolApprovalMetadata {
|
||||
annotations: Some(annotations(Some(false), Some(true), Some(true))),
|
||||
connector_id: None,
|
||||
connector_name: None,
|
||||
connector_description: None,
|
||||
tool_title: Some("Dangerous Tool".to_string()),
|
||||
tool_description: Some("Performs a risky action.".to_string()),
|
||||
codex_apps_meta: None,
|
||||
};
|
||||
|
||||
let decision = maybe_request_mcp_tool_approval(
|
||||
&session,
|
||||
&turn_context,
|
||||
"call-2-custom",
|
||||
&invocation,
|
||||
Some(&metadata),
|
||||
AppToolApproval::Approve,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
decision,
|
||||
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
|
||||
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
|
||||
))
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
|
||||
use wiremock::Mock;
|
||||
@@ -1203,7 +1398,7 @@ async fn approve_mode_blocks_when_arc_returns_interrupt_without_annotations() {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn full_access_auto_mode_blocks_when_arc_returns_interrupt_for_model() {
|
||||
async fn full_access_mode_skips_arc_monitor_for_all_approval_modes() {
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
@@ -1224,7 +1419,7 @@ async fn full_access_auto_mode_blocks_when_arc_returns_interrupt_for_model() {
|
||||
"why": "high-risk action",
|
||||
}],
|
||||
})))
|
||||
.expect(1)
|
||||
.expect(0)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
@@ -1261,22 +1456,23 @@ async fn full_access_auto_mode_blocks_when_arc_returns_interrupt_for_model() {
|
||||
codex_apps_meta: None,
|
||||
};
|
||||
|
||||
let decision = maybe_request_mcp_tool_approval(
|
||||
&session,
|
||||
&turn_context,
|
||||
"call-2",
|
||||
&invocation,
|
||||
Some(&metadata),
|
||||
for approval_mode in [
|
||||
AppToolApproval::Auto,
|
||||
)
|
||||
.await;
|
||||
AppToolApproval::Prompt,
|
||||
AppToolApproval::Approve,
|
||||
] {
|
||||
let decision = maybe_request_mcp_tool_approval(
|
||||
&session,
|
||||
&turn_context,
|
||||
"call-2",
|
||||
&invocation,
|
||||
Some(&metadata),
|
||||
approval_mode,
|
||||
)
|
||||
.await;
|
||||
|
||||
assert_eq!(
|
||||
decision,
|
||||
Some(McpToolApprovalDecision::BlockedBySafetyMonitor(
|
||||
"Tool call was cancelled because of safety risks: high-risk action".to_string(),
|
||||
))
|
||||
);
|
||||
assert_eq!(decision, None);
|
||||
}
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -164,6 +164,7 @@ fn load_plugins_loads_default_skills_and_mcp_servers() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]),
|
||||
apps: vec![AppConnectorId("connector_example".to_string())],
|
||||
@@ -483,6 +484,7 @@ fn load_plugins_uses_manifest_configured_component_paths() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)])
|
||||
);
|
||||
@@ -586,6 +588,7 @@ fn load_plugins_ignores_manifest_component_paths_without_dot_slash() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)])
|
||||
);
|
||||
@@ -735,6 +738,7 @@ fn capability_index_filters_inactive_and_zero_capability_plugins() {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
let plugin = |config_name: &str, dir_name: &str, manifest_name: &str| LoadedPlugin {
|
||||
config_name: config_name.to_string(),
|
||||
|
||||
@@ -207,6 +207,7 @@ async fn run_code_mode_turn_with_rmcp(
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
|
||||
@@ -108,6 +108,7 @@ async fn stdio_server_round_trip() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -253,6 +254,7 @@ async fn stdio_image_responses_round_trip() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -476,6 +478,7 @@ async fn stdio_image_responses_are_sanitized_for_text_only_model() -> anyhow::Re
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -597,6 +600,7 @@ async fn stdio_server_propagates_whitelisted_env_vars() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -759,6 +763,7 @@ async fn streamable_http_tool_call_round_trip() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -981,6 +986,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
|
||||
@@ -375,6 +375,7 @@ async fn mcp_call_marks_thread_memory_mode_polluted_when_configured() -> Result<
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
|
||||
@@ -371,6 +371,7 @@ async fn mcp_tool_call_output_exceeds_limit_truncated_for_model() -> Result<()>
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -466,6 +467,7 @@ async fn mcp_image_output_preserves_image_and_no_text_summary() -> Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
@@ -734,6 +736,7 @@ async fn mcp_tool_call_output_not_truncated_with_custom_limit() -> Result<()> {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
);
|
||||
config
|
||||
|
||||
@@ -2914,6 +2914,7 @@ mod tests {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
let mut servers = config.mcp_servers.get().clone();
|
||||
servers.insert("docs".to_string(), stdio_config);
|
||||
@@ -2938,6 +2939,7 @@ mod tests {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
servers.insert("http".to_string(), http_config);
|
||||
config
|
||||
|
||||
@@ -3143,6 +3143,7 @@ mod tests {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
let mut servers = config.mcp_servers.get().clone();
|
||||
servers.insert("docs".to_string(), stdio_config);
|
||||
@@ -3167,6 +3168,7 @@ mod tests {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
};
|
||||
servers.insert("http".to_string(), http_config);
|
||||
config
|
||||
@@ -3237,6 +3239,7 @@ mod tests {
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]);
|
||||
config
|
||||
|
||||
@@ -12,6 +12,16 @@ Codex can connect to MCP servers configured in `~/.codex/config.toml`. See the c
|
||||
|
||||
- https://developers.openai.com/codex/config-reference
|
||||
|
||||
## MCP tool approvals
|
||||
|
||||
Codex stores per-tool approval overrides for custom MCP servers under
|
||||
`mcp_servers` in `~/.codex/config.toml`:
|
||||
|
||||
```toml
|
||||
[mcp_servers.docs.tools.search]
|
||||
approval_mode = "approve"
|
||||
```
|
||||
|
||||
## Apps (Connectors)
|
||||
|
||||
Use `$` in the composer to insert a ChatGPT connector; the popover lists accessible
|
||||
|
||||
Reference in New Issue
Block a user