mirror of
https://github.com/pchuan98/codex.git
synced 2026-07-01 00:31:56 +08:00
test: harden app-server integration tests (#19683)
## Why Windows Bazel runs in the permissions stack exposed that app-server integration tests were launching normal plugin startup warmups in every subprocess. Those warmups can call `https://chatgpt.com/backend-api/plugins/featured` when a test is not specifically exercising plugin startup, which adds slow background work, noisy stderr, and dependence on external network state. The relevant startup/featured-plugin behavior was introduced across #15042 and #15264. A few app-server tests also had long optional waits or unbounded cleanup paths, making failures expensive to diagnose and contributing to slow Windows shards. One external-agent config test from #18246 used a GitHub-style marketplace source, which was enough to exercise the pending remote-import path but also meant the background completion task could attempt a real clone. ## What Changed - Adds explicit `AppServerRuntimeOptions` / `PluginStartupTasks` plumbing and a hidden debug-only `--disable-plugin-startup-tasks-for-tests` app-server flag, so integration tests can suppress startup plugin warmups without adding a production env-var gate. - Has the app-server test harness pass that hidden flag by default, while opting plugin-startup coverage back in for tests that intentionally exercise startup sync and featured-plugin warmup behavior. - Lowers normal app-server subprocess logging from `info`/`debug` to `warn` to avoid multi-megabyte stderr output in Bazel logs. - Prevents the external-agent config test from attempting a real marketplace clone by using an invalid non-local source while still exercising the pending-import completion path. - Bounds optional filesystem/realtime waits and fake WebSocket test-server shutdown so failures produce targeted timeouts instead of hanging a shard. - Fixes the Unix script-resolution test in `rmcp-client` to exercise PATH resolution directly and include the actual spawn error in failures. ## Verification - `cargo check -p codex-app-server` - `cargo clippy -p codex-app-server --tests -- -D warnings` - `cargo test -p codex-rmcp-client program_resolver::tests::test_unix_executes_script_without_extension` - `cargo test -p codex-app-server --test all external_agent_config_import_sends_completion_notification_after_pending_plugins_finish -- --nocapture` - `cargo test -p codex-app-server --test all plugin_list_uses_warmed_featured_plugin_ids_cache_on_first_request -- --nocapture` - Windows Local Bazel passed with this test-hardening bundle before it was extracted from #19606. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/19683). * #19395 * #19394 * #19393 * #19392 * #19606 * __->__ #19683
This commit is contained in:
@@ -415,6 +415,7 @@ fn start_uninitialized(args: InProcessStartArgs) -> InProcessClientHandle {
|
||||
auth_manager,
|
||||
rpc_transport: AppServerRpcTransport::InProcess,
|
||||
remote_control_handle: None,
|
||||
plugin_startup_tasks: crate::PluginStartupTasks::Start,
|
||||
}));
|
||||
let mut thread_created_rx = processor.thread_created_receiver();
|
||||
let session = Arc::new(ConnectionSessionState::new(ConnectionOrigin::InProcess));
|
||||
|
||||
@@ -362,6 +362,25 @@ pub async fn run_main(
|
||||
.await
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub enum PluginStartupTasks {
|
||||
Start,
|
||||
Skip,
|
||||
}
|
||||
|
||||
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
|
||||
pub struct AppServerRuntimeOptions {
|
||||
pub plugin_startup_tasks: PluginStartupTasks,
|
||||
}
|
||||
|
||||
impl Default for AppServerRuntimeOptions {
|
||||
fn default() -> Self {
|
||||
Self {
|
||||
plugin_startup_tasks: PluginStartupTasks::Start,
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
pub async fn run_main_with_transport(
|
||||
arg0_paths: Arg0DispatchPaths,
|
||||
cli_config_overrides: CliConfigOverrides,
|
||||
@@ -370,6 +389,30 @@ pub async fn run_main_with_transport(
|
||||
transport: AppServerTransport,
|
||||
session_source: SessionSource,
|
||||
auth: AppServerWebsocketAuthSettings,
|
||||
) -> IoResult<()> {
|
||||
run_main_with_transport_options(
|
||||
arg0_paths,
|
||||
cli_config_overrides,
|
||||
loader_overrides,
|
||||
default_analytics_enabled,
|
||||
transport,
|
||||
session_source,
|
||||
auth,
|
||||
AppServerRuntimeOptions::default(),
|
||||
)
|
||||
.await
|
||||
}
|
||||
|
||||
#[allow(clippy::too_many_arguments)]
|
||||
pub async fn run_main_with_transport_options(
|
||||
arg0_paths: Arg0DispatchPaths,
|
||||
cli_config_overrides: CliConfigOverrides,
|
||||
loader_overrides: LoaderOverrides,
|
||||
default_analytics_enabled: bool,
|
||||
transport: AppServerTransport,
|
||||
session_source: SessionSource,
|
||||
auth: AppServerWebsocketAuthSettings,
|
||||
runtime_options: AppServerRuntimeOptions,
|
||||
) -> IoResult<()> {
|
||||
let environment_manager = Arc::new(EnvironmentManager::new(EnvironmentManagerArgs::from_env(
|
||||
ExecServerRuntimePaths::from_optional_paths(
|
||||
@@ -683,6 +726,7 @@ pub async fn run_main_with_transport(
|
||||
auth_manager,
|
||||
rpc_transport: analytics_rpc_transport(&transport),
|
||||
remote_control_handle: Some(remote_control_handle),
|
||||
plugin_startup_tasks: runtime_options.plugin_startup_tasks,
|
||||
}));
|
||||
let mut thread_created_rx = processor.thread_created_receiver();
|
||||
let mut running_turn_count_rx = processor.subscribe_running_assistant_turn_count();
|
||||
|
||||
@@ -1,7 +1,9 @@
|
||||
use clap::Parser;
|
||||
use codex_app_server::AppServerRuntimeOptions;
|
||||
use codex_app_server::AppServerTransport;
|
||||
use codex_app_server::AppServerWebsocketAuthArgs;
|
||||
use codex_app_server::run_main_with_transport;
|
||||
use codex_app_server::PluginStartupTasks;
|
||||
use codex_app_server::run_main_with_transport_options;
|
||||
use codex_arg0::Arg0DispatchPaths;
|
||||
use codex_arg0::arg0_dispatch_or_else;
|
||||
use codex_core::config_loader::LoaderOverrides;
|
||||
@@ -36,6 +38,12 @@ struct AppServerArgs {
|
||||
|
||||
#[command(flatten)]
|
||||
auth: AppServerWebsocketAuthArgs,
|
||||
|
||||
/// Hidden debug-only test hook used by integration tests that spawn the
|
||||
/// production app-server binary.
|
||||
#[cfg(debug_assertions)]
|
||||
#[arg(long = "disable-plugin-startup-tasks-for-tests", hide = true)]
|
||||
disable_plugin_startup_tasks_for_tests: bool,
|
||||
}
|
||||
|
||||
fn main() -> anyhow::Result<()> {
|
||||
@@ -51,8 +59,13 @@ fn main() -> anyhow::Result<()> {
|
||||
let transport = args.listen;
|
||||
let session_source = args.session_source;
|
||||
let auth = args.auth.try_into_settings()?;
|
||||
let mut runtime_options = AppServerRuntimeOptions::default();
|
||||
#[cfg(debug_assertions)]
|
||||
if args.disable_plugin_startup_tasks_for_tests {
|
||||
runtime_options.plugin_startup_tasks = PluginStartupTasks::Skip;
|
||||
}
|
||||
|
||||
run_main_with_transport(
|
||||
run_main_with_transport_options(
|
||||
arg0_paths,
|
||||
CliConfigOverrides::default(),
|
||||
loader_overrides,
|
||||
@@ -60,6 +73,7 @@ fn main() -> anyhow::Result<()> {
|
||||
transport,
|
||||
session_source,
|
||||
auth,
|
||||
runtime_options,
|
||||
)
|
||||
.await?;
|
||||
Ok(())
|
||||
|
||||
@@ -95,7 +95,6 @@ use tokio::time::timeout;
|
||||
use tracing::Instrument;
|
||||
|
||||
const EXTERNAL_AUTH_REFRESH_TIMEOUT: Duration = Duration::from_secs(10);
|
||||
|
||||
#[derive(Clone)]
|
||||
struct ExternalAuthRefreshBridge {
|
||||
outgoing: Arc<OutgoingMessageSender>,
|
||||
@@ -260,6 +259,7 @@ pub(crate) struct MessageProcessorArgs {
|
||||
pub(crate) auth_manager: Arc<AuthManager>,
|
||||
pub(crate) rpc_transport: AppServerRpcTransport,
|
||||
pub(crate) remote_control_handle: Option<RemoteControlHandle>,
|
||||
pub(crate) plugin_startup_tasks: crate::PluginStartupTasks,
|
||||
}
|
||||
|
||||
impl MessageProcessor {
|
||||
@@ -279,6 +279,7 @@ impl MessageProcessor {
|
||||
auth_manager,
|
||||
rpc_transport,
|
||||
remote_control_handle,
|
||||
plugin_startup_tasks,
|
||||
} = args;
|
||||
auth_manager.set_external_auth(Arc::new(ExternalAuthRefreshBridge {
|
||||
outgoing: outgoing.clone(),
|
||||
@@ -315,11 +316,13 @@ impl MessageProcessor {
|
||||
feedback,
|
||||
log_db,
|
||||
});
|
||||
// Keep plugin startup warmups aligned at app-server startup.
|
||||
// TODO(xl): Move into PluginManager once this no longer depends on config feature gating.
|
||||
thread_manager
|
||||
.plugins_manager()
|
||||
.maybe_start_plugin_startup_tasks_for_config(&config, auth_manager.clone());
|
||||
if matches!(plugin_startup_tasks, crate::PluginStartupTasks::Start) {
|
||||
// Keep plugin startup warmups aligned at app-server startup.
|
||||
// TODO(xl): Move into PluginManager once this no longer depends on config feature gating.
|
||||
thread_manager
|
||||
.plugins_manager()
|
||||
.maybe_start_plugin_startup_tasks_for_config(&config, auth_manager.clone());
|
||||
}
|
||||
let config_api = ConfigApi::new(
|
||||
config_manager,
|
||||
thread_manager.clone(),
|
||||
|
||||
@@ -288,6 +288,7 @@ fn build_test_processor(
|
||||
auth_manager,
|
||||
rpc_transport: AppServerRpcTransport::Stdio,
|
||||
remote_control_handle: None,
|
||||
plugin_startup_tasks: crate::PluginStartupTasks::Start,
|
||||
}));
|
||||
(processor, outgoing_rx)
|
||||
}
|
||||
|
||||
Reference in New Issue
Block a user