From d017d0fc3561a7b1d43c2723e7f46cadf5c24305 Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim <219906144+aibrahim-oai@users.noreply.github.com> Date: Sat, 14 Mar 2026 08:51:10 +0000 Subject: [PATCH] Stabilize Windows shell and fuzzy search tests Co-authored-by: Codex --- .codex/flaky-test-triage.md | 14 +- .../tests/suite/fuzzy_file_search.rs | 336 +++++++++++++++++- codex-rs/core/src/codex_tests.rs | 19 +- 3 files changed, 360 insertions(+), 9 deletions(-) diff --git a/.codex/flaky-test-triage.md b/.codex/flaky-test-triage.md index d7838f536a..e09a82c71b 100644 --- a/.codex/flaky-test-triage.md +++ b/.codex/flaky-test-triage.md @@ -46,12 +46,13 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron ## Current Fix In Progress -- Make the generic permissions-popup helper recognize `Read Only`, which is a real preset on Windows. -- Replace Windows-specific hard-coded navigation counts with label-driven movement in the permission history snapshot tests. -- Replace the remaining Windows-only extra navigation in the full-access confirmation/history test, and use the same label-driven movement in the adjacent Smart Approvals popup tests. -- Follow-up on `87941e5d7`: the helper compared the entire selected row with `contains(label)`, which made `Smart Approvals` look like `Default` because its description text says "Same workspace-write permissions as Default...". Tighten the helper to match the selected preset name at the start of the row instead of any substring in the rendered line. -- Follow-up on `13c9d91b0`: the cross-platform helper bug is fixed, but the default unelevated Windows popup still opens on `Read Only` instead of `Default`. The remaining Windows-only failures are in tests that added exact initial-selection assertions while assuming the popup would start on `Default`. -- Rationale: commit `e96d895c9` still failed on both Windows `Tests` jobs in run `23080608881`. The remaining common pattern is Windows-only popup navigation that assumes a fixed number of `Up`/`Down` presses even though the Windows menu includes `Read Only` and selection wraps. +- Commit `495ef3f76` extended `rust-ci` annotations enough to expose the exact flaky tests on the two remaining Windows jobs. +- Windows x64 failure: `codex::tests::run_user_shell_command_does_not_set_reference_context_item` timed out waiting for `TurnComplete`. +- Windows arm64 failure: `all::suite::fuzzy_file_search::test_fuzzy_file_search_session_multiple_query_updates_work` timed out during app-server `initialize`, before the fuzzy-search session logic started. +- Current patch set: + - Pin the standalone shell test to `cmd.exe` on Windows so it validates reference-context isolation without depending on PowerShell startup behavior. + - Replace the fuzzy-file-search suite's spawned `codex-app-server` harness with the in-process app-server runtime so the tests still exercise request/notification behavior without the flaky stdio startup path. +- Rationale: both failures are test-harness flakes, not product behaviors. The fixes keep the assertions intact and remove environment-sensitive startup paths instead of stretching timeouts. ## Constraints @@ -79,3 +80,4 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron | `9835ec89d` | Record validation pass 1 | full pass | Full PR CI passed on run `23082212165`, including the rerun of `Tests — windows-arm64 - aarch64-pc-windows-msvc` on job `67054619213`. The branch now has 2 consecutive full-suite green commits after `c3b8a0ebf`. | | `b6e18d2e8` | Record validation pass 2 | failed | Run `23083106021` isolated a new failure in `Tests — ubuntu-24.04 - x86_64-unknown-linux-gnu` while both Windows `Tests` jobs and all Bazel/lint checks passed. The GitHub CLI log download hit EOFs on both `results-receiver.actions.githubusercontent.com` and the signed Azure blob URL again, so the follow-up commit adds CI annotations and step-summary output for parsed nextest `FAIL`/`LEAK` lines. | | `761363008` | Annotate nextest failures in CI | failed | Run `23083522224` flipped to a new `Tests — windows-x64 - x86_64-pc-windows-msvc` failure while `Tests — windows-arm64 - aarch64-pc-windows-msvc` and every non-test job passed. The new annotations proved this failure did not emit a parsable nextest `FAIL`/`LEAK` line, so the next follow-up extends the annotations to the last 80 log lines and requests explicit final failure output from nextest. | +| `495ef3f76` | Expose nextest failure tail in CI | failed | Run `23083992418` failed in both Windows `Tests` jobs. The new annotations identified `codex::tests::run_user_shell_command_does_not_set_reference_context_item` timing out on Windows x64 and `all::suite::fuzzy_file_search::test_fuzzy_file_search_session_multiple_query_updates_work` timing out during app-server `initialize` on Windows arm64. | diff --git a/codex-rs/app-server/tests/suite/fuzzy_file_search.rs b/codex-rs/app-server/tests/suite/fuzzy_file_search.rs index 7341a5a5f7..137d308f9d 100644 --- a/codex-rs/app-server/tests/suite/fuzzy_file_search.rs +++ b/codex-rs/app-server/tests/suite/fuzzy_file_search.rs @@ -1,17 +1,44 @@ use anyhow::Result; use anyhow::anyhow; -use app_test_support::McpProcess; +use codex_app_server::in_process::InProcessClientHandle; +use codex_app_server::in_process::InProcessServerEvent; +use codex_app_server::in_process::InProcessStartArgs; +use codex_app_server::in_process::start as start_in_process; +use codex_app_server_protocol::ClientInfo; +use codex_app_server_protocol::ClientRequest; +use codex_app_server_protocol::FuzzyFileSearchParams; use codex_app_server_protocol::FuzzyFileSearchSessionCompletedNotification; +use codex_app_server_protocol::FuzzyFileSearchSessionStartParams; +use codex_app_server_protocol::FuzzyFileSearchSessionStopParams; +use codex_app_server_protocol::FuzzyFileSearchSessionUpdateParams; use codex_app_server_protocol::FuzzyFileSearchSessionUpdatedNotification; +use codex_app_server_protocol::InitializeCapabilities; +use codex_app_server_protocol::InitializeParams; +use codex_app_server_protocol::JSONRPCError; +use codex_app_server_protocol::JSONRPCErrorError; +use codex_app_server_protocol::JSONRPCNotification; use codex_app_server_protocol::JSONRPCResponse; use codex_app_server_protocol::RequestId; +use codex_app_server_protocol::ServerNotification; +use codex_arg0::Arg0DispatchPaths; +use codex_core::config::ConfigBuilder; +use codex_core::config_loader::CloudRequirementsLoader; +use codex_core::config_loader::LoaderOverrides; +use codex_feedback::CodexFeedback; +use codex_protocol::protocol::SessionSource; use pretty_assertions::assert_eq; use serde_json::json; +use std::collections::HashMap; use std::path::Path; +use std::sync::Arc; use tempfile::TempDir; +use tokio::task::JoinHandle; use tokio::time::timeout; -const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(10); +type RawRequestResult = std::result::Result; +type PendingRequest = JoinHandle>; + +const DEFAULT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_secs(5); const SHORT_READ_TIMEOUT: std::time::Duration = std::time::Duration::from_millis(500); const STOP_GRACE_PERIOD: std::time::Duration = std::time::Duration::from_millis(250); const SESSION_UPDATED_METHOD: &str = "fuzzyFileSearch/sessionUpdated"; @@ -39,6 +66,311 @@ shell_snapshot = false ) } +struct McpProcess { + client: Option, + next_request_id: i64, + pending_requests: HashMap, + start_args: Option, +} + +impl McpProcess { + async fn new(codex_home: &Path) -> Result { + let config = Arc::new( + ConfigBuilder::default() + .codex_home(codex_home.to_path_buf()) + .fallback_cwd(Some(codex_home.to_path_buf())) + .build() + .await?, + ); + + Ok(Self { + client: None, + next_request_id: 1, + pending_requests: HashMap::new(), + start_args: Some(InProcessStartArgs { + arg0_paths: Arg0DispatchPaths::default(), + config, + cli_overrides: Vec::new(), + loader_overrides: LoaderOverrides::default(), + cloud_requirements: CloudRequirementsLoader::default(), + auth_manager: None, + thread_manager: None, + feedback: CodexFeedback::new(), + config_warnings: Vec::new(), + session_source: SessionSource::Cli, + enable_codex_api_key_env: false, + initialize: InitializeParams { + client_info: ClientInfo { + name: "codex-app-server-tests".to_string(), + title: None, + version: "0.1.0".to_string(), + }, + capabilities: Some(InitializeCapabilities { + experimental_api: true, + ..Default::default() + }), + }, + channel_capacity: 64, + }), + }) + } + + async fn initialize(&mut self) -> Result<()> { + if self.client.is_some() { + return Ok(()); + } + + let start_args = self + .start_args + .take() + .ok_or_else(|| anyhow!("MCP already initialized"))?; + self.client = Some(start_in_process(start_args).await?); + Ok(()) + } + + async fn send_fuzzy_file_search_request( + &mut self, + query: &str, + roots: Vec, + cancellation_token: Option, + ) -> Result { + let request_id = self.next_request_id(); + self.enqueue_request( + request_id, + ClientRequest::FuzzyFileSearch { + request_id: RequestId::Integer(request_id), + params: FuzzyFileSearchParams { + query: query.to_string(), + roots, + cancellation_token, + }, + }, + )?; + Ok(request_id) + } + + async fn send_fuzzy_file_search_session_start_request( + &mut self, + session_id: &str, + roots: Vec, + ) -> Result { + let request_id = self.next_request_id(); + self.enqueue_request( + request_id, + ClientRequest::FuzzyFileSearchSessionStart { + request_id: RequestId::Integer(request_id), + params: FuzzyFileSearchSessionStartParams { + session_id: session_id.to_string(), + roots, + }, + }, + )?; + Ok(request_id) + } + + async fn start_fuzzy_file_search_session( + &mut self, + session_id: &str, + roots: Vec, + ) -> Result { + let request_id = self + .send_fuzzy_file_search_session_start_request(session_id, roots) + .await?; + self.read_stream_until_response_message(RequestId::Integer(request_id)) + .await + } + + async fn send_fuzzy_file_search_session_update_request( + &mut self, + session_id: &str, + query: &str, + ) -> Result { + let request_id = self.next_request_id(); + self.enqueue_request( + request_id, + ClientRequest::FuzzyFileSearchSessionUpdate { + request_id: RequestId::Integer(request_id), + params: FuzzyFileSearchSessionUpdateParams { + session_id: session_id.to_string(), + query: query.to_string(), + }, + }, + )?; + Ok(request_id) + } + + async fn update_fuzzy_file_search_session( + &mut self, + session_id: &str, + query: &str, + ) -> Result { + let request_id = self + .send_fuzzy_file_search_session_update_request(session_id, query) + .await?; + self.read_stream_until_response_message(RequestId::Integer(request_id)) + .await + } + + async fn send_fuzzy_file_search_session_stop_request( + &mut self, + session_id: &str, + ) -> Result { + let request_id = self.next_request_id(); + self.enqueue_request( + request_id, + ClientRequest::FuzzyFileSearchSessionStop { + request_id: RequestId::Integer(request_id), + params: FuzzyFileSearchSessionStopParams { + session_id: session_id.to_string(), + }, + }, + )?; + Ok(request_id) + } + + async fn stop_fuzzy_file_search_session( + &mut self, + session_id: &str, + ) -> Result { + let request_id = self + .send_fuzzy_file_search_session_stop_request(session_id) + .await?; + self.read_stream_until_response_message(RequestId::Integer(request_id)) + .await + } + + async fn read_stream_until_response_message( + &mut self, + request_id: RequestId, + ) -> Result { + let pending = self.take_pending_request(&request_id)?; + match pending + .await + .map_err(|err| anyhow!("request task failed for {request_id:?}: {err}"))?? + { + Ok(result) => Ok(JSONRPCResponse { + id: request_id, + result, + }), + Err(error) => { + anyhow::bail!("expected response for {request_id:?}, got error: {error:?}") + } + } + } + + async fn read_stream_until_error_message( + &mut self, + request_id: RequestId, + ) -> Result { + let pending = self.take_pending_request(&request_id)?; + match pending + .await + .map_err(|err| anyhow!("request task failed for {request_id:?}: {err}"))?? + { + Ok(result) => { + anyhow::bail!("expected error for {request_id:?}, got response: {result:?}") + } + Err(error) => Ok(JSONRPCError { + id: request_id, + error, + }), + } + } + + async fn read_stream_until_notification_message( + &mut self, + method: &str, + ) -> Result { + loop { + let event = self + .client_mut()? + .next_event() + .await + .ok_or_else(|| anyhow!("app-server closed before emitting {method}"))?; + + let notification = match event { + InProcessServerEvent::ServerNotification(notification) => { + jsonrpc_notification_from_server(notification)? + } + InProcessServerEvent::LegacyNotification(notification) => notification, + InProcessServerEvent::Lagged { skipped } => { + anyhow::bail!("missed {skipped} app-server events while waiting for {method}") + } + InProcessServerEvent::ServerRequest(request) => { + anyhow::bail!( + "unexpected server request while waiting for {method}: {request:?}" + ) + } + }; + + if notification.method == method { + return Ok(notification); + } + } + } + + fn client_mut(&mut self) -> Result<&mut InProcessClientHandle> { + self.client + .as_mut() + .ok_or_else(|| anyhow!("MCP client not initialized")) + } + + fn enqueue_request(&mut self, request_id: i64, request: ClientRequest) -> Result<()> { + let sender = self + .client + .as_ref() + .ok_or_else(|| anyhow!("MCP client not initialized"))? + .sender(); + if self + .pending_requests + .insert( + request_id, + tokio::spawn(async move { sender.request(request).await }), + ) + .is_some() + { + anyhow::bail!("duplicate pending request id: {request_id}"); + } + Ok(()) + } + + fn next_request_id(&mut self) -> i64 { + let request_id = self.next_request_id; + self.next_request_id += 1; + request_id + } + + fn take_pending_request(&mut self, request_id: &RequestId) -> Result { + let RequestId::Integer(request_id) = request_id else { + anyhow::bail!("unsupported request id: {request_id:?}"); + }; + self.pending_requests + .remove(request_id) + .ok_or_else(|| anyhow!("missing pending request for id {request_id}")) + } +} + +impl Drop for McpProcess { + fn drop(&mut self) { + for (_, pending_request) in self.pending_requests.drain() { + pending_request.abort(); + } + if let Some(client) = self.client.take() + && let Ok(runtime) = tokio::runtime::Handle::try_current() + { + runtime.spawn(async move { + let _ = client.shutdown().await; + }); + } + } +} + +fn jsonrpc_notification_from_server( + notification: ServerNotification, +) -> Result { + Ok(serde_json::from_value(serde_json::to_value(notification)?)?) +} + async fn initialized_mcp(codex_home: &TempDir) -> Result { create_config_toml(codex_home.path())?; let mut mcp = McpProcess::new(codex_home.path()).await?; diff --git a/codex-rs/core/src/codex_tests.rs b/codex-rs/core/src/codex_tests.rs index fa948a782c..6494ce9aa4 100644 --- a/codex-rs/core/src/codex_tests.rs +++ b/codex-rs/core/src/codex_tests.rs @@ -11,7 +11,10 @@ use crate::exec::ExecToolCallOutput; use crate::function_tool::FunctionCallError; use crate::mcp_connection_manager::ToolInfo; use crate::models_manager::model_info; +use crate::shell::Shell; +use crate::shell::ShellType; use crate::shell::default_user_shell; +use crate::shell::empty_shell_snapshot_receiver; use crate::tools::format_exec_output_str; use codex_protocol::ThreadId; @@ -3758,7 +3761,21 @@ async fn record_context_updates_and_set_reference_context_item_persists_full_rei #[tokio::test] async fn run_user_shell_command_does_not_set_reference_context_item() { - let (session, _turn_context, rx) = make_session_and_context_with_rx().await; + let (mut session, _turn_context, rx) = make_session_and_context_with_rx().await; + if cfg!(windows) { + // This test only cares that standalone shell commands do not mutate the + // prior reference context. Use cmd.exe on Windows so the assertion does + // not depend on PowerShell startup behavior. + let windows_shell = Arc::new(Shell { + shell_type: ShellType::Cmd, + shell_path: PathBuf::from("cmd.exe"), + shell_snapshot: empty_shell_snapshot_receiver(), + }); + Arc::get_mut(&mut session) + .expect("session should be uniquely owned for test setup") + .services + .user_shell = windows_shell; + } { let mut state = session.state.lock().await; state.set_reference_context_item(None);