mirror of
https://github.com/openai/codex.git
synced 2026-11-09 20:50:33 +00:00
Simplify review MCP isolation
This commit is contained in:
@@ -17,6 +17,7 @@ use tracing::warn;
|
||||
|
||||
use crate::SkillMetadata;
|
||||
use crate::session::session::Session;
|
||||
use crate::session::turn_context::McpAccess;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::skills::model::SkillToolDependency;
|
||||
use codex_mcp::ElicitationReviewerHandle;
|
||||
@@ -45,7 +46,8 @@ pub(crate) async fn maybe_prompt_and_install_mcp_dependencies(
|
||||
}
|
||||
|
||||
let config = turn_context.config.clone();
|
||||
if mentioned_skills.is_empty()
|
||||
if turn_context.mcp_access() == McpAccess::Disabled
|
||||
|| mentioned_skills.is_empty()
|
||||
|| !config
|
||||
.features
|
||||
.enabled(codex_features::Feature::SkillMcpDependencyInstall)
|
||||
|
||||
@@ -173,7 +173,7 @@ impl Session {
|
||||
.await
|
||||
}
|
||||
|
||||
pub(crate) async fn isolated_mcp_runtime_for_step(
|
||||
pub(crate) fn disabled_mcp_runtime_for_step(
|
||||
&self,
|
||||
turn_context: &TurnContext,
|
||||
environments: &TurnEnvironmentSnapshot,
|
||||
@@ -181,16 +181,11 @@ impl Session {
|
||||
) -> Arc<McpRuntimeSnapshot> {
|
||||
let available_environment_ids =
|
||||
Self::available_selected_environment_ids(selected_capability_roots);
|
||||
let mcp_config = self
|
||||
.services
|
||||
.mcp_manager
|
||||
.runtime_config_for_step(
|
||||
&turn_context.config,
|
||||
&self.services.mcp_thread_init,
|
||||
&self.services.thread_extension_data,
|
||||
&available_environment_ids,
|
||||
)
|
||||
.await;
|
||||
let mut mcp_config = self.services.latest_mcp_runtime().config().clone();
|
||||
mcp_config.apps_enabled = false;
|
||||
mcp_config.skill_mcp_dependency_install_enabled = false;
|
||||
mcp_config.mcp_server_catalog = Default::default();
|
||||
mcp_config.connector_snapshot = Default::default();
|
||||
let environment_manager = self.services.turn_environments.environment_manager();
|
||||
#[allow(deprecated)]
|
||||
let cwd = environments
|
||||
|
||||
@@ -239,6 +239,7 @@ use self::turn::AssistantMessageStreamParsers;
|
||||
#[cfg(test)]
|
||||
use self::turn::collect_explicit_app_ids_from_skill_items;
|
||||
use self::turn::realtime_text_for_event;
|
||||
use self::turn_context::McpAccess;
|
||||
use self::turn_context::TurnContext;
|
||||
use self::turn_context::TurnSkillsContext;
|
||||
#[cfg(test)]
|
||||
@@ -2855,20 +2856,20 @@ impl Session {
|
||||
let selected_capability_roots = self
|
||||
.resolve_selected_capability_roots_for_step(&environments)
|
||||
.await;
|
||||
let mcp = if turn_context.is_review_subagent() {
|
||||
self.isolated_mcp_runtime_for_step(
|
||||
let mcp = match turn_context.mcp_access() {
|
||||
McpAccess::Disabled => self.disabled_mcp_runtime_for_step(
|
||||
turn_context.as_ref(),
|
||||
&environments,
|
||||
&selected_capability_roots,
|
||||
)
|
||||
.await
|
||||
} else {
|
||||
self.mcp_runtime_for_step(
|
||||
turn_context.as_ref(),
|
||||
&environments,
|
||||
&selected_capability_roots,
|
||||
)
|
||||
.await
|
||||
),
|
||||
McpAccess::Enabled => {
|
||||
self.mcp_runtime_for_step(
|
||||
turn_context.as_ref(),
|
||||
&environments,
|
||||
&selected_capability_roots,
|
||||
)
|
||||
.await
|
||||
}
|
||||
};
|
||||
Arc::new(StepContext::new(
|
||||
turn_context,
|
||||
|
||||
@@ -1,5 +1,4 @@
|
||||
use super::*;
|
||||
use std::collections::HashMap;
|
||||
use std::sync::atomic::AtomicBool;
|
||||
|
||||
/// Spawn a review thread using the given prompt.
|
||||
@@ -45,7 +44,7 @@ pub(super) async fn spawn_review_thread(
|
||||
// Build per‑turn client with the requested model/family.
|
||||
let mut per_turn_config = (*config).clone();
|
||||
per_turn_config.model = Some(model.clone());
|
||||
per_turn_config.features = review_features.clone();
|
||||
per_turn_config.features = review_features;
|
||||
per_turn_config.permissions.shell_environment_policy = parent_turn_context
|
||||
.config
|
||||
.permissions
|
||||
@@ -54,13 +53,6 @@ pub(super) async fn spawn_review_thread(
|
||||
per_turn_config.codex_linux_sandbox_exe =
|
||||
parent_turn_context.config.codex_linux_sandbox_exe.clone();
|
||||
per_turn_config.compact_prompt = parent_turn_context.config.compact_prompt.clone();
|
||||
per_turn_config.include_apps_instructions = false;
|
||||
per_turn_config.orchestrator_mcp_enabled = false;
|
||||
per_turn_config.mcp_servers = codex_config::Constrained::allow_only(HashMap::new());
|
||||
let _ = per_turn_config.features.disable(Feature::Apps);
|
||||
let _ = per_turn_config
|
||||
.features
|
||||
.disable(Feature::SkillMcpDependencyInstall);
|
||||
if let Err(err) = per_turn_config.web_search_mode.set(review_web_search_mode) {
|
||||
let fallback_value = per_turn_config.web_search_mode.value();
|
||||
tracing::warn!(
|
||||
|
||||
@@ -677,10 +677,7 @@ impl Session {
|
||||
let mcp_runtime_context =
|
||||
McpRuntimeContext::new(Arc::clone(&environment_manager), mcp_runtime_cwd);
|
||||
let mcp_runtime_context_for_auth = mcp_runtime_context.clone();
|
||||
let start_mcp_clients = !matches!(
|
||||
session_configuration.session_source,
|
||||
SessionSource::SubAgent(SubAgentSource::Review)
|
||||
);
|
||||
let mcp_access = McpAccess::for_session_source(&session_configuration.session_source);
|
||||
let auth_and_mcp_fut = async move {
|
||||
let auth = auth_manager_clone.auth().await;
|
||||
let mcp_config = mcp_manager_for_mcp
|
||||
@@ -693,17 +690,18 @@ impl Session {
|
||||
.await;
|
||||
let mcp_servers = codex_mcp::effective_mcp_servers(&mcp_config, auth.as_ref());
|
||||
let tool_plugin_provenance = codex_mcp::tool_plugin_provenance(&mcp_config);
|
||||
let auth_statuses = if start_mcp_clients {
|
||||
compute_auth_statuses(
|
||||
mcp_servers.iter(),
|
||||
config_for_mcp.mcp_oauth_credentials_store_mode,
|
||||
config_for_mcp.auth_keyring_backend_kind(),
|
||||
auth.as_ref(),
|
||||
&mcp_runtime_context_for_auth,
|
||||
)
|
||||
.await
|
||||
} else {
|
||||
Default::default()
|
||||
let auth_statuses = match mcp_access {
|
||||
McpAccess::Enabled => {
|
||||
compute_auth_statuses(
|
||||
mcp_servers.iter(),
|
||||
config_for_mcp.mcp_oauth_credentials_store_mode,
|
||||
config_for_mcp.auth_keyring_backend_kind(),
|
||||
auth.as_ref(),
|
||||
&mcp_runtime_context_for_auth,
|
||||
)
|
||||
.await
|
||||
}
|
||||
McpAccess::Disabled => Default::default(),
|
||||
};
|
||||
(
|
||||
auth,
|
||||
@@ -1201,8 +1199,8 @@ impl Session {
|
||||
*cancel_guard = cancel_token.clone();
|
||||
cancel_token
|
||||
};
|
||||
let mcp_connection_manager = if start_mcp_clients {
|
||||
McpConnectionManager::new(
|
||||
let mcp_connection_manager = match mcp_access {
|
||||
McpAccess::Enabled => McpConnectionManager::new(
|
||||
&mcp_servers,
|
||||
config.mcp_oauth_credentials_store_mode,
|
||||
config.auth_keyring_backend_kind(),
|
||||
@@ -1230,13 +1228,12 @@ impl Session {
|
||||
"session_init.mcp_manager_init",
|
||||
otel.name = "session_init.mcp_manager_init",
|
||||
))
|
||||
.await
|
||||
} else {
|
||||
McpConnectionManager::new_uninitialized_with_permission_profile(
|
||||
.await,
|
||||
McpAccess::Disabled => McpConnectionManager::new_uninitialized_with_permission_profile(
|
||||
&session_configuration.approval_policy,
|
||||
&session_configuration.permission_profile(),
|
||||
config.prefix_mcp_tool_names(),
|
||||
)
|
||||
),
|
||||
};
|
||||
sess.services
|
||||
.install_mcp_connection_manager(
|
||||
|
||||
@@ -7790,7 +7790,7 @@ async fn built_tools_uses_the_step_mcp_runtime() -> anyhow::Result<()> {
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn review_step_uses_isolated_mcp_runtime_without_publishing_it() {
|
||||
async fn review_step_uses_disabled_mcp_runtime_without_publishing_it() {
|
||||
let (mut session, mut turn_context) = make_session_and_context().await;
|
||||
session.services.selected_capability_roots = vec![SelectedCapabilityRoot {
|
||||
id: "review-root".to_string(),
|
||||
|
||||
@@ -1181,16 +1181,11 @@ pub(crate) async fn built_tools(
|
||||
) -> CodexResult<Arc<ToolRouter>> {
|
||||
let turn_context = step_context.turn.as_ref();
|
||||
let mcp_connection_manager = step_context.mcp.manager();
|
||||
let mcp_tools_enabled = !turn_context.is_review_subagent();
|
||||
let has_mcp_servers = mcp_tools_enabled && mcp_connection_manager.has_servers();
|
||||
let all_mcp_tools = if mcp_tools_enabled {
|
||||
mcp_connection_manager
|
||||
.list_all_tools()
|
||||
.or_cancel(cancellation_token)
|
||||
.await?
|
||||
} else {
|
||||
Vec::new()
|
||||
};
|
||||
let has_mcp_servers = mcp_connection_manager.has_servers();
|
||||
let all_mcp_tools = mcp_connection_manager
|
||||
.list_all_tools()
|
||||
.or_cancel(cancellation_token)
|
||||
.await?;
|
||||
let loaded_plugins = sess
|
||||
.services
|
||||
.plugins_manager
|
||||
|
||||
@@ -149,12 +149,36 @@ enum TurnMultiAgentRuntime {
|
||||
Preview,
|
||||
}
|
||||
|
||||
#[derive(Clone, Copy, Debug, PartialEq, Eq)]
|
||||
pub(crate) enum McpAccess {
|
||||
Enabled,
|
||||
Disabled,
|
||||
}
|
||||
|
||||
impl McpAccess {
|
||||
pub(crate) fn for_session_source(session_source: &SessionSource) -> Self {
|
||||
match session_source {
|
||||
SessionSource::SubAgent(SubAgentSource::Review) => Self::Disabled,
|
||||
SessionSource::Cli
|
||||
| SessionSource::VSCode
|
||||
| SessionSource::Exec
|
||||
| SessionSource::Mcp
|
||||
| SessionSource::Custom(_)
|
||||
| SessionSource::Internal(_)
|
||||
| SessionSource::Unknown
|
||||
| SessionSource::SubAgent(
|
||||
SubAgentSource::Compact
|
||||
| SubAgentSource::MemoryConsolidation
|
||||
| SubAgentSource::ThreadSpawn { .. }
|
||||
| SubAgentSource::Other(_),
|
||||
) => Self::Enabled,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
impl TurnContext {
|
||||
pub(crate) fn is_review_subagent(&self) -> bool {
|
||||
matches!(
|
||||
self.session_source,
|
||||
SessionSource::SubAgent(SubAgentSource::Review)
|
||||
)
|
||||
pub(crate) fn mcp_access(&self) -> McpAccess {
|
||||
McpAccess::for_session_source(&self.session_source)
|
||||
}
|
||||
|
||||
pub(crate) fn permission_profile(&self) -> PermissionProfile {
|
||||
@@ -207,9 +231,11 @@ impl TurnContext {
|
||||
.auth_manager
|
||||
.as_deref()
|
||||
.is_some_and(AuthManager::current_auth_uses_codex_backend);
|
||||
self.config
|
||||
.features
|
||||
.apps_enabled_for_auth(uses_codex_backend)
|
||||
self.mcp_access() == McpAccess::Enabled
|
||||
&& self
|
||||
.config
|
||||
.features
|
||||
.apps_enabled_for_auth(uses_codex_backend)
|
||||
&& self.config.orchestrator_mcp_enabled
|
||||
}
|
||||
|
||||
|
||||
@@ -1,6 +1,7 @@
|
||||
use crate::agent::exceeds_thread_spawn_depth_limit;
|
||||
use crate::agent::next_thread_spawn_depth;
|
||||
use crate::session::step_context::StepContext;
|
||||
use crate::session::turn_context::McpAccess;
|
||||
use crate::session::turn_context::TurnContext;
|
||||
use crate::tools::code_mode::execute_spec::create_code_mode_tool;
|
||||
use crate::tools::context::ToolInvocation;
|
||||
@@ -334,7 +335,7 @@ pub(crate) fn search_tool_enabled(turn_context: &TurnContext) -> bool {
|
||||
|
||||
pub(crate) fn tool_suggest_enabled(turn_context: &TurnContext) -> bool {
|
||||
let features = turn_context.config.features.get();
|
||||
!turn_context.is_review_subagent()
|
||||
turn_context.mcp_access() == McpAccess::Enabled
|
||||
&& features.enabled(Feature::ToolSuggest)
|
||||
&& features.enabled(Feature::Apps)
|
||||
&& features.enabled(Feature::Plugins)
|
||||
|
||||
@@ -1,14 +1,10 @@
|
||||
use codex_config::test_support::CloudConfigBundleFixture;
|
||||
use codex_config::types::McpServerConfig;
|
||||
use codex_config::types::McpServerTransportConfig;
|
||||
use codex_core::CodexThread;
|
||||
use codex_core::REVIEW_PROMPT;
|
||||
use codex_core::config::Config;
|
||||
use codex_core::review_format::render_review_output_text;
|
||||
use codex_features::Feature;
|
||||
use codex_login::CodexAuth;
|
||||
use codex_plugin::PluginHookSource;
|
||||
use codex_plugin::PluginId;
|
||||
use codex_protocol::models::ContentItem;
|
||||
use codex_protocol::models::PermissionProfile;
|
||||
use codex_protocol::models::ResponseItem;
|
||||
@@ -25,26 +21,22 @@ use codex_protocol::protocol::ReviewTarget;
|
||||
use codex_protocol::protocol::RolloutItem;
|
||||
use codex_protocol::protocol::RolloutLine;
|
||||
use codex_protocol::user_input::UserInput;
|
||||
use codex_utils_absolute_path::AbsolutePathBuf;
|
||||
use codex_utils_path_uri::PathUri;
|
||||
use core_test_support::PathBufExt;
|
||||
use core_test_support::apps_test_server::AppsTestServer;
|
||||
use core_test_support::hooks::trust_hooks;
|
||||
use core_test_support::apps_test_server::apps_enabled_builder;
|
||||
use core_test_support::responses;
|
||||
use core_test_support::responses::ResponseMock;
|
||||
use core_test_support::responses::mount_sse_sequence;
|
||||
use core_test_support::responses::start_mock_server;
|
||||
use core_test_support::skip_if_no_network;
|
||||
use core_test_support::stdio_server_bin;
|
||||
use core_test_support::test_codex::TestCodexBuilder;
|
||||
use core_test_support::test_codex::local_selections;
|
||||
use core_test_support::test_codex::test_codex;
|
||||
use core_test_support::wait_for_event;
|
||||
use core_test_support::wait_for_mcp_server;
|
||||
use pretty_assertions::assert_eq;
|
||||
use std::collections::HashMap;
|
||||
use std::path::PathBuf;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
use tempfile::TempDir;
|
||||
use tokio::io::AsyncWriteExt as _;
|
||||
use uuid::Uuid;
|
||||
@@ -271,72 +263,80 @@ async fn review_op_with_plain_text_emits_review_fallback() {
|
||||
server.verify().await;
|
||||
}
|
||||
|
||||
async fn submit_custom_review(codex: &CodexThread, instructions: &str) -> anyhow::Result<()> {
|
||||
codex
|
||||
.submit(Op::Review {
|
||||
review_request: ReviewRequest {
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: instructions.to_string(),
|
||||
},
|
||||
user_facing_hint: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
fn pinned_apps_builder(codex_home: Arc<TempDir>, apps_base_url: String) -> TestCodexBuilder {
|
||||
test_codex()
|
||||
.with_home(codex_home)
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"[features]
|
||||
apps = true
|
||||
"#,
|
||||
),
|
||||
)
|
||||
.with_config(move |config| config.chatgpt_base_url = apps_base_url)
|
||||
}
|
||||
|
||||
fn write_enabled_test_plugin(codex_home: &TempDir) -> anyhow::Result<PathBuf> {
|
||||
let plugin_root = codex_home.path().join("plugins/cache/test/sample/local");
|
||||
std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"sample"}"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
codex_home.path().join("config.toml"),
|
||||
r#"[features]
|
||||
plugins = true
|
||||
|
||||
[plugins."sample@test"]
|
||||
enabled = true
|
||||
"#,
|
||||
)?;
|
||||
Ok(plugin_root)
|
||||
}
|
||||
|
||||
#[tokio::test(flavor = "multi_thread", worker_threads = 2)]
|
||||
async fn review_does_not_expose_mcp_tools() -> anyhow::Result<()> {
|
||||
skip_if_no_network!(Ok(()));
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let apps_base_url = apps_server.chatgpt_base_url.clone();
|
||||
let rmcp_test_server_bin = stdio_server_bin()?;
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let codex = test_codex()
|
||||
.with_home(codex_home)
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Apps)
|
||||
.expect("test config should allow apps");
|
||||
config.chatgpt_base_url = apps_base_url;
|
||||
config
|
||||
.mcp_servers
|
||||
.set(HashMap::from([(
|
||||
"rmcp".to_string(),
|
||||
McpServerConfig {
|
||||
auth: Default::default(),
|
||||
transport: McpServerTransportConfig::Stdio {
|
||||
command: rmcp_test_server_bin,
|
||||
args: Vec::new(),
|
||||
env: None,
|
||||
env_vars: Vec::new(),
|
||||
cwd: None,
|
||||
},
|
||||
environment_id: codex_config::DEFAULT_MCP_SERVER_ENVIRONMENT_ID.to_string(),
|
||||
enabled: true,
|
||||
required: false,
|
||||
supports_parallel_tool_calls: false,
|
||||
disabled_reason: None,
|
||||
startup_timeout_sec: Some(Duration::from_secs(10)),
|
||||
tool_timeout_sec: None,
|
||||
default_tools_approval_mode: None,
|
||||
enabled_tools: None,
|
||||
disabled_tools: None,
|
||||
scopes: None,
|
||||
oauth: None,
|
||||
oauth_resource: None,
|
||||
tools: HashMap::new(),
|
||||
},
|
||||
)]))
|
||||
.expect("test MCP server config should be allowed");
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?
|
||||
.codex;
|
||||
wait_for_mcp_server(&codex, "rmcp").await?;
|
||||
let request_log = mount_sse_sequence(&server, vec![responses::sse(completed_sse())]).await;
|
||||
let mut builder =
|
||||
apps_enabled_builder(apps_server.chatgpt_base_url.clone()).with_home(codex_home);
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::McpStartupComplete(_))
|
||||
})
|
||||
.await;
|
||||
let request_log = mount_sse_sequence(
|
||||
&server,
|
||||
vec![
|
||||
responses::sse(completed_sse()),
|
||||
responses::sse(completed_sse()),
|
||||
],
|
||||
)
|
||||
.await;
|
||||
|
||||
codex
|
||||
.submit(Op::Review {
|
||||
review_request: ReviewRequest {
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: "Review without MCP tools".to_string(),
|
||||
},
|
||||
user_facing_hint: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
let entered = wait_for_event(&codex, |event| {
|
||||
test.submit_turn("Inspect available Apps tools").await?;
|
||||
|
||||
submit_custom_review(&test.codex, "Review without MCP tools").await?;
|
||||
let entered = wait_for_event(&test.codex, |event| {
|
||||
matches!(
|
||||
event,
|
||||
EventMsg::EnteredReviewMode(_) | EventMsg::Error(_) | EventMsg::TurnComplete(_)
|
||||
@@ -347,20 +347,16 @@ async fn review_does_not_expose_mcp_tools() -> anyhow::Result<()> {
|
||||
matches!(entered, EventMsg::EnteredReviewMode(_)),
|
||||
"review did not start: {entered:?}"
|
||||
);
|
||||
wait_for_event(&codex, |event| matches!(event, EventMsg::TurnComplete(_))).await;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
.await;
|
||||
|
||||
let request = request_log.single_request();
|
||||
assert!(
|
||||
!request.body_contains_text("mcp__codex_apps"),
|
||||
"review request unexpectedly exposed MCP tools: {:?}",
|
||||
request.body_json()["tools"]
|
||||
);
|
||||
assert!(
|
||||
!request.body_contains_text("mcp__rmcp"),
|
||||
"review request unexpectedly exposed configured MCP tools: {:?}",
|
||||
request.body_json()["tools"]
|
||||
);
|
||||
let requests = request_log.requests();
|
||||
assert!(requests[0].body_contains_text("list_mcp_resources"));
|
||||
let request = &requests[1];
|
||||
for tool_name in [
|
||||
"mcp__codex_apps",
|
||||
"list_mcp_resources",
|
||||
"list_mcp_resource_templates",
|
||||
"read_mcp_resource",
|
||||
@@ -405,20 +401,9 @@ async fn review_preserves_plugin_pre_tool_use_hooks() -> anyhow::Result<()> {
|
||||
.await;
|
||||
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let plugin_root = codex_home.path().join("plugins/cache/test/sample/local");
|
||||
let plugin_root = write_enabled_test_plugin(&codex_home)?;
|
||||
let hooks_dir = plugin_root.join("hooks");
|
||||
std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?;
|
||||
std::fs::create_dir_all(&hooks_dir)?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"sample"}"#,
|
||||
)?;
|
||||
std::fs::write(
|
||||
codex_home.path().join("config.toml"),
|
||||
r#"[plugins."sample@test"]
|
||||
enabled = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let hook_script = hooks_dir.join("pre_tool_use_hook.py");
|
||||
std::fs::write(
|
||||
@@ -447,53 +432,25 @@ print(json.dumps({
|
||||
}]
|
||||
}
|
||||
}"#;
|
||||
let plugin_hooks_path = hooks_dir.join("hooks.json");
|
||||
std::fs::write(&plugin_hooks_path, plugin_hooks_json)?;
|
||||
let plugin_hook_sources = vec![PluginHookSource {
|
||||
plugin_id: PluginId::parse("sample@test")?,
|
||||
plugin_root: AbsolutePathBuf::try_from(plugin_root.clone())?,
|
||||
plugin_data_root: AbsolutePathBuf::try_from(plugin_root.join("data"))?,
|
||||
source_path: AbsolutePathBuf::try_from(plugin_hooks_path)?,
|
||||
source_relative_path: "hooks/hooks.json".to_string(),
|
||||
hooks: serde_json::from_str::<codex_config::HooksFile>(plugin_hooks_json)?.hooks,
|
||||
}];
|
||||
std::fs::write(hooks_dir.join("hooks.json"), plugin_hooks_json)?;
|
||||
|
||||
let test = test_codex()
|
||||
.with_home(Arc::clone(&codex_home))
|
||||
.with_config(move |config| {
|
||||
config
|
||||
.features
|
||||
.enable(Feature::Plugins)
|
||||
.expect("test config should allow plugins");
|
||||
config
|
||||
.features
|
||||
.enable(Feature::CodexHooks)
|
||||
.expect("test config should allow hooks");
|
||||
config.bypass_hook_trust = true;
|
||||
config
|
||||
.permissions
|
||||
.set_permission_profile(PermissionProfile::Disabled)
|
||||
.expect("test config should allow disabling the sandbox");
|
||||
let listed = codex_hooks::list_hooks(codex_hooks::HooksConfig {
|
||||
feature_enabled: true,
|
||||
config_layer_stack: Some(config.config_layer_stack.clone()),
|
||||
plugin_hook_sources,
|
||||
..codex_hooks::HooksConfig::default()
|
||||
});
|
||||
trust_hooks(config, listed.hooks);
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
|
||||
test.codex
|
||||
.submit(Op::Review {
|
||||
review_request: ReviewRequest {
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: "Apply the patch while reviewing".to_string(),
|
||||
},
|
||||
user_facing_hint: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
submit_custom_review(&test.codex, "Apply the patch while reviewing").await?;
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
})
|
||||
@@ -527,23 +484,9 @@ async fn review_does_not_start_mcp_when_apps_feature_is_pinned_on() -> anyhow::R
|
||||
|
||||
let server = start_mock_server().await;
|
||||
let apps_server = AppsTestServer::mount(&server).await?;
|
||||
let apps_base_url = apps_server.chatgpt_base_url.clone();
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let test = test_codex()
|
||||
.with_home(codex_home)
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"[features]
|
||||
apps = true
|
||||
"#,
|
||||
),
|
||||
)
|
||||
.with_config(move |config| {
|
||||
config.chatgpt_base_url = apps_base_url;
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
let mut builder = pinned_apps_builder(codex_home, apps_server.chatgpt_base_url.clone());
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
let mut pinned_features = test.config.features.clone();
|
||||
pinned_features
|
||||
@@ -560,16 +503,7 @@ apps = true
|
||||
vec![responses::sse(vec![responses::ev_completed("resp-1")])],
|
||||
)
|
||||
.await;
|
||||
test.codex
|
||||
.submit(Op::Review {
|
||||
review_request: ReviewRequest {
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: "Review without starting pinned Apps MCP".to_string(),
|
||||
},
|
||||
user_facing_hint: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
submit_custom_review(&test.codex, "Review without starting pinned Apps MCP").await?;
|
||||
|
||||
wait_for_event(&test.codex, |event| match event {
|
||||
EventMsg::McpStartupUpdate(update) => {
|
||||
@@ -608,18 +542,8 @@ async fn review_does_not_suggest_tools_when_apps_feature_is_pinned_on() -> anyho
|
||||
})))
|
||||
.mount(&server)
|
||||
.await;
|
||||
let apps_base_url = apps_server.chatgpt_base_url.clone();
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let test = test_codex()
|
||||
.with_home(codex_home)
|
||||
.with_auth(CodexAuth::create_dummy_chatgpt_auth_for_testing())
|
||||
.with_cloud_config_bundle(
|
||||
CloudConfigBundleFixture::loader_with_enterprise_requirement(
|
||||
r#"[features]
|
||||
apps = true
|
||||
"#,
|
||||
),
|
||||
)
|
||||
let mut builder = pinned_apps_builder(codex_home, apps_server.chatgpt_base_url.clone())
|
||||
.with_config(move |config| {
|
||||
for feature in [
|
||||
Feature::Plugins,
|
||||
@@ -631,10 +555,8 @@ apps = true
|
||||
.enable(feature)
|
||||
.expect("test config should allow feature update");
|
||||
}
|
||||
config.chatgpt_base_url = apps_base_url;
|
||||
})
|
||||
.build_with_auto_env(&server)
|
||||
.await?;
|
||||
});
|
||||
let test = builder.build_with_auto_env(&server).await?;
|
||||
|
||||
let suggested_requests_before_review = server
|
||||
.received_requests()
|
||||
@@ -648,16 +570,7 @@ apps = true
|
||||
vec![responses::sse(vec![responses::ev_completed("resp-1")])],
|
||||
)
|
||||
.await;
|
||||
test.codex
|
||||
.submit(Op::Review {
|
||||
review_request: ReviewRequest {
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: "Review without tool suggestions".to_string(),
|
||||
},
|
||||
user_facing_hint: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
submit_custom_review(&test.codex, "Review without tool suggestions").await?;
|
||||
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
@@ -691,12 +604,7 @@ async fn review_does_not_discover_auth_for_plugin_mcp_servers() -> anyhow::Resul
|
||||
let server = start_mock_server().await;
|
||||
let plugin_mcp_server = MockServer::start().await;
|
||||
let codex_home = Arc::new(TempDir::new()?);
|
||||
let plugin_root = codex_home.path().join("plugins/cache/test/sample/local");
|
||||
std::fs::create_dir_all(plugin_root.join(".codex-plugin"))?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".codex-plugin/plugin.json"),
|
||||
r#"{"name":"sample"}"#,
|
||||
)?;
|
||||
let plugin_root = write_enabled_test_plugin(&codex_home)?;
|
||||
std::fs::write(
|
||||
plugin_root.join(".mcp.json"),
|
||||
format!(
|
||||
@@ -711,15 +619,6 @@ async fn review_does_not_discover_auth_for_plugin_mcp_servers() -> anyhow::Resul
|
||||
plugin_mcp_server.uri()
|
||||
),
|
||||
)?;
|
||||
std::fs::write(
|
||||
codex_home.path().join("config.toml"),
|
||||
r#"[features]
|
||||
plugins = true
|
||||
|
||||
[plugins."sample@test"]
|
||||
enabled = true
|
||||
"#,
|
||||
)?;
|
||||
|
||||
let test = test_codex()
|
||||
.with_home(codex_home)
|
||||
@@ -744,16 +643,7 @@ enabled = true
|
||||
vec![responses::sse(vec![responses::ev_completed("resp-1")])],
|
||||
)
|
||||
.await;
|
||||
test.codex
|
||||
.submit(Op::Review {
|
||||
review_request: ReviewRequest {
|
||||
target: ReviewTarget::Custom {
|
||||
instructions: "Review without MCP auth discovery".to_string(),
|
||||
},
|
||||
user_facing_hint: None,
|
||||
},
|
||||
})
|
||||
.await?;
|
||||
submit_custom_review(&test.codex, "Review without MCP auth discovery").await?;
|
||||
|
||||
wait_for_event(&test.codex, |event| {
|
||||
matches!(event, EventMsg::TurnComplete(_))
|
||||
|
||||
Reference in New Issue
Block a user