mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
Add config toggles for orchestrator skills and MCP (#28942)
## Why Orchestrator-provided skills and Codex Apps MCP tools add model-visible instructions, resources, and tools beyond the local workspace. Hosts need config-level switches to disable those orchestrator-owned surfaces independently, without disabling regular skills or regular MCP servers. ## What changed - Adds `[orchestrator.skills].enabled` and `[orchestrator.mcp].enabled` config entries, both defaulting to `true`. - Includes the new settings in `config.schema.json` and in the config lock so resolved thread configuration preserves the same orchestrator exposure decisions. - Threads `orchestrator.skills.enabled` through the app-server skills extension so disabled orchestrator skills do not expose the `skills` namespace or inject orchestrator skill context. - Gates Codex Apps MCP exposure, app instructions, and app auth eligibility on `orchestrator.mcp.enabled` while leaving non-Codex-Apps MCP tools available. - Updates the thread-manager sample config to disable both orchestrator-owned surfaces. ## Verification - Added config parsing, loading, defaulting, and schema coverage for the new settings. - Added MCP exposure coverage that `orchestrator.mcp.enabled = false` removes Codex Apps tools while preserving regular MCP tools. - Added app-server coverage that `orchestrator.skills.enabled = false` prevents orchestrator skill tools, prompts, and resource reads from reaching the model turn.
This commit is contained in:
@@ -88,6 +88,7 @@ where
|
||||
|config: &Config| codex_skills_extension::SkillsExtensionConfig {
|
||||
include_instructions: config.include_skill_instructions,
|
||||
bundled_skills_enabled: config.bundled_skills_enabled(),
|
||||
orchestrator_skills_enabled: config.orchestrator_skills_enabled,
|
||||
},
|
||||
);
|
||||
Arc::new(builder.build())
|
||||
|
||||
@@ -436,6 +436,95 @@ async fn local_executor_does_not_expose_orchestrator_skills() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn disabled_orchestrator_skills_do_not_expose_skills_namespace() -> Result<()> {
|
||||
let responses_server = responses::start_mock_server().await;
|
||||
let (apps_server_url, apps_server_calls, apps_server_handle) =
|
||||
start_resource_apps_mcp_server().await?;
|
||||
let responses_server_uri = responses_server.uri();
|
||||
let (_codex_home, mut mcp) = start_resource_test_app_server_with_extra_config(
|
||||
&apps_server_url,
|
||||
&responses_server_uri,
|
||||
r#"
|
||||
[orchestrator.skills]
|
||||
enabled = false
|
||||
"#,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let thread_start_id = mcp
|
||||
.send_thread_start_request(ThreadStartParams {
|
||||
model: Some("mock-model".to_string()),
|
||||
environments: Some(Vec::new()),
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
let thread_start_resp: JSONRPCResponse = timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(thread_start_id)),
|
||||
)
|
||||
.await??;
|
||||
let ThreadStartResponse { thread, .. } = to_response(thread_start_resp)?;
|
||||
|
||||
let response_mock = responses::mount_sse_once(
|
||||
&responses_server,
|
||||
responses::sse(vec![
|
||||
responses::ev_response_created("resp-disabled-orchestrator-skills"),
|
||||
responses::ev_assistant_message("msg-disabled-orchestrator-skills", "Done"),
|
||||
responses::ev_completed("resp-disabled-orchestrator-skills"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
let turn_start_id = mcp
|
||||
.send_turn_start_request(TurnStartParams {
|
||||
thread_id: thread.id,
|
||||
input: vec![UserInput::Text {
|
||||
text: format!("Use ${SKILL_NAME}"),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
..Default::default()
|
||||
})
|
||||
.await?;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_response_message(RequestId::Integer(turn_start_id)),
|
||||
)
|
||||
.await??;
|
||||
timeout(
|
||||
DEFAULT_READ_TIMEOUT,
|
||||
mcp.read_stream_until_notification_message("turn/completed"),
|
||||
)
|
||||
.await??;
|
||||
|
||||
let request = response_mock.single_request();
|
||||
assert!(request.tool_by_name("skills", "list").is_none());
|
||||
assert!(request.tool_by_name("skills", "read").is_none());
|
||||
assert!(
|
||||
request
|
||||
.message_input_texts("developer")
|
||||
.iter()
|
||||
.all(|text| !text.contains(SKILL_NAME))
|
||||
);
|
||||
assert!(
|
||||
request
|
||||
.message_input_texts("user")
|
||||
.iter()
|
||||
.all(|text| !text.contains(SKILL_MARKER))
|
||||
);
|
||||
assert_eq!(
|
||||
ResourceAppsMcpCallCounts {
|
||||
list_resources: 0,
|
||||
main_prompt_reads: 0,
|
||||
reference_reads: 0,
|
||||
},
|
||||
apps_server_calls.snapshot()
|
||||
);
|
||||
|
||||
apps_server_handle.abort();
|
||||
let _ = apps_server_handle.await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn mcp_resource_read_returns_resource_contents_without_thread() -> Result<()> {
|
||||
let (apps_server_url, _apps_server_calls, apps_server_handle) =
|
||||
@@ -555,6 +644,15 @@ async fn mcp_resource_read_returns_error_for_unknown_thread() -> Result<()> {
|
||||
async fn start_resource_test_app_server(
|
||||
apps_server_url: &str,
|
||||
responses_server_uri: &str,
|
||||
) -> Result<(TempDir, TestAppServer)> {
|
||||
start_resource_test_app_server_with_extra_config(apps_server_url, responses_server_uri, "")
|
||||
.await
|
||||
}
|
||||
|
||||
async fn start_resource_test_app_server_with_extra_config(
|
||||
apps_server_url: &str,
|
||||
responses_server_uri: &str,
|
||||
extra_config: &str,
|
||||
) -> Result<(TempDir, TestAppServer)> {
|
||||
let codex_home = TempDir::new()?;
|
||||
std::fs::write(
|
||||
@@ -574,6 +672,7 @@ apps = true
|
||||
|
||||
[skills]
|
||||
include_instructions = true
|
||||
{extra_config}
|
||||
|
||||
[model_providers.mock_provider]
|
||||
name = "Mock provider for test"
|
||||
|
||||
@@ -547,14 +547,20 @@ impl McpConnectionManager {
|
||||
))
|
||||
}
|
||||
|
||||
/// Returns a single map that contains all resources. Each key is the
|
||||
/// server name and the value is a vector of resources.
|
||||
pub async fn list_all_resources(&self) -> HashMap<String, Vec<Resource>> {
|
||||
/// Returns resources from servers selected by `include_server`. Each key
|
||||
/// is the server name and the value is a vector of resources.
|
||||
pub async fn list_all_resources(
|
||||
&self,
|
||||
include_server: impl Fn(&str) -> bool,
|
||||
) -> HashMap<String, Vec<Resource>> {
|
||||
let mut join_set = JoinSet::new();
|
||||
|
||||
let clients_snapshot = &self.clients;
|
||||
|
||||
for (server_name, async_managed_client) in clients_snapshot {
|
||||
for (server_name, async_managed_client) in clients_snapshot
|
||||
.iter()
|
||||
.filter(|(server_name, _)| include_server(server_name))
|
||||
{
|
||||
let server_name = server_name.clone();
|
||||
let Ok(managed_client) = async_managed_client.client().await else {
|
||||
continue;
|
||||
@@ -612,14 +618,20 @@ impl McpConnectionManager {
|
||||
aggregated
|
||||
}
|
||||
|
||||
/// Returns a single map that contains all resource templates. Each key is the
|
||||
/// server name and the value is a vector of resource templates.
|
||||
pub async fn list_all_resource_templates(&self) -> HashMap<String, Vec<ResourceTemplate>> {
|
||||
/// Returns resource templates from servers selected by `include_server`.
|
||||
/// Each key is the server name and the value is a vector of templates.
|
||||
pub async fn list_all_resource_templates(
|
||||
&self,
|
||||
include_server: impl Fn(&str) -> bool,
|
||||
) -> HashMap<String, Vec<ResourceTemplate>> {
|
||||
let mut join_set = JoinSet::new();
|
||||
|
||||
let clients_snapshot = &self.clients;
|
||||
|
||||
for (server_name, async_managed_client) in clients_snapshot {
|
||||
for (server_name, async_managed_client) in clients_snapshot
|
||||
.iter()
|
||||
.filter(|(server_name, _)| include_server(server_name))
|
||||
{
|
||||
let server_name_cloned = server_name.clone();
|
||||
let Ok(managed_client) = async_managed_client.client().await else {
|
||||
continue;
|
||||
|
||||
@@ -619,14 +619,16 @@ async fn collect_mcp_server_status_snapshot_from_manager(
|
||||
mcp_connection_manager.list_all_tools(),
|
||||
async {
|
||||
if detail.include_resources() {
|
||||
mcp_connection_manager.list_all_resources().await
|
||||
mcp_connection_manager.list_all_resources(|_| true).await
|
||||
} else {
|
||||
HashMap::new()
|
||||
}
|
||||
},
|
||||
async {
|
||||
if detail.include_resources() {
|
||||
mcp_connection_manager.list_all_resource_templates().await
|
||||
mcp_connection_manager
|
||||
.list_all_resource_templates(|_| true)
|
||||
.await
|
||||
} else {
|
||||
HashMap::new()
|
||||
}
|
||||
|
||||
@@ -133,6 +133,21 @@ of strings; comma-separated strings are not supported. Use \
|
||||
}
|
||||
}
|
||||
|
||||
/// Orchestrator-owned feature settings.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct OrchestratorToml {
|
||||
pub skills: Option<OrchestratorFeatureToml>,
|
||||
pub mcp: Option<OrchestratorFeatureToml>,
|
||||
}
|
||||
|
||||
/// Settings for a feature owned by the orchestrator.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, Eq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
pub struct OrchestratorFeatureToml {
|
||||
pub enabled: Option<bool>,
|
||||
}
|
||||
|
||||
/// Base config deserialized from ~/.codex/config.toml.
|
||||
#[derive(Serialize, Deserialize, Debug, Clone, Default, PartialEq, JsonSchema)]
|
||||
#[schemars(deny_unknown_fields)]
|
||||
@@ -360,6 +375,9 @@ pub struct ConfigToml {
|
||||
/// Optional product SKU forwarded on host-owned Codex Apps MCP requests.
|
||||
pub apps_mcp_product_sku: Option<String>,
|
||||
|
||||
/// Orchestrator-owned feature settings.
|
||||
pub orchestrator: Option<OrchestratorToml>,
|
||||
|
||||
/// Base URL override for the built-in `openai` model provider.
|
||||
pub openai_base_url: Option<String>,
|
||||
|
||||
|
||||
@@ -2090,6 +2090,29 @@
|
||||
}
|
||||
]
|
||||
},
|
||||
"OrchestratorFeatureToml": {
|
||||
"additionalProperties": false,
|
||||
"description": "Settings for a feature owned by the orchestrator.",
|
||||
"properties": {
|
||||
"enabled": {
|
||||
"type": "boolean"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"OrchestratorToml": {
|
||||
"additionalProperties": false,
|
||||
"description": "Orchestrator-owned feature settings.",
|
||||
"properties": {
|
||||
"mcp": {
|
||||
"$ref": "#/definitions/OrchestratorFeatureToml"
|
||||
},
|
||||
"skills": {
|
||||
"$ref": "#/definitions/OrchestratorFeatureToml"
|
||||
}
|
||||
},
|
||||
"type": "object"
|
||||
},
|
||||
"OtelConfigToml": {
|
||||
"additionalProperties": false,
|
||||
"description": "OTEL settings loaded from config.toml. Fields are optional so we can apply defaults.",
|
||||
@@ -5192,6 +5215,14 @@
|
||||
"description": "Base URL override for the built-in `openai` model provider.",
|
||||
"type": "string"
|
||||
},
|
||||
"orchestrator": {
|
||||
"allOf": [
|
||||
{
|
||||
"$ref": "#/definitions/OrchestratorToml"
|
||||
}
|
||||
],
|
||||
"description": "Orchestrator-owned feature settings."
|
||||
},
|
||||
"oss_provider": {
|
||||
"description": "Preferred OSS provider for local models, e.g. \"lmstudio\" or \"ollama\".",
|
||||
"type": "string"
|
||||
|
||||
@@ -9111,6 +9111,39 @@ apps_mcp_product_sku = "tpp"
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn config_loads_orchestrator_settings_from_toml() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
let cfg: ConfigToml = toml::from_str(
|
||||
r#"
|
||||
model = "gpt-5.4"
|
||||
|
||||
[orchestrator.skills]
|
||||
enabled = false
|
||||
|
||||
[orchestrator.mcp]
|
||||
enabled = false
|
||||
"#,
|
||||
)
|
||||
.expect("TOML deserialization should succeed for orchestrator settings");
|
||||
|
||||
let config = Config::load_from_base_config_with_overrides(
|
||||
cfg,
|
||||
ConfigOverrides::default(),
|
||||
codex_home.abs(),
|
||||
)
|
||||
.await?;
|
||||
|
||||
assert_eq!(
|
||||
(
|
||||
config.orchestrator_skills_enabled,
|
||||
config.orchestrator_mcp_enabled
|
||||
),
|
||||
(false, false)
|
||||
);
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn config_loads_mcp_oauth_callback_url_from_toml() -> std::io::Result<()> {
|
||||
let codex_home = TempDir::new()?;
|
||||
|
||||
@@ -690,6 +690,12 @@ pub struct Config {
|
||||
/// Whether to inject the `<skills_instructions>` developer block.
|
||||
pub include_skill_instructions: bool,
|
||||
|
||||
/// Whether orchestrator-owned skills are exposed to the model.
|
||||
pub orchestrator_skills_enabled: bool,
|
||||
|
||||
/// Whether orchestrator-owned MCP tools are exposed to the model.
|
||||
pub orchestrator_mcp_enabled: bool,
|
||||
|
||||
/// Whether to inject the `<environment_context>` user block.
|
||||
pub include_environment_context: bool,
|
||||
|
||||
@@ -2425,6 +2431,12 @@ fn resolve_experimental_request_user_input_enabled(config_toml: &ConfigToml) ->
|
||||
.is_none_or(|config| config.enabled)
|
||||
}
|
||||
|
||||
fn resolve_orchestrator_feature_enabled(
|
||||
feature: Option<&codex_config::config_toml::OrchestratorFeatureToml>,
|
||||
) -> bool {
|
||||
feature.and_then(|feature| feature.enabled).unwrap_or(true)
|
||||
}
|
||||
|
||||
fn resolve_code_mode_config(config_toml: &ConfigToml) -> CodeModeConfig {
|
||||
let base = code_mode_toml_config(config_toml.features.as_ref());
|
||||
|
||||
@@ -2812,6 +2824,11 @@ impl Config {
|
||||
|
||||
validate_model_providers(&cfg.model_providers)
|
||||
.map_err(|message| std::io::Error::new(std::io::ErrorKind::InvalidInput, message))?;
|
||||
let orchestrator = cfg.orchestrator.as_ref();
|
||||
let orchestrator_skills_enabled =
|
||||
resolve_orchestrator_feature_enabled(orchestrator.and_then(|value| value.skills.as_ref()));
|
||||
let orchestrator_mcp_enabled =
|
||||
resolve_orchestrator_feature_enabled(orchestrator.and_then(|value| value.mcp.as_ref()));
|
||||
// Ensure that every field of ConfigRequirements is applied to the final
|
||||
// Config.
|
||||
let ConfigRequirements {
|
||||
@@ -3688,6 +3705,8 @@ impl Config {
|
||||
include_apps_instructions,
|
||||
include_collaboration_mode_instructions,
|
||||
include_skill_instructions,
|
||||
orchestrator_skills_enabled,
|
||||
orchestrator_mcp_enabled,
|
||||
include_environment_context,
|
||||
// The config.toml omits "_mode" because it's a config file. However, "_mode"
|
||||
// is important in code to differentiate the mode from the store implementation.
|
||||
|
||||
@@ -1,6 +1,8 @@
|
||||
use anyhow::Context;
|
||||
use codex_config::config_toml::ConfigLockfileToml;
|
||||
use codex_config::config_toml::ConfigToml;
|
||||
use codex_config::config_toml::OrchestratorFeatureToml;
|
||||
use codex_config::config_toml::OrchestratorToml;
|
||||
use codex_config::types::MemoriesToml;
|
||||
use codex_features::CurrentTimeReminderConfigToml;
|
||||
use codex_features::Feature;
|
||||
@@ -183,6 +185,18 @@ fn save_config_resolved_fields(
|
||||
.skills
|
||||
.get_or_insert_with(Default::default)
|
||||
.include_instructions = Some(config.include_skill_instructions);
|
||||
lock_config
|
||||
.orchestrator
|
||||
.get_or_insert_with(OrchestratorToml::default)
|
||||
.skills
|
||||
.get_or_insert_with(OrchestratorFeatureToml::default)
|
||||
.enabled = Some(config.orchestrator_skills_enabled);
|
||||
lock_config
|
||||
.orchestrator
|
||||
.get_or_insert_with(OrchestratorToml::default)
|
||||
.mcp
|
||||
.get_or_insert_with(Default::default)
|
||||
.enabled = Some(config.orchestrator_mcp_enabled);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -204,6 +204,7 @@ impl TurnContext {
|
||||
self.config
|
||||
.features
|
||||
.apps_enabled_for_auth(uses_codex_backend)
|
||||
&& self.config.orchestrator_mcp_enabled
|
||||
}
|
||||
|
||||
pub(crate) fn tool_environment_mode(&self) -> ToolEnvironmentMode {
|
||||
|
||||
@@ -2,6 +2,7 @@ use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
use codex_mcp::CODEX_APPS_MCP_SERVER_NAME;
|
||||
use codex_protocol::items::McpToolCallError;
|
||||
use codex_protocol::items::McpToolCallItem;
|
||||
use codex_protocol::items::McpToolCallStatus;
|
||||
@@ -33,6 +34,23 @@ pub use list_mcp_resource_templates::ListMcpResourceTemplatesHandler;
|
||||
pub use list_mcp_resources::ListMcpResourcesHandler;
|
||||
pub use read_mcp_resource::ReadMcpResourceHandler;
|
||||
|
||||
fn model_can_access_mcp_server(turn: &TurnContext, server: &str) -> bool {
|
||||
turn.config.orchestrator_mcp_enabled || server != CODEX_APPS_MCP_SERVER_NAME
|
||||
}
|
||||
|
||||
fn ensure_model_can_access_mcp_server(
|
||||
turn: &TurnContext,
|
||||
server: &str,
|
||||
) -> Result<(), FunctionCallError> {
|
||||
if model_can_access_mcp_server(turn, server) {
|
||||
Ok(())
|
||||
} else {
|
||||
Err(FunctionCallError::RespondToModel(format!(
|
||||
"MCP server '{server}' is disabled by `orchestrator.mcp.enabled`"
|
||||
)))
|
||||
}
|
||||
}
|
||||
|
||||
#[derive(Debug, Deserialize, Default)]
|
||||
struct ListResourcesArgs {
|
||||
/// Lists all resources from all servers if not specified.
|
||||
|
||||
@@ -19,6 +19,8 @@ use super::ListResourceTemplatesPayload;
|
||||
use super::call_tool_result_from_content;
|
||||
use super::emit_tool_call_begin;
|
||||
use super::emit_tool_call_end;
|
||||
use super::ensure_model_can_access_mcp_server;
|
||||
use super::model_can_access_mcp_server;
|
||||
use super::normalize_optional_string;
|
||||
use super::parse_args_with_default;
|
||||
use super::parse_arguments;
|
||||
@@ -83,6 +85,7 @@ impl ListMcpResourceTemplatesHandler {
|
||||
|
||||
let payload_result: Result<ListResourceTemplatesPayload, FunctionCallError> = async {
|
||||
if let Some(server_name) = server.clone() {
|
||||
ensure_model_can_access_mcp_server(turn.as_ref(), &server_name)?;
|
||||
let params = cursor
|
||||
.clone()
|
||||
.map(|value| PaginatedRequestParams::default().with_cursor(Some(value)));
|
||||
@@ -109,7 +112,9 @@ impl ListMcpResourceTemplatesHandler {
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.load_full()
|
||||
.list_all_resource_templates()
|
||||
.list_all_resource_templates(|server_name| {
|
||||
model_can_access_mcp_server(turn.as_ref(), server_name)
|
||||
})
|
||||
.await;
|
||||
Ok(ListResourceTemplatesPayload::from_all_servers(templates))
|
||||
}
|
||||
|
||||
@@ -19,6 +19,8 @@ use super::ListResourcesPayload;
|
||||
use super::call_tool_result_from_content;
|
||||
use super::emit_tool_call_begin;
|
||||
use super::emit_tool_call_end;
|
||||
use super::ensure_model_can_access_mcp_server;
|
||||
use super::model_can_access_mcp_server;
|
||||
use super::normalize_optional_string;
|
||||
use super::parse_args_with_default;
|
||||
use super::parse_arguments;
|
||||
@@ -83,6 +85,7 @@ impl ListMcpResourcesHandler {
|
||||
|
||||
let payload_result: Result<ListResourcesPayload, FunctionCallError> = async {
|
||||
if let Some(server_name) = server.clone() {
|
||||
ensure_model_can_access_mcp_server(turn.as_ref(), &server_name)?;
|
||||
let params = cursor
|
||||
.clone()
|
||||
.map(|value| PaginatedRequestParams::default().with_cursor(Some(value)));
|
||||
@@ -107,7 +110,9 @@ impl ListMcpResourcesHandler {
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.load_full()
|
||||
.list_all_resources()
|
||||
.list_all_resources(|server_name| {
|
||||
model_can_access_mcp_server(turn.as_ref(), server_name)
|
||||
})
|
||||
.await;
|
||||
Ok(ListResourcesPayload::from_all_servers(resources))
|
||||
}
|
||||
|
||||
@@ -19,6 +19,7 @@ use super::ReadResourcePayload;
|
||||
use super::call_tool_result_from_content;
|
||||
use super::emit_tool_call_begin;
|
||||
use super::emit_tool_call_end;
|
||||
use super::ensure_model_can_access_mcp_server;
|
||||
use super::normalize_required_string;
|
||||
use super::parse_args;
|
||||
use super::parse_arguments;
|
||||
@@ -82,6 +83,7 @@ impl ReadMcpResourceHandler {
|
||||
let start = Instant::now();
|
||||
|
||||
let payload_result: Result<ReadResourcePayload, FunctionCallError> = async {
|
||||
ensure_model_can_access_mcp_server(turn.as_ref(), &server)?;
|
||||
let result = session
|
||||
.read_resource(&server, ReadResourceRequestParams::new(uri.clone()))
|
||||
.await
|
||||
|
||||
@@ -57,6 +57,7 @@ use core_test_support::responses::ResponsesRequest;
|
||||
use core_test_support::responses::ev_assistant_message;
|
||||
use core_test_support::responses::ev_completed;
|
||||
use core_test_support::responses::ev_completed_with_tokens;
|
||||
use core_test_support::responses::ev_function_call;
|
||||
use core_test_support::responses::ev_message_item_added;
|
||||
use core_test_support::responses::ev_output_text_delta;
|
||||
use core_test_support::responses::ev_response_created;
|
||||
@@ -1678,6 +1679,123 @@ async fn omits_apps_guidance_when_configured_off() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn omits_apps_guidance_when_orchestrator_mcp_is_disabled() {
|
||||
skip_if_no_network!();
|
||||
let server = MockServer::start().await;
|
||||
let apps_server = AppsTestServer::mount(&server)
|
||||
.await
|
||||
.expect("mount apps MCP mock");
|
||||
let apps_base_url = apps_server.chatgpt_base_url.clone();
|
||||
|
||||
let list_call_id = "list-resources";
|
||||
let read_call_id = "read-resource";
|
||||
let resp_mock = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
sse(vec![
|
||||
ev_response_created("resp1"),
|
||||
ev_function_call(list_call_id, "list_mcp_resources", "{}"),
|
||||
ev_completed("resp1"),
|
||||
]),
|
||||
sse(vec![
|
||||
ev_response_created("resp2"),
|
||||
ev_function_call(
|
||||
read_call_id,
|
||||
"read_mcp_resource",
|
||||
&json!({
|
||||
"server": "codex_apps",
|
||||
"uri": "skill://demo/SKILL.md",
|
||||
})
|
||||
.to_string(),
|
||||
),
|
||||
ev_completed("resp2"),
|
||||
]),
|
||||
sse(vec![ev_response_created("resp3"), ev_completed("resp3")]),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_auth(create_dummy_codex_auth())
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Apps)
|
||||
.expect("test config should allow feature update");
|
||||
config.chatgpt_base_url = apps_base_url;
|
||||
config.orchestrator_mcp_enabled = false;
|
||||
});
|
||||
let codex = builder
|
||||
.build(&server)
|
||||
.await
|
||||
.expect("create new conversation")
|
||||
.codex;
|
||||
|
||||
codex
|
||||
.submit(Op::UserInput {
|
||||
items: vec![UserInput::Text {
|
||||
text: "hello".into(),
|
||||
text_elements: Vec::new(),
|
||||
}],
|
||||
final_output_json_schema: None,
|
||||
responsesapi_client_metadata: None,
|
||||
additional_context: Default::default(),
|
||||
thread_settings: Default::default(),
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
let requests = resp_mock.requests();
|
||||
assert_eq!(requests.len(), 3);
|
||||
let request = &requests[0];
|
||||
assert!(
|
||||
!message_input_text_contains(request, "developer", "<apps_instructions>"),
|
||||
"did not expect apps instructions when orchestrator MCP is disabled, got {:?}",
|
||||
request.body_json()["input"]
|
||||
);
|
||||
assert!(
|
||||
!request.body_contains_text("mcp__codex_apps"),
|
||||
"did not expect codex_apps MCP tools when orchestrator MCP is disabled, got {:?}",
|
||||
request.body_json()["tools"]
|
||||
);
|
||||
let list_output = requests[1]
|
||||
.function_call_output_text(list_call_id)
|
||||
.expect("resource list output should be sent to the model");
|
||||
assert_eq!(
|
||||
serde_json::from_str::<serde_json::Value>(&list_output)
|
||||
.expect("parse resource list output"),
|
||||
json!({"resources": []})
|
||||
);
|
||||
let read_output = requests[2]
|
||||
.function_call_output_text(read_call_id)
|
||||
.expect("resource read output should be sent to the model");
|
||||
assert!(
|
||||
read_output.contains("disabled by `orchestrator.mcp.enabled`"),
|
||||
"unexpected resource read output: {read_output}"
|
||||
);
|
||||
|
||||
let resource_methods = server
|
||||
.received_requests()
|
||||
.await
|
||||
.expect("read recorded requests")
|
||||
.into_iter()
|
||||
.filter_map(|request| serde_json::from_slice::<serde_json::Value>(&request.body).ok())
|
||||
.filter_map(|body| {
|
||||
body.get("method")
|
||||
.and_then(serde_json::Value::as_str)
|
||||
.map(str::to_owned)
|
||||
})
|
||||
.filter(|method| method.starts_with("resources/"))
|
||||
.collect::<Vec<_>>();
|
||||
assert!(
|
||||
resource_methods.is_empty(),
|
||||
"did not expect codex_apps resource calls: {resource_methods:?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn omits_environment_context_when_configured_off() {
|
||||
let server = MockServer::start().await;
|
||||
|
||||
@@ -5,4 +5,6 @@ pub struct SkillsExtensionConfig {
|
||||
pub include_instructions: bool,
|
||||
/// Whether bundled skills are eligible for discovery.
|
||||
pub bundled_skills_enabled: bool,
|
||||
/// Whether orchestrator-owned skills are eligible for discovery.
|
||||
pub orchestrator_skills_enabled: bool,
|
||||
}
|
||||
|
||||
@@ -61,14 +61,14 @@ where
|
||||
.get::<Vec<SelectedCapabilityRoot>>()
|
||||
.map(|selected_roots| selected_roots.as_ref().clone())
|
||||
.unwrap_or_default();
|
||||
let orchestrator_skills_enabled = !input
|
||||
let orchestrator_skills_available = !input
|
||||
.environments
|
||||
.iter()
|
||||
.any(|environment| environment.environment_id == LOCAL_ENVIRONMENT_ID);
|
||||
input.thread_store.insert(SkillsThreadState::new(
|
||||
(self.config_from_host)(input.config),
|
||||
selected_roots,
|
||||
orchestrator_skills_enabled,
|
||||
orchestrator_skills_available,
|
||||
));
|
||||
})
|
||||
}
|
||||
@@ -89,11 +89,11 @@ where
|
||||
if let Some(state) = thread_store.get::<SkillsThreadState>() {
|
||||
state.set_config(next_config);
|
||||
} else {
|
||||
let orchestrator_skills_enabled = true;
|
||||
let orchestrator_skills_available = true;
|
||||
thread_store.insert(SkillsThreadState::new(
|
||||
next_config,
|
||||
Vec::new(),
|
||||
orchestrator_skills_enabled,
|
||||
orchestrator_skills_available,
|
||||
));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -27,7 +27,7 @@ const MAX_CACHED_ORCHESTRATOR_CONTENT_BYTES: usize = 8 * 1024 * 1024;
|
||||
pub(crate) struct SkillsThreadState {
|
||||
config: Mutex<SkillsExtensionConfig>,
|
||||
selected_roots: Vec<SelectedCapabilityRoot>,
|
||||
orchestrator_skills_enabled: bool,
|
||||
orchestrator_skills_available: bool,
|
||||
orchestrator_cache: Mutex<Option<Arc<OrchestratorGenerationCache>>>,
|
||||
}
|
||||
|
||||
@@ -35,12 +35,12 @@ impl SkillsThreadState {
|
||||
pub(crate) fn new(
|
||||
config: SkillsExtensionConfig,
|
||||
selected_roots: Vec<SelectedCapabilityRoot>,
|
||||
orchestrator_skills_enabled: bool,
|
||||
orchestrator_skills_available: bool,
|
||||
) -> Self {
|
||||
Self {
|
||||
config: Mutex::new(config),
|
||||
selected_roots,
|
||||
orchestrator_skills_enabled,
|
||||
orchestrator_skills_available,
|
||||
orchestrator_cache: Mutex::new(None),
|
||||
}
|
||||
}
|
||||
@@ -64,7 +64,7 @@ impl SkillsThreadState {
|
||||
}
|
||||
|
||||
pub(crate) fn orchestrator_skills_enabled(&self) -> bool {
|
||||
self.orchestrator_skills_enabled
|
||||
self.orchestrator_skills_available && self.config().orchestrator_skills_enabled
|
||||
}
|
||||
|
||||
pub(crate) async fn orchestrator_catalog_snapshot(
|
||||
|
||||
@@ -565,12 +565,14 @@ fn test_entry(
|
||||
struct TestConfig {
|
||||
include_instructions: bool,
|
||||
bundled_skills_enabled: bool,
|
||||
orchestrator_skills_enabled: bool,
|
||||
}
|
||||
|
||||
fn default_config() -> TestConfig {
|
||||
TestConfig {
|
||||
include_instructions: true,
|
||||
bundled_skills_enabled: true,
|
||||
orchestrator_skills_enabled: true,
|
||||
}
|
||||
}
|
||||
|
||||
@@ -578,6 +580,7 @@ fn skills_extension_config(config: &TestConfig) -> SkillsExtensionConfig {
|
||||
SkillsExtensionConfig {
|
||||
include_instructions: config.include_instructions,
|
||||
bundled_skills_enabled: config.bundled_skills_enabled,
|
||||
orchestrator_skills_enabled: config.orchestrator_skills_enabled,
|
||||
}
|
||||
}
|
||||
|
||||
|
||||
@@ -197,6 +197,8 @@ fn new_config(model: Option<String>, arg0_paths: Arg0DispatchPaths) -> anyhow::R
|
||||
include_apps_instructions: false,
|
||||
include_collaboration_mode_instructions: false,
|
||||
include_skill_instructions: false,
|
||||
orchestrator_skills_enabled: false,
|
||||
orchestrator_mcp_enabled: false,
|
||||
include_environment_context: false,
|
||||
compact_prompt: None,
|
||||
notify: None,
|
||||
|
||||
Reference in New Issue
Block a user