From 4dbe4593e3bca7285e097b81809e7f2d17f96fe1 Mon Sep 17 00:00:00 2001 From: simota Date: Fri, 11 Sep 2026 08:28:17 +0900 Subject: [PATCH] fix(app): keep tracking auto-approve prompts that land while scrolled back A prompt drawn while the viewport was scrolled back was dropped by the ViewportNotLive suppression, and a wheel scroll back to the live rows produces no pty output to rescan on, so the prompt was never approved until the agent redrew. Codex's inline UI makes this common. - Scan the live tail rows instead of the scrolled viewport and treat ViewportNotLive like the input cooldown: keep the match tracked and let the 350 ms stability rescan poll until the viewport is live again. Firing still requires two unsuppressed matches at offset 0. - Make the Codex `Environment:` row optional (codex 0.154 omits it without a selected environment) and accept the cross-thread footer. - Add NOA_AUTO_APPROVE_TRACE=1 to log decisions and main-thread rejects. Claude-Session: https://claude.ai/code/session_01Nzt9w133uttk9d1FjUUJ2G --- crates/noa-app/src/app/auto_approve.rs | 24 +- crates/noa-app/src/auto_approve.rs | 252 +++++++++++++++++-- crates/noa-app/src/io_thread/auto_approve.rs | 10 +- docs/specs/auto-approve-mode.md | 1 + 4 files changed, 264 insertions(+), 23 deletions(-) diff --git a/crates/noa-app/src/app/auto_approve.rs b/crates/noa-app/src/app/auto_approve.rs index 686c1da8..33e0b452 100644 --- a/crates/noa-app/src/app/auto_approve.rs +++ b/crates/noa-app/src/app/auto_approve.rs @@ -36,7 +36,10 @@ impl App { else { return; }; - let reject = || { + let reject = |why: &str| { + if auto_approve::trace_enabled() { + eprintln!("[auto-approve] reject {signature:?}: {why}"); + } let _ = feedback_tx.send(AutoApproveFeedback { signature, region_hash, @@ -44,7 +47,7 @@ impl App { }); }; if !pane_live || !auto_enabled { - reject(); + reject(if pane_live { "mode off" } else { "pane gone" }); return; } @@ -53,11 +56,13 @@ impl App { .get(&id) .and_then(|card| card.process.clone()) else { - reject(); + reject("no foreground process name yet"); return; }; if classify_agent(&process) != signature.agent() { - reject(); + reject(&format!( + "foreground process {process:?} is not the prompt's agent" + )); return; } @@ -67,7 +72,7 @@ impl App { .get(&window_id) .and_then(|state| state.surfaces.get(&pane_id)) else { - reject(); + reject("surface gone"); return; }; let terminal = surface.terminal.lock(); @@ -77,7 +82,7 @@ impl App { scrollback_offset: terminal.viewport_offset(), guards: *surface.auto_approve_guards.lock(), }; - let rows = auto_approve::viewport_rows_from_terminal(&terminal); + let rows = auto_approve::live_rows_from_terminal(&terminal); let cursor = terminal.active().cursor; auto_approve::rescan_signature( &rows, @@ -90,17 +95,20 @@ impl App { ) }; if live_match.is_none_or(|matched| matched.region_hash != region_hash) { - reject(); + reject("prompt changed or suppressed at injection time"); return; } match self.queue_pane_pty_bytes(window_id, pane_id, signature.bytes()) { QueueInputResult::Queued | QueueInputResult::Deferred => {} QueueInputResult::Dropped | QueueInputResult::Disconnected => { - reject(); + reject("pty input queue refused the bytes"); return; } } + if auto_approve::trace_enabled() { + eprintln!("[auto-approve] sent {signature:?} to {process:?}"); + } let _ = feedback_tx.send(AutoApproveFeedback { signature, region_hash, diff --git a/crates/noa-app/src/auto_approve.rs b/crates/noa-app/src/auto_approve.rs index 63c93359..90fe005c 100644 --- a/crates/noa-app/src/auto_approve.rs +++ b/crates/noa-app/src/auto_approve.rs @@ -324,7 +324,9 @@ pub(crate) fn detect_and_update_any_agent( // (see `apply_decision_state`), so only they pay for a scan. let matched = matches!( reason, - SuppressReason::RecentUserInput | SuppressReason::PasteActive + SuppressReason::RecentUserInput + | SuppressReason::PasteActive + | SuppressReason::ViewportNotLive ) .then(|| find_prompt(rows, cursor, None)) .flatten(); @@ -351,6 +353,14 @@ pub(crate) fn rescan_signature( find_signature(rows, cursor, signature(signature_id)) } +/// `NOA_AUTO_APPROVE_TRACE=1`: log every tracked-prompt decision and every +/// main-thread reject to stderr, so a silent non-approval can be diagnosed +/// from a terminal launch without a debugger. +pub(crate) fn trace_enabled() -> bool { + static ENABLED: std::sync::OnceLock = std::sync::OnceLock::new(); + *ENABLED.get_or_init(|| std::env::var_os("NOA_AUTO_APPROVE_TRACE").is_some()) +} + pub(crate) fn viewport_rows_from_terminal(terminal: &Terminal) -> Vec { terminal .active() @@ -360,6 +370,28 @@ pub(crate) fn viewport_rows_from_terminal(terminal: &Terminal) -> Vec { .collect() } +/// The live bottom of the active screen regardless of how far the viewport +/// is scrolled back. A prompt that lands while the user is reading history +/// stays tracked (and `ViewportNotLive`-suppressed) until they return to the +/// live rows; scanning the scrolled viewport instead would drop it, and a +/// wheel scroll back to the bottom produces no pty output to rescan on. +pub(crate) fn live_rows_from_terminal(terminal: &Terminal) -> Vec { + let screen = terminal.active(); + if screen.viewport_offset() == 0 { + return viewport_rows_from_terminal(terminal); + } + let rows = usize::from(screen.rows); + let total = screen.total_rows(); + (total.saturating_sub(rows)..total) + .map(|idx| { + screen + .absolute_row(idx) + .map(|row| row_text(&row.cells)) + .unwrap_or_default() + }) + .collect() +} + #[cfg(test)] fn detect_inner( rows: &[RowText], @@ -475,12 +507,17 @@ fn apply_decision_state( } } Decision::Suppressed(reason) => { - // A fast reply can become static during the input cooldown. Keep - // rescanning that known prompt so it can arm when the guard expires, - // while still requiring two unsuppressed matches before sending. + // A fast reply can become static during the input cooldown, and a + // prompt can land while the viewport is scrolled back. Keep + // rescanning that known prompt so it can arm when the guard expires + // or the user returns to the live rows (a wheel scroll produces no + // pty output to rescan on), while still requiring two unsuppressed + // matches before sending. state.last_match = if matches!( reason, - SuppressReason::RecentUserInput | SuppressReason::PasteActive + SuppressReason::RecentUserInput + | SuppressReason::PasteActive + | SuppressReason::ViewportNotLive ) { matched.map(|matched| MatchKey { signature: matched.signature, @@ -648,7 +685,12 @@ fn match_menu_prompt_region( Some((start, end)) } else { let text = rows[start].split_whitespace().collect::>().join(" "); - (text == footer_text).then_some((start, start)) + let matched = text == footer_text + || (sig.id == AutoApproveSignature::CodexCommand + && text + .strip_prefix(footer_text) + .is_some_and(|rest| rest == " or o to open thread")); + matched.then_some((start, start)) } })?; let mut selected = @@ -721,14 +763,15 @@ fn agy_status_row(status: &str) -> bool { } fn codex_command_menu(rows: &[RowText], anchor: usize, option: usize, footer: usize) -> bool { + // `Environment:` only appears when Codex has a selected environment + // (0.154 omits it for plain local runs), so the command row is the + // dialog's sole mandatory context. let context = &rows[anchor + 1..option]; - if !context.iter().any(|row| row.trim() == "Environment: local") - || !context.iter().any(|row| { - row.trim() - .strip_prefix("$ ") - .is_some_and(|command| !command.trim().is_empty()) - }) - { + if !context.iter().any(|row| { + row.trim() + .strip_prefix("$ ") + .is_some_and(|command| !command.trim().is_empty()) + }) { return false; } let options: Vec<_> = rows[option + 1..footer] @@ -1025,6 +1068,41 @@ mod tests { ]) } + // Codex 0.154 renders `Environment:` only for a selected environment; + // a local run shows `Reason:`/`Permission rule:`/`Thread:` rows instead + // (layouts from codex-rs approval_overlay snapshots). + fn codex_local_command_prompt() -> Vec { + rows(&[ + "Would you like to run the following command?", + "", + "Reason: need filesystem access", + "", + "Permission rule: network; read `/tmp/readme.txt`; write `/tmp/out.txt`", + "", + "$ cat /tmp/readme.txt", + "", + "› 1. Yes, proceed (y)", + " 2. No, and tell Codex what to do differently (esc)", + "", + "Press enter to confirm or esc to cancel", + ]) + } + + fn codex_cross_thread_command_prompt() -> Vec { + rows(&[ + "Would you like to run the following command?", + "", + "Thread: Robie [explorer]", + "", + "$ echo hi", + "", + "› 1. Yes, proceed (y)", + " 2. No, and tell Codex what to do differently (esc)", + "", + "Press enter to confirm or esc to cancel or o to open thread", + ]) + } + fn agy_question_prompt() -> Vec { rows(&[ "Question", @@ -1300,7 +1378,6 @@ mod tests { codex_command_prompt(), vec![ (0, "Would you like to do something else?"), - (2, "Environment: remote"), (6, "$ "), (8, " 1. Yes, proceed (y)"), (8, "› 1. Yes, proceed (y), and remember this choice"), @@ -1315,6 +1392,10 @@ mod tests { 12, "Press enter to confirm or esc to cancel · ctrl+r Review", ), + ( + 12, + "Press enter to confirm or esc to cancel or x to open thread", + ), ], ), ( @@ -1604,6 +1685,66 @@ mod tests { assert!(!state.needs_static_rescan()); } + #[test] + fn scrolled_back_prompt_rearms_when_viewport_returns_live() { + let now = fixed_now(); + for prompt in [ + codex_command_prompt(), + claude_edit_prompt(), + agy_command_prompt(), + ] { + let cursor = cursor(if prompt == claude_edit_prompt() { 1 } else { 0 }); + let mut state = AutoApproveState::default(); + let mut ctx = base_ctx(now); + ctx.alt_screen = false; + ctx.scrollback_offset = 3; + assert_eq!( + detect_and_update_any_agent(&prompt, cursor, ctx, &mut state), + Decision::Suppressed(SuppressReason::ViewportNotLive) + ); + assert!(state.needs_static_rescan()); + ctx.scrollback_offset = 0; + assert_eq!( + detect_and_update_any_agent(&prompt, cursor, ctx, &mut state), + Decision::Hold + ); + assert!(matches!( + detect_and_update_any_agent(&prompt, cursor, ctx, &mut state), + Decision::Fire { .. } + )); + } + // Nothing at the live bottom: scrolled-back tracking costs no rescan. + let mut state = AutoApproveState::default(); + let mut ctx = base_ctx(now); + ctx.alt_screen = false; + ctx.scrollback_offset = 3; + let _ = + detect_and_update_any_agent(&rows(&["unrelated output"]), cursor(0), ctx, &mut state); + assert!(!state.needs_static_rescan()); + } + + #[test] + fn live_rows_ignore_viewport_scrollback() { + let mut terminal = Terminal::new(noa_core::GridSize::new(20, 3)); + let mut stream = noa_vt::Stream::new(); + stream.feed(b"one\r\ntwo\r\nthree\r\nfour\r\nfive", &mut terminal); + terminal.scroll_viewport_up(2); + assert_ne!(terminal.viewport_offset(), 0); + assert_eq!( + viewport_rows_from_terminal(&terminal), + rows(&["one", "two", "three"]) + ); + assert_eq!( + live_rows_from_terminal(&terminal), + rows(&["three", "four", "five"]) + ); + terminal.scroll_viewport_to_bottom(); + assert_eq!( + live_rows_from_terminal(&terminal), + viewport_rows_from_terminal(&terminal) + ); + } + #[test] fn static_rescan_tracks_changed_prompts_after_an_accepted_approval() { let now = fixed_now(); @@ -1882,6 +2023,89 @@ mod tests { } } + #[test] + fn detect_codex_prompts_without_environment_row() { + let now = fixed_now(); + for prompt in [ + codex_local_command_prompt(), + codex_cross_thread_command_prompt(), + ] { + let mut state = AutoApproveState::default(); + let mut ctx = base_ctx(now); + ctx.alt_screen = false; + let cursor = cursor((prompt.len() - 1) as u16); + assert_eq!( + detect_and_update_any_agent(&prompt, cursor, ctx, &mut state), + Decision::Hold + ); + let Decision::Fire { signature, .. } = + detect_and_update_any_agent(&prompt, cursor, ctx, &mut state) + else { + panic!("codex prompt without Environment row should fire: {prompt:?}"); + }; + assert_eq!(signature, AutoApproveSignature::CodexCommand); + } + } + + #[test] + fn codex_environment_prompt_with_wide_reason_from_vt_grid() { + // Layout from the 2026-09-11 screenshot (`Environment:` present, + // wide-character `Reason:`), painted the way ratatui does: absolute + // cursor positioning per row, styled spans, hidden cursor parked on + // the footer row. + let cols = 200u16; + let grid_rows = 40u16; + let mut terminal = Terminal::new(noa_core::GridSize::new(cols, grid_rows)); + let mut stream = noa_vt::Stream::new(); + let top = 26u16; + let lines: Vec = vec![ + " \x1b[1mWould you like to run the following command?\x1b[0m".into(), + "".into(), + " Environment: \x1b[1mlocal\x1b[0m".into(), + "".into(), + " Reason: \x1b[3mSwiftのコンパイラーキャッシュへの書き込みがサンドボックスで拒否されたため、権限を拡張してチート機能を含むホスト試験を実行してよいですか？\x1b[0m".into(), + "".into(), + " \x1b[1m$\x1b[0m \x1b[34mmake\x1b[0m host-check".into(), + "".into(), + "\x1b[1;36m› 1. Yes, proceed \x1b[2m(y)\x1b[0m".into(), + " 2. Yes, and don't ask again for commands that start with `make host-check` \x1b[2m(p)\x1b[0m".into(), + " 3. No, and tell Codex what to do differently \x1b[2m(esc)\x1b[0m".into(), + "".into(), + " \x1b[2mPress enter to confirm or esc to cancel\x1b[0m".into(), + ]; + let mut frame = String::from("\x1b[?25l"); + for (i, line) in lines.iter().enumerate() { + frame.push_str(&format!("\x1b[{};1H\x1b[2K{}", top + i as u16, line)); + } + frame.push_str(&format!("\x1b[{};1H", top + lines.len() as u16)); + for chunk in frame.as_bytes().chunks(5) { + stream.feed(chunk, &mut terminal); + } + let screen = viewport_rows_from_terminal(&terminal); + let cursor = terminal.active().cursor; + let cursor = Point { + x: cursor.x, + y: cursor.y, + }; + let mut state = AutoApproveState::default(); + let ctx = base_ctx(fixed_now()); + assert_eq!( + detect_and_update_any_agent(&screen, cursor, ctx, &mut state), + Decision::Hold + ); + let d = detect_and_update_any_agent(&screen, cursor, ctx, &mut state); + assert!( + matches!( + d, + Decision::Fire { + signature: AutoApproveSignature::CodexCommand, + .. + } + ), + "{d:?}" + ); + } + #[test] fn codex_wrapped_rejection_is_detected_from_vt_grid() { for remember in [false, true] { diff --git a/crates/noa-app/src/io_thread/auto_approve.rs b/crates/noa-app/src/io_thread/auto_approve.rs index 280f7960..acb72103 100644 --- a/crates/noa-app/src/io_thread/auto_approve.rs +++ b/crates/noa-app/src/io_thread/auto_approve.rs @@ -60,7 +60,7 @@ pub(super) fn detect_auto_approve_candidate( scrollback_offset: term.viewport_offset(), guards: *publish.guards.lock(), }; - let rows = auto_approve::viewport_rows_from_terminal(term); + let rows = auto_approve::live_rows_from_terminal(term); let cursor = term.active().cursor; let decision = auto_approve::detect_and_update_any_agent( &rows, @@ -71,6 +71,14 @@ pub(super) fn detect_auto_approve_candidate( ctx, state, ); + if auto_approve::trace_enabled() + && (!matches!(decision, Decision::Hold) || state.needs_static_rescan()) + { + eprintln!( + "[auto-approve] scan: {decision:?} alt={} offset={} guards={:?}", + ctx.alt_screen, ctx.scrollback_offset, ctx.guards + ); + } match decision { Decision::Fire { signature, diff --git a/docs/specs/auto-approve-mode.md b/docs/specs/auto-approve-mode.md index 4188f51d..f1aeb41f 100644 --- a/docs/specs/auto-approve-mode.md +++ b/docs/specs/auto-approve-mode.md @@ -198,6 +198,7 @@ A per-tab opt-in "auto-approve mode." In tabs where it's ON, within io_thread's - **FR-3** Prompt detection: match the visible viewport against a **hardcoded signature matrix** of `AgentKind` × prompt type. Dual signature = ① anchor text + numeric label, ② **selection-marker condition** (the selection cursor character, e.g. "❯", must be at the start of the line for the first affirmative option; grid character-based check, independent of SGR attributes). If either fails to match, no fire. - **FR-4** Two-consecutive-match debounce: confirm only when the same signature matches on two consecutive feed scans in a row (blocks false matches from partial rendering). - **FR-5** Precondition gate: armed only during alt-screen or while tracking the viewport tail (**scrollback display offset == 0**, i.e. the live tail is being displayed). + - Addendum (2026-09-11): the gate suppresses *firing* only. Detection scans the **live tail rows** (not the scrolled viewport) so a prompt that lands while the user is reading history stays tracked under `ViewportNotLive`, and the 350 ms stability rescan keeps polling until the viewport returns to offset 0 (a wheel scroll produces no pty output to rescan on). Two unsuppressed matches are still required before injection. `NOA_AUTO_APPROVE_TRACE=1` logs every tracked decision and main-thread reject to stderr. - **FR-6** Affirmative response injection: on confirmation, the main thread authoritatively resolves (window,pane) via `UserEvent::AutoApprove` and sends the fixed injection byte sequence for that agent × signature via `write_pane_pty_bytes`. - **FR-7** Pre-injection reconfirmation: right before injecting, the main thread rescans the target pane; if the signature has disappeared, abort the send (guards against wrong target pane / stale state). - **FR-8** Consumed flag: after approval, don't rearm until the **cell-content hash of the signature-matched row range (anchor row through the last option row)** changes, plus a cap on consecutive approvals.