From 87941e5d753dba159798576ab519cc97a5dc3edc Mon Sep 17 00:00:00 2001 From: Ahmed Ibrahim <219906144+aibrahim-oai@users.noreply.github.com> Date: Sat, 14 Mar 2026 05:08:11 +0000 Subject: [PATCH] Harden remaining Windows permissions popup navigation Co-authored-by: Codex --- .codex/flaky-test-triage.md | 4 ++- codex-rs/tui/src/chatwidget/tests.rs | 37 ++++++++++++++-------------- 2 files changed, 21 insertions(+), 20 deletions(-) diff --git a/.codex/flaky-test-triage.md b/.codex/flaky-test-triage.md index 960ad18c6c..16613b1e12 100644 --- a/.codex/flaky-test-triage.md +++ b/.codex/flaky-test-triage.md @@ -48,7 +48,8 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron - 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. -- Rationale: commit `8bc3d489a` fixed the cross-platform `(current)` suffix issue, but `rust-ci` still failed on both Windows test lanes. The remaining Windows-only difference is that the permissions popup includes `Read Only` and selection wraps, so hard-coded extra key presses can land on the wrong preset even when the product behavior is correct. +- 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. +- 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. ## Constraints @@ -69,3 +70,4 @@ Older failures also appeared on Linux, but the repeated cross-PR signal is stron | `fc98d21ad` | Select Smart Approvals in session-configured popup tests | failed | `Bazel (experimental)` failed on run `23079852646` across macOS and Linux. Investigation narrowed the likely issue to the overly broad `selected_popup_line()` helper introduced in this commit. | | `a30e8e2ec` | Narrow permissions popup selection helper | failed | `Bazel (experimental)` still failed on Linux in run `23080049381`. The helper narrowing was necessary, but Linux snapshots showed the generic `(current)` assertions were still too strict for the default/full-access permission history tests. | | `8bc3d489a` | Relax popup current-label assertions | failed | `rust-ci` failed on run `23080137075` in both Windows test jobs after Linux and Bazel turned green. The remaining Windows-only issue is likely the `Read Only` preset and wrapping popup navigation in the generic permission history tests. | +| `e96d895c9` | Stabilize Windows permissions popup navigation | failed | `rust-ci` failed on run `23080608881` in both Windows test jobs again. The history snapshot tests were fixed, but another Windows-only extra `Down` remained in `permissions_full_access_history_cell_emitted_only_after_confirmation`, and adjacent popup-selection tests still relied on fixed-step navigation. | diff --git a/codex-rs/tui/src/chatwidget/tests.rs b/codex-rs/tui/src/chatwidget/tests.rs index f1bbcf0b35..a08d13a16e 100644 --- a/codex-rs/tui/src/chatwidget/tests.rs +++ b/codex-rs/tui/src/chatwidget/tests.rs @@ -6615,6 +6615,19 @@ fn selected_permissions_popup_line(popup: &str) -> &str { }) } +fn move_permissions_popup_selection_to(chat: &mut ChatWidget, label: &str, direction: KeyCode) { + for _ in 0..4 { + let popup = render_bottom_popup(chat, 120); + if selected_permissions_popup_line(&popup).contains(label) { + return; + } + chat.handle_key_event(KeyEvent::from(direction)); + } + + let popup = render_bottom_popup(chat, 120); + panic!("expected permissions popup to select {label}: {popup}"); +} + #[tokio::test] async fn apps_popup_stays_loading_until_final_snapshot_updates() { let (mut chat, _rx, _op_rx) = make_chatwidget_manual(None).await; @@ -8386,13 +8399,7 @@ async fn permissions_selection_history_snapshot_after_mode_switch() { selected_permissions_popup_line(&popup).contains("Default"), "expected permissions popup to open with Default selected: {popup}" ); - for _ in 0..3 { - let popup = render_bottom_popup(&chat, 120); - if selected_permissions_popup_line(&popup).contains("Full Access") { - break; - } - chat.handle_key_event(KeyEvent::from(KeyCode::Down)); - } + move_permissions_popup_selection_to(&mut chat, "Full Access", KeyCode::Down); let popup = render_bottom_popup(&chat, 120); assert!( selected_permissions_popup_line(&popup).contains("Full Access"), @@ -8434,13 +8441,7 @@ async fn permissions_selection_history_snapshot_full_access_to_default() { selected_permissions_popup_line(&popup).contains("Full Access"), "expected permissions popup to open with Full Access selected: {popup}" ); - for _ in 0..3 { - let popup = render_bottom_popup(&chat, 120); - if selected_permissions_popup_line(&popup).contains("Default") { - break; - } - chat.handle_key_event(KeyEvent::from(KeyCode::Up)); - } + move_permissions_popup_selection_to(&mut chat, "Default", KeyCode::Up); let popup = render_bottom_popup(&chat, 120); assert!( selected_permissions_popup_line(&popup).contains("Default"), @@ -8684,7 +8685,7 @@ async fn permissions_selection_can_disable_smart_approvals() { "expected permissions popup to open with Smart Approvals selected: {popup}" ); - chat.handle_key_event(KeyEvent::from(KeyCode::Up)); + move_permissions_popup_selection_to(&mut chat, "Default", KeyCode::Up); let popup = render_bottom_popup(&chat, 120); assert!( popup @@ -8741,7 +8742,7 @@ async fn permissions_selection_sends_approvals_reviewer_in_override_turn_context "expected permissions popup to open with the current preset selected: {popup}" ); - chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + move_permissions_popup_selection_to(&mut chat, "Smart Approvals", KeyCode::Down); let popup = render_bottom_popup(&chat, 120); assert!( popup @@ -8787,9 +8788,7 @@ async fn permissions_full_access_history_cell_emitted_only_after_confirmation() chat.config.notices.hide_full_access_warning = None; chat.open_permissions_popup(); - chat.handle_key_event(KeyEvent::from(KeyCode::Down)); - #[cfg(target_os = "windows")] - chat.handle_key_event(KeyEvent::from(KeyCode::Down)); + move_permissions_popup_selection_to(&mut chat, "Full Access", KeyCode::Down); chat.handle_key_event(KeyEvent::from(KeyCode::Enter)); let mut open_confirmation_event = None;