Stabilize Windows shell and fuzzy search tests

Co-authored-by: Codex <noreply@openai.com>
2026-10-29 19:08:43 +00:00 · 2026-03-14 08:51:10 +00:00
parent 495ef3f76f
commit d017d0fc35
3 changed files with 360 additions and 9 deletions
@@ -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. |
@@ -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<serde_json::Value, JSONRPCErrorError>;
+type PendingRequest = JoinHandle<std::io::Result<RawRequestResult>>;
+
+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<InProcessClientHandle>,
+    next_request_id: i64,
+    pending_requests: HashMap<i64, PendingRequest>,
+    start_args: Option<InProcessStartArgs>,
+}
+
+impl McpProcess {
+    async fn new(codex_home: &Path) -> Result<Self> {
+        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<String>,
+        cancellation_token: Option<String>,
+    ) -> Result<i64> {
+        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<String>,
+    ) -> Result<i64> {
+        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<String>,
+    ) -> Result<JSONRPCResponse> {
+        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<i64> {
+        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<JSONRPCResponse> {
+        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<i64> {
+        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<JSONRPCResponse> {
+        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<JSONRPCResponse> {
+        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<JSONRPCError> {
+        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<JSONRPCNotification> {
+        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<PendingRequest> {
+        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<JSONRPCNotification> {
+    Ok(serde_json::from_value(serde_json::to_value(notification)?)?)
+}
+
 async fn initialized_mcp(codex_home: &TempDir) -> Result<McpProcess> {
    create_config_toml(codex_home.path())?;
    let mut mcp = McpProcess::new(codex_home.path()).await?;
@@ -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);