mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
[apps] Fix apps enablement condition. (#14011)
- [x] Fix apps enablement condition to check both the feature flag and that the user is not an API key user.
This commit is contained in:
@@ -212,6 +212,10 @@ impl CodexAuth {
|
||||
}
|
||||
}
|
||||
|
||||
pub fn is_api_key_auth(&self) -> bool {
|
||||
self.auth_mode() == AuthMode::ApiKey
|
||||
}
|
||||
|
||||
pub fn is_chatgpt_auth(&self) -> bool {
|
||||
self.auth_mode() == AuthMode::Chatgpt
|
||||
}
|
||||
|
||||
+29
-25
@@ -736,6 +736,11 @@ impl TurnContext {
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn apps_enabled(&self) -> bool {
|
||||
self.features
|
||||
.apps_enabled_cached(self.auth_manager.as_deref())
|
||||
}
|
||||
|
||||
pub(crate) async fn with_model(&self, model: String, models_manager: &ModelsManager) -> Self {
|
||||
let mut config = (*self.config).clone();
|
||||
config.model = Some(model.clone());
|
||||
@@ -3407,7 +3412,7 @@ impl Session {
|
||||
);
|
||||
}
|
||||
}
|
||||
if turn_context.features.enabled(Feature::Apps) {
|
||||
if turn_context.apps_enabled() {
|
||||
developer_sections.push(render_apps_section());
|
||||
}
|
||||
if turn_context.features.enabled(Feature::CodexGitCommit)
|
||||
@@ -3894,7 +3899,7 @@ impl Session {
|
||||
.tool_plugin_provenance(config.as_ref());
|
||||
let mcp_servers = with_codex_apps_mcp(
|
||||
mcp_servers,
|
||||
self.features.enabled(Feature::Apps),
|
||||
self.features.apps_enabled_for_auth(auth.as_ref()),
|
||||
auth.as_ref(),
|
||||
config.as_ref(),
|
||||
);
|
||||
@@ -5357,28 +5362,27 @@ pub(crate) async fn run_turn(
|
||||
// enabled plugins, then converted into turn-scoped guidance below.
|
||||
let mentioned_plugins =
|
||||
collect_explicit_plugin_mentions(&input, loaded_plugins.capability_summaries());
|
||||
let mcp_tools =
|
||||
if turn_context.config.features.enabled(Feature::Apps) || !mentioned_plugins.is_empty() {
|
||||
// Plugin mentions need raw MCP/app inventory even when app tools
|
||||
// are normally hidden so we can describe the plugin's currently
|
||||
// usable capabilities for this turn.
|
||||
match sess
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.list_all_tools()
|
||||
.or_cancel(&cancellation_token)
|
||||
.await
|
||||
{
|
||||
Ok(mcp_tools) => mcp_tools,
|
||||
Err(_) if turn_context.config.features.enabled(Feature::Apps) => return None,
|
||||
Err(_) => HashMap::new(),
|
||||
}
|
||||
} else {
|
||||
HashMap::new()
|
||||
};
|
||||
let available_connectors = if turn_context.config.features.enabled(Feature::Apps) {
|
||||
let mcp_tools = if turn_context.apps_enabled() || !mentioned_plugins.is_empty() {
|
||||
// Plugin mentions need raw MCP/app inventory even when app tools
|
||||
// are normally hidden so we can describe the plugin's currently
|
||||
// usable capabilities for this turn.
|
||||
match sess
|
||||
.services
|
||||
.mcp_connection_manager
|
||||
.read()
|
||||
.await
|
||||
.list_all_tools()
|
||||
.or_cancel(&cancellation_token)
|
||||
.await
|
||||
{
|
||||
Ok(mcp_tools) => mcp_tools,
|
||||
Err(_) if turn_context.apps_enabled() => return None,
|
||||
Err(_) => HashMap::new(),
|
||||
}
|
||||
} else {
|
||||
HashMap::new()
|
||||
};
|
||||
let available_connectors = if turn_context.apps_enabled() {
|
||||
let connectors = connectors::merge_plugin_apps_with_accessible(
|
||||
loaded_plugins.effective_apps(),
|
||||
connectors::accessible_connectors_from_mcp_tools(&mcp_tools),
|
||||
@@ -6234,7 +6238,7 @@ async fn built_tools(
|
||||
let mut effective_explicitly_enabled_connectors = explicitly_enabled_connectors.clone();
|
||||
effective_explicitly_enabled_connectors.extend(sess.get_connector_selection().await);
|
||||
|
||||
let connectors = if turn_context.features.enabled(Feature::Apps) {
|
||||
let connectors = if turn_context.apps_enabled() {
|
||||
let connectors = connectors::merge_plugin_apps_with_accessible(
|
||||
loaded_plugins.effective_apps(),
|
||||
connectors::accessible_connectors_from_mcp_tools(&mcp_tools),
|
||||
|
||||
@@ -93,12 +93,11 @@ pub async fn list_accessible_connectors_from_mcp_tools(
|
||||
pub async fn list_cached_accessible_connectors_from_mcp_tools(
|
||||
config: &Config,
|
||||
) -> Option<Vec<AppInfo>> {
|
||||
if !config.features.enabled(Feature::Apps) {
|
||||
return Some(Vec::new());
|
||||
}
|
||||
|
||||
let auth_manager = auth_manager_from_config(config);
|
||||
let auth = auth_manager.auth().await;
|
||||
if !config.features.apps_enabled_for_auth(auth.as_ref()) {
|
||||
return Some(Vec::new());
|
||||
}
|
||||
let cache_key = accessible_connectors_cache_key(config, auth.as_ref());
|
||||
read_cached_accessible_connectors(&cache_key).map(filter_disallowed_connectors)
|
||||
}
|
||||
@@ -118,15 +117,14 @@ pub async fn list_accessible_connectors_from_mcp_tools_with_options_and_status(
|
||||
config: &Config,
|
||||
force_refetch: bool,
|
||||
) -> anyhow::Result<AccessibleConnectorsStatus> {
|
||||
if !config.features.enabled(Feature::Apps) {
|
||||
let auth_manager = auth_manager_from_config(config);
|
||||
let auth = auth_manager.auth().await;
|
||||
if !config.features.apps_enabled_for_auth(auth.as_ref()) {
|
||||
return Ok(AccessibleConnectorsStatus {
|
||||
connectors: Vec::new(),
|
||||
codex_apps_ready: true,
|
||||
});
|
||||
}
|
||||
|
||||
let auth_manager = auth_manager_from_config(config);
|
||||
let auth = auth_manager.auth().await;
|
||||
let cache_key = accessible_connectors_cache_key(config, auth.as_ref());
|
||||
let mcp_manager = McpManager::new(Arc::new(PluginsManager::new(config.codex_home.clone())));
|
||||
let tool_plugin_provenance = mcp_manager.tool_plugin_provenance(config);
|
||||
|
||||
@@ -5,6 +5,8 @@
|
||||
//! booleans through multiple types, call sites consult a single `Features`
|
||||
//! container attached to `Config`.
|
||||
|
||||
use crate::auth::AuthManager;
|
||||
use crate::auth::CodexAuth;
|
||||
use crate::config::Config;
|
||||
use crate::config::ConfigToml;
|
||||
use crate::config::profile::ConfigProfile;
|
||||
@@ -257,6 +259,27 @@ impl Features {
|
||||
self.enabled.contains(&f)
|
||||
}
|
||||
|
||||
pub async fn apps_enabled(&self, auth_manager: Option<&AuthManager>) -> bool {
|
||||
if !self.enabled(Feature::Apps) {
|
||||
return false;
|
||||
}
|
||||
|
||||
let auth = match auth_manager {
|
||||
Some(auth_manager) => auth_manager.auth().await,
|
||||
None => None,
|
||||
};
|
||||
self.apps_enabled_for_auth(auth.as_ref())
|
||||
}
|
||||
|
||||
pub fn apps_enabled_cached(&self, auth_manager: Option<&AuthManager>) -> bool {
|
||||
let auth = auth_manager.and_then(AuthManager::auth_cached);
|
||||
self.apps_enabled_for_auth(auth.as_ref())
|
||||
}
|
||||
|
||||
pub(crate) fn apps_enabled_for_auth(&self, auth: Option<&CodexAuth>) -> bool {
|
||||
self.enabled(Feature::Apps) && auth.is_some_and(CodexAuth::is_chatgpt_auth)
|
||||
}
|
||||
|
||||
pub fn enable(&mut self, f: Feature) -> &mut Self {
|
||||
self.enabled.insert(f);
|
||||
self
|
||||
@@ -973,4 +996,19 @@ mod tests {
|
||||
assert_eq!(feature_for_key("multi_agent"), Some(Feature::Collab));
|
||||
assert_eq!(feature_for_key("collab"), Some(Feature::Collab));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn apps_require_feature_flag_and_chatgpt_auth() {
|
||||
let mut features = Features::with_defaults();
|
||||
assert!(!features.apps_enabled_for_auth(None));
|
||||
|
||||
features.enable(Feature::Apps);
|
||||
assert!(!features.apps_enabled_for_auth(None));
|
||||
|
||||
let api_key_auth = CodexAuth::from_api_key("test-api-key");
|
||||
assert!(!features.apps_enabled_for_auth(Some(&api_key_auth)));
|
||||
|
||||
let chatgpt_auth = CodexAuth::create_dummy_chatgpt_auth_for_testing();
|
||||
assert!(features.apps_enabled_for_auth(Some(&chatgpt_auth)));
|
||||
}
|
||||
}
|
||||
|
||||
@@ -268,7 +268,7 @@ fn effective_mcp_servers(
|
||||
let servers = configured_mcp_servers(config, plugins_manager);
|
||||
with_codex_apps_mcp(
|
||||
servers,
|
||||
config.features.enabled(Feature::Apps),
|
||||
config.features.apps_enabled_for_auth(auth),
|
||||
auth,
|
||||
config,
|
||||
)
|
||||
|
||||
@@ -1294,8 +1294,13 @@ fn create_search_tool_bm25_tool(app_tools: &HashMap<String, ToolInfo>) -> ToolSp
|
||||
app_names.dedup();
|
||||
let app_names = app_names.join(", ");
|
||||
|
||||
let description =
|
||||
SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str());
|
||||
let description = if app_names.is_empty() {
|
||||
SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE
|
||||
.replace("({{app_names}})", "(None currently enabled)")
|
||||
.replace("{{app_names}}", "available apps")
|
||||
} else {
|
||||
SEARCH_TOOL_BM25_DESCRIPTION_TEMPLATE.replace("{{app_names}}", app_names.as_str())
|
||||
};
|
||||
|
||||
ToolSpec::Function(ResponsesApiTool {
|
||||
name: SEARCH_TOOL_BM25_TOOL_NAME.to_string(),
|
||||
@@ -1996,9 +2001,8 @@ pub(crate) fn build_specs(
|
||||
builder.register_handler("request_permissions", request_permissions_handler);
|
||||
}
|
||||
|
||||
if config.search_tool
|
||||
&& let Some(app_tools) = app_tools
|
||||
{
|
||||
if config.search_tool {
|
||||
let app_tools = app_tools.unwrap_or_default();
|
||||
builder.push_spec_with_parallel_support(create_search_tool_bm25_tool(&app_tools), true);
|
||||
builder.register_handler(SEARCH_TOOL_BM25_TOOL_NAME, search_tool_handler);
|
||||
}
|
||||
@@ -3393,6 +3397,74 @@ mod tests {
|
||||
assert!(!description.contains("mcp__rmcp__echo"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn search_tool_requires_apps_feature_flag_only() {
|
||||
let config = test_config();
|
||||
let model_info =
|
||||
ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config);
|
||||
let app_tools = Some(HashMap::from([(
|
||||
"mcp__codex_apps__calendar_create_event".to_string(),
|
||||
ToolInfo {
|
||||
server_name: crate::mcp::CODEX_APPS_MCP_SERVER_NAME.to_string(),
|
||||
tool_name: "calendar_create_event".to_string(),
|
||||
tool: mcp_tool(
|
||||
"calendar_create_event",
|
||||
"Create calendar event",
|
||||
serde_json::json!({"type": "object"}),
|
||||
),
|
||||
connector_id: Some("calendar".to_string()),
|
||||
connector_name: Some("Calendar".to_string()),
|
||||
plugin_display_names: Vec::new(),
|
||||
},
|
||||
)]));
|
||||
|
||||
let features = Features::with_defaults();
|
||||
let tools_config = ToolsConfig::new(&ToolsConfigParams {
|
||||
model_info: &model_info,
|
||||
features: &features,
|
||||
web_search_mode: Some(WebSearchMode::Cached),
|
||||
session_source: SessionSource::Cli,
|
||||
});
|
||||
let (tools, _) = build_specs(&tools_config, None, app_tools.clone(), &[]).build();
|
||||
assert_lacks_tool_name(&tools, SEARCH_TOOL_BM25_TOOL_NAME);
|
||||
|
||||
let mut features = Features::with_defaults();
|
||||
features.enable(Feature::Apps);
|
||||
let tools_config = ToolsConfig::new(&ToolsConfigParams {
|
||||
model_info: &model_info,
|
||||
features: &features,
|
||||
web_search_mode: Some(WebSearchMode::Cached),
|
||||
session_source: SessionSource::Cli,
|
||||
});
|
||||
let (tools, _) = build_specs(&tools_config, None, app_tools, &[]).build();
|
||||
assert_contains_tool_names(&tools, &[SEARCH_TOOL_BM25_TOOL_NAME]);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn search_tool_description_handles_no_enabled_apps() {
|
||||
let config = test_config();
|
||||
let model_info =
|
||||
ModelsManager::construct_model_info_offline_for_tests("gpt-5-codex", &config);
|
||||
let mut features = Features::with_defaults();
|
||||
features.enable(Feature::Apps);
|
||||
let tools_config = ToolsConfig::new(&ToolsConfigParams {
|
||||
model_info: &model_info,
|
||||
features: &features,
|
||||
web_search_mode: Some(WebSearchMode::Cached),
|
||||
session_source: SessionSource::Cli,
|
||||
});
|
||||
|
||||
let (tools, _) = build_specs(&tools_config, None, Some(HashMap::new()), &[]).build();
|
||||
let search_tool = find_tool(&tools, SEARCH_TOOL_BM25_TOOL_NAME);
|
||||
let ToolSpec::Function(ResponsesApiTool { description, .. }) = &search_tool.spec else {
|
||||
panic!("expected function tool");
|
||||
};
|
||||
|
||||
assert!(description.contains("(None currently enabled)"));
|
||||
assert!(description.contains("available apps."));
|
||||
assert!(!description.contains("{{app_names}}"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn test_mcp_tool_property_missing_type_defaults_to_string() {
|
||||
let config = test_config();
|
||||
|
||||
@@ -921,7 +921,7 @@ async fn includes_user_instructions_message_in_request() {
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn includes_apps_guidance_as_developer_message_when_enabled() {
|
||||
async fn includes_apps_guidance_as_developer_message_for_chatgpt_auth() {
|
||||
skip_if_no_network!();
|
||||
let server = MockServer::start().await;
|
||||
let apps_server = AppsTestServer::mount(&server)
|
||||
@@ -936,7 +936,7 @@ async fn includes_apps_guidance_as_developer_message_when_enabled() {
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_auth(create_dummy_codex_auth())
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
@@ -1011,6 +1011,76 @@ async fn includes_apps_guidance_as_developer_message_when_enabled() {
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn omits_apps_guidance_for_api_key_auth_even_when_feature_enabled() {
|
||||
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 resp_mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![ev_response_created("resp1"), ev_completed("resp1")]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Apps)
|
||||
.expect("test config should allow feature update");
|
||||
config
|
||||
.features
|
||||
.disable(Feature::AppsMcpGateway)
|
||||
.expect("test config should allow feature update");
|
||||
config.chatgpt_base_url = apps_base_url;
|
||||
});
|
||||
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,
|
||||
})
|
||||
.await
|
||||
.unwrap();
|
||||
|
||||
wait_for_event(&codex, |ev| matches!(ev, EventMsg::TurnComplete(_))).await;
|
||||
|
||||
let request = resp_mock.single_request();
|
||||
let request_body = request.body_json();
|
||||
let input = request_body["input"].as_array().expect("input array");
|
||||
let apps_snippet = "Apps are mentioned in the prompt in the format";
|
||||
|
||||
let has_apps_guidance = input.iter().any(|item| {
|
||||
item.get("content")
|
||||
.and_then(|value| value.as_array())
|
||||
.is_some_and(|content| {
|
||||
content.iter().any(|entry| {
|
||||
entry
|
||||
.get("text")
|
||||
.and_then(|value| value.as_str())
|
||||
.is_some_and(|text| text.contains(apps_snippet))
|
||||
})
|
||||
})
|
||||
});
|
||||
assert!(
|
||||
!has_apps_guidance,
|
||||
"did not expect apps guidance for API key auth, got {input:#?}"
|
||||
);
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn skills_append_to_instructions() {
|
||||
skip_if_no_network!();
|
||||
|
||||
@@ -114,7 +114,7 @@ async fn build_apps_enabled_plugin_test_codex(
|
||||
) -> Result<Arc<codex_core::CodexThread>> {
|
||||
let mut builder = test_codex()
|
||||
.with_home(codex_home)
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
|
||||
@@ -5,6 +5,7 @@ use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_core::CodexAuth;
|
||||
use codex_core::CodexThread;
|
||||
use codex_core::NewThread;
|
||||
use codex_core::config::Config;
|
||||
@@ -163,9 +164,11 @@ fn configure_apps_with_optional_rmcp(
|
||||
}
|
||||
|
||||
fn configured_builder(apps_base_url: String, rmcp_server_bin: Option<String>) -> TestCodexBuilder {
|
||||
test_codex().with_config(move |config| {
|
||||
configure_apps_with_optional_rmcp(config, apps_base_url.as_str(), rmcp_server_bin);
|
||||
})
|
||||
test_codex()
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_config(move |config| {
|
||||
configure_apps_with_optional_rmcp(config, apps_base_url.as_str(), rmcp_server_bin);
|
||||
})
|
||||
}
|
||||
|
||||
async fn submit_user_input(thread: &Arc<CodexThread>, text: &str) -> Result<()> {
|
||||
@@ -218,6 +221,46 @@ async fn search_tool_flag_adds_tool() -> Result<()> {
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn search_tool_flag_adds_tool_for_api_key_auth() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let mock = mount_sse_once(
|
||||
&server,
|
||||
sse(vec![
|
||||
ev_response_created("resp-1"),
|
||||
ev_assistant_message("msg-1", "done"),
|
||||
ev_completed("resp-1"),
|
||||
]),
|
||||
)
|
||||
.await;
|
||||
|
||||
let mut builder = test_codex()
|
||||
.with_auth(CodexAuth::from_api_key("Test API Key"))
|
||||
.with_config(move |config| {
|
||||
configure_apps_with_optional_rmcp(config, apps_server.chatgpt_base_url.as_str(), None);
|
||||
});
|
||||
let test = builder.build(&server).await?;
|
||||
|
||||
test.submit_turn_with_policies(
|
||||
"list tools",
|
||||
AskForApproval::Never,
|
||||
SandboxPolicy::DangerFullAccess,
|
||||
)
|
||||
.await?;
|
||||
|
||||
let body = mock.single_request().body_json();
|
||||
let tools = tool_names(&body);
|
||||
assert!(
|
||||
tools.iter().any(|name| name == SEARCH_TOOL_BM25_TOOL_NAME),
|
||||
"tools list should include {SEARCH_TOOL_BM25_TOOL_NAME} for API key auth when Apps is enabled: {tools:?}"
|
||||
);
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn search_tool_adds_discovery_instructions_to_tool_description() -> Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
Reference in New Issue
Block a user