diff --git a/Cargo.lock b/Cargo.lock index 5c3a134..94f6ddf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5475,6 +5475,15 @@ dependencies = [ "either", ] +[[package]] +name = "itertools" +version = "0.12.1" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "ba291022dbbd398a455acf126c1e341954079855bc60dfdda641363bd6922569" +dependencies = [ + "either", +] + [[package]] name = "itertools" version = "0.13.0" @@ -7879,6 +7888,18 @@ dependencies = [ "thiserror 2.0.18", ] +[[package]] +name = "prometheus-parse" +version = "0.2.5" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "811031bea65e5a401fb2e1f37d802cca6601e204ac463809a3189352d13b78a5" +dependencies = [ + "chrono", + "itertools 0.12.1", + "once_cell", + "regex", +] + [[package]] name = "proptest" version = "1.10.0" @@ -12884,6 +12905,7 @@ name = "xai-circuit-breaker" version = "0.1.0" dependencies = [ "log", + "tonic", ] [[package]] @@ -12977,6 +12999,7 @@ dependencies = [ "opentelemetry_sdk", "parking_lot", "prometheus", + "prometheus-parse", "prost", "reqwest 0.12.24", "schemars 1.0.4", diff --git a/SOURCE_REV b/SOURCE_REV index cdc7e55..8553d0a 100644 --- a/SOURCE_REV +++ b/SOURCE_REV @@ -1 +1 @@ -1adcd1f477870e4a97bacbd6be78c8a3bfbac46d +2a818575225183d8ca915f5632a09b8067b5156a diff --git a/crates/codegen/xai-crash-handler/src/handler.rs b/crates/codegen/xai-crash-handler/src/handler.rs index 327be17..17efd62 100644 --- a/crates/codegen/xai-crash-handler/src/handler.rs +++ b/crates/codegen/xai-crash-handler/src/handler.rs @@ -1,7 +1,12 @@ -//! Cross-platform crash handler for fatal memory faults. +//! Cross-platform crash handler for fatal memory faults and aborts. //! -//! - **Unix**: SIGBUS/SIGSEGV via `sigaction(2)`. +//! - **Unix**: SIGBUS/SIGSEGV/SIGABRT via `sigaction(2)`. SIGABRT matters +//! because release builds ship with `panic = "abort"`, so every Rust panic +//! terminates via `abort(3)` — without a SIGABRT handler those deaths leave +//! no crash report. //! - **Windows**: `EXCEPTION_ACCESS_VIOLATION` et al. via `SetUnhandledExceptionFilter`. +//! `abort()` does not go through the unhandled-exception filter, so SIGABRT +//! capture is Unix-only. //! //! Captures crash PC + frame-pointer chain. All handler operations are //! minimal (raw pointer reads, direct file I/O, atomics — no allocation). @@ -275,11 +280,16 @@ mod imp { } } - /// Register a signal handler for SIGBUS and SIGSEGV. + /// Register a signal handler for SIGBUS, SIGSEGV, and SIGABRT. + /// + /// SIGABRT is hooked so `panic = "abort"` deaths (every Rust panic in + /// release builds) produce a crash report instead of a bare `Aborted`. /// /// Flags: `SA_SIGINFO | SA_ONSTACK | SA_RESETHAND`. `SA_RESETHAND` /// resets disposition to `SIG_DFL` after delivery, preventing recursive - /// faults in the handler from looping. + /// faults in the handler from looping. The handlers additionally restore + /// `SIG_DFL` and re-raise explicitly, so the process still terminates + /// with the original signal's semantics (exit status, core dumps). /// /// # Safety /// @@ -295,6 +305,7 @@ mod imp { libc::sigaction(libc::SIGBUS, &sa, std::ptr::null_mut()); libc::sigaction(libc::SIGSEGV, &sa, std::ptr::null_mut()); + libc::sigaction(libc::SIGABRT, &sa, std::ptr::null_mut()); } } @@ -424,7 +435,8 @@ mod imp { } } - /// Install a minimal SIGSEGV/SIGBUS handler that restores termios on crash. + /// Install a minimal SIGSEGV/SIGBUS/SIGABRT handler that restores termios + /// on crash. /// /// Does NOT write terminal escape codes — call /// [`enable_terminal_escape_restore`] after TUI modes are enabled. @@ -486,8 +498,8 @@ mod imp { true } - /// Upgrade SIGSEGV/SIGBUS handlers to include terminal escape code - /// restoration. Call when TUI modes are enabled. + /// Upgrade SIGSEGV/SIGBUS/SIGABRT handlers to include terminal escape + /// code restoration. Call when TUI modes are enabled. pub fn enable_terminal_escape_restore() { unsafe { register_crash_signals(if CRASH_FD.load(Ordering::Relaxed) >= 0 { @@ -498,7 +510,7 @@ mod imp { } } - /// Downgrade SIGSEGV/SIGBUS handlers to termios-only restoration. + /// Downgrade SIGSEGV/SIGBUS/SIGABRT handlers to termios-only restoration. /// Call when TUI modes are disabled. pub fn disable_terminal_escape_restore() { unsafe { @@ -856,7 +868,7 @@ pub fn disable_terminal_escape_restore() {} mod tests { use std::sync::Mutex; - // SIGSEGV/SIGBUS handlers are process-global. Tests in this binary run on + // SIGSEGV/SIGBUS/SIGABRT handlers are process-global. Tests in this binary run on // parallel threads, so any two tests that install/read these handlers race. // Serialize them through this lock (poison-tolerant: a real assertion // failure in one test must not cascade into the other). @@ -899,6 +911,18 @@ mod tests { 0, "SIGBUS handler must use alternate signal stack" ); + + assert_eq!(libc::sigaction(libc::SIGABRT, std::ptr::null(), &mut sa), 0); + assert_ne!( + sa.sa_sigaction, + libc::SIG_DFL, + "SIGABRT handler should not be SIG_DFL after install" + ); + assert_ne!( + sa.sa_flags & libc::SA_ONSTACK, + 0, + "SIGABRT handler must use alternate signal stack" + ); } } diff --git a/crates/codegen/xai-crash-handler/src/lib.rs b/crates/codegen/xai-crash-handler/src/lib.rs index 1e0b603..ab77181 100644 --- a/crates/codegen/xai-crash-handler/src/lib.rs +++ b/crates/codegen/xai-crash-handler/src/lib.rs @@ -1,7 +1,11 @@ //! Cross-platform crash handler with startup crash detection. //! -//! - **Unix**: SIGBUS/SIGSEGV via `sigaction(2)`. +//! - **Unix**: SIGBUS/SIGSEGV/SIGABRT via `sigaction(2)`. SIGABRT capture +//! means `panic = "abort"` builds (every shipped release) leave a crash +//! report when a Rust panic aborts the process. //! - **Windows**: access violations via `SetUnhandledExceptionFilter`. +//! SIGABRT capture is Unix-only — `abort()` on Windows does not route +//! through the unhandled-exception filter. //! //! # Usage //! @@ -66,7 +70,8 @@ pub struct CrashReport { pub report_path: PathBuf, } -/// Install the crash handler for SIGBUS and SIGSEGV. +/// Install the crash handler for SIGBUS, SIGSEGV, and SIGABRT (Unix; on +/// Windows only access violations are captured). /// /// Must be called early in `main()`, before any async runtime or thread /// spawning. Creates `crash_dir` if it does not exist. @@ -77,7 +82,8 @@ pub fn install(config: CrashHandlerConfig) -> bool { handler::install(&config.crash_dir, &config.app_version) } -/// Install a minimal SIGSEGV/SIGBUS handler that only restores the terminal. +/// Install a minimal SIGSEGV/SIGBUS/SIGABRT handler that only restores the +/// terminal. /// /// On Unix, saves the current termios state, allocates an alternate signal /// stack, and registers a handler that writes terminal restore escape @@ -96,13 +102,13 @@ pub fn install_terminal_restore_only() { handler::install_terminal_restore_only() } -/// Upgrade SIGSEGV/SIGBUS handlers to include terminal escape code +/// Upgrade SIGSEGV/SIGBUS/SIGABRT handlers to include terminal escape code /// restoration. Call when TUI modes are enabled. pub fn enable_terminal_escape_restore() { handler::enable_terminal_escape_restore() } -/// Downgrade SIGSEGV/SIGBUS handlers to termios-only restoration. +/// Downgrade SIGSEGV/SIGBUS/SIGABRT handlers to termios-only restoration. /// Call when TUI modes are disabled. pub fn disable_terminal_escape_restore() { handler::disable_terminal_escape_restore() diff --git a/crates/codegen/xai-crash-handler/src/symbolicate.rs b/crates/codegen/xai-crash-handler/src/symbolicate.rs index 19ff111..71633c6 100644 --- a/crates/codegen/xai-crash-handler/src/symbolicate.rs +++ b/crates/codegen/xai-crash-handler/src/symbolicate.rs @@ -81,6 +81,9 @@ pub fn format_report(blob: &CrashBlob, frames: &[ResolvedFrame]) -> String { pub fn signal_name(sig: u8) -> &'static str { match sig as i32 { 4 => "SIGILL (Illegal instruction)", + // SIGABRT is 6 on both macOS and Linux. + // With panic = "abort", every Rust panic terminates via SIGABRT. + 6 => "SIGABRT (Abort)", // SIGBUS is 10 on macOS, 7 on Linux 7 | 10 => "SIGBUS (Bus error)", 11 => "SIGSEGV (Segmentation fault)", @@ -89,6 +92,11 @@ pub fn signal_name(sig: u8) -> &'static str { } fn si_code_name(sig: u8, code: i32) -> &'static str { + // SIGABRT carries no fault-specific si_code (abort(3) raises it directly; + // the kernel reports SI_USER/SI_TKILL-style origins instead). + if sig == 6 { + return "abort() - raised by the process (e.g. Rust panic with panic=abort)"; + } let is_bus = sig == 7 || sig == 10; if is_bus { match code { @@ -112,6 +120,7 @@ mod tests { #[test] fn signal_names() { + assert_eq!(signal_name(6), "SIGABRT (Abort)"); assert_eq!(signal_name(10), "SIGBUS (Bus error)"); assert_eq!(signal_name(7), "SIGBUS (Bus error)"); assert_eq!(signal_name(11), "SIGSEGV (Segmentation fault)"); diff --git a/crates/codegen/xai-crash-handler/tests/integration.rs b/crates/codegen/xai-crash-handler/tests/integration.rs index 754757b..44fb886 100644 --- a/crates/codegen/xai-crash-handler/tests/integration.rs +++ b/crates/codegen/xai-crash-handler/tests/integration.rs @@ -110,6 +110,12 @@ fn subprocess_entry() { unsafe { libc::raise(libc::SIGSEGV) }; } + // Scenario 6: install handler, abort. This is the path every Rust + // panic takes in release builds (panic = "abort" → SIGABRT). + "sigabrt" => { + std::process::abort(); + } + // Scenario 5: tokio runtime + signal coexistence, then clean shutdown. "tokio_signals" => { let rt = tokio::runtime::Builder::new_multi_thread() @@ -269,6 +275,53 @@ fn sigsegv_produces_valid_crash_blob() { assert_eq!(blob.app_version, "0.0.0-test"); } +#[test] +fn sigabrt_produces_valid_crash_blob() { + let tmp = tempfile::tempdir().expect("tempdir"); + let (status, _stdout, _stderr) = run_scenario("sigabrt", tmp.path()); + + // The handler must re-raise with default disposition so the process + // still dies with SIGABRT semantics. The frame-pointer walker may hit + // unmapped memory and cause a secondary SIGSEGV (as in the SIGBUS test). + #[cfg(unix)] + { + use std::os::unix::process::ExitStatusExt; + let sig = status.signal(); + assert!( + sig == Some(libc::SIGABRT) || sig == Some(libc::SIGSEGV), + "process should be killed by SIGABRT (or a secondary SIGSEGV), got signal={sig:?} status={status:?}" + ); + } + + let crash_file = tmp.path().join("last-crash.bin"); + assert!(crash_file.exists(), "crash file should exist after abort()"); + let data = std::fs::read(&crash_file).expect("read crash file"); + let blob = xai_crash_handler::format::CrashBlob::parse(&data).expect("crash blob should parse"); + assert_eq!( + blob.signal, 6, + "signal should be SIGABRT (6), got {}", + blob.signal + ); + assert_eq!(blob.app_version, "0.0.0-test"); + assert!(blob.pid > 0, "PID should be nonzero"); + assert!(blob.timestamp > 0, "timestamp should be nonzero"); + + // check_previous_crash should produce a SIGABRT-labelled report. + let report = + xai_crash_handler::check_previous_crash(tmp.path()).expect("should produce a crash report"); + assert!( + report.signal_name.contains("SIGABRT"), + "report should name SIGABRT, got {}", + report.signal_name + ); + assert_eq!(report.app_version, "0.0.0-test"); + assert!(report.report_path.exists(), "report file should be written"); + assert!( + !crash_file.exists(), + "crash file should be deleted after processing" + ); +} + #[test] fn clean_exit_does_not_produce_crash_report() { let tmp = tempfile::tempdir().expect("tempdir"); diff --git a/crates/codegen/xai-grok-agent/src/builder.rs b/crates/codegen/xai-grok-agent/src/builder.rs index 7c1ab83..ab7c556 100644 --- a/crates/codegen/xai-grok-agent/src/builder.rs +++ b/crates/codegen/xai-grok-agent/src/builder.rs @@ -1568,6 +1568,64 @@ mod tests { "should state the resumed agent must match subagent_type" ); } + /// The bridge's full-discovery snapshot must record every discovered + /// skill name — including `paths:`-gated and preloaded skills that the + /// listing baseline (`slash_skills`) holds back — so session-start + /// telemetry can reuse it instead of re-walking the disk. + #[tokio::test] + async fn discovery_snapshot_records_gated_and_preloaded_skills() { + use xai_grok_tools::computer::local::LocalTerminalBackend; + use xai_grok_tools::notification::ToolNotificationHandle; + let tmp = tempfile::tempdir().unwrap(); + let write_skill = |dir: &str, content: &str| { + let d = tmp.path().join(".grok/skills").join(dir); + std::fs::create_dir_all(&d).unwrap(); + std::fs::write(d.join("SKILL.md"), content).unwrap(); + }; + write_skill( + "snapshot-plain-skill", + "---\nname: snapshot-plain-skill\ndescription: plain\n---\nbody\n", + ); + write_skill( + "snapshot-gated-skill", + "---\nname: snapshot-gated-skill\ndescription: gated\npaths: \"src/**\"\n---\nbody\n", + ); + let mut definition = crate::config::AgentDefinition::default_grok_build(); + definition.skills = vec!["snapshot-plain-skill".to_string()]; + let agent = AgentBuilder::new( + tmp.path().to_path_buf(), + Arc::new(LocalTerminalBackend::new()), + ToolNotificationHandle::noop(), + ) + .from_definition(definition) + .build() + .await + .expect("agent should build with local skill fixtures"); + let snapshot = agent.tool_bridge().skill_discovery_snapshot_names().await; + assert!( + snapshot.contains(&"snapshot-plain-skill".to_string()), + "preloaded skill missing from snapshot: {snapshot:?}" + ); + assert!( + snapshot.contains(&"snapshot-gated-skill".to_string()), + "paths:-gated skill missing from snapshot: {snapshot:?}" + ); + let listed: Vec = agent + .tool_bridge() + .slash_skills() + .await + .into_iter() + .map(|s| s.name) + .collect(); + assert!( + !listed.contains(&"snapshot-gated-skill".to_string()), + "paths:-gated skill must stay out of the listing baseline: {listed:?}" + ); + assert!( + !listed.contains(&"snapshot-plain-skill".to_string()), + "preloaded skill must stay out of the listing baseline: {listed:?}" + ); + } async fn build_pager_agent( profile: crate::config::AgentDefinition, subagents_enabled: bool, diff --git a/crates/codegen/xai-grok-mcp/src/servers.rs b/crates/codegen/xai-grok-mcp/src/servers.rs index c0ebe1f..8fdf567 100644 --- a/crates/codegen/xai-grok-mcp/src/servers.rs +++ b/crates/codegen/xai-grok-mcp/src/servers.rs @@ -37,7 +37,7 @@ use xai_grok_tools::types::{ tool::{ToolKind, ToolNamespace}, tool_metadata::ToolMetadata, }; -use xai_grok_tools::util::ProcessGroup; +use xai_grok_tools::util::{ProcessGroup, ProcessScope}; /// MCP tool name delimiter: server names are qualified as `"server__tool"`. /// Canonical definition lives in `xai_grok_workspace_types`; re-exported here @@ -2017,16 +2017,19 @@ where /// grandchildren (e.g. `npx` -> `node`) before reaping the leader. pub struct SafeTokioChildProcess { child: Option, - process_group: Option, + /// Strong `Arc` owner; the scope holds only a `Weak`, dropped on reap. + process_group: Option>, transport: ResilientRwTransport, } impl SafeTokioChildProcess { /// `server_name` + `event_writer` are threaded into the transport so a /// skipped (undecodable) stdout line emits an `McpTransportDecodeError` - /// event for that server. + /// event for that server. `scope`, when set, enrolls the child's group for + /// session-close reaping. fn spawn( mut cmd: Command, + scope: Option<&ProcessScope>, server_name: String, event_writer: xai_file_utils::events::EventWriter, ) -> std::io::Result<(Self, Option)> { @@ -2048,7 +2051,7 @@ impl SafeTokioChildProcess { // Best-effort: a missing group just degrades to direct-child-only cleanup. let process_group = match ProcessGroup::new() { Ok(mut group) => match group.attach(&child) { - Ok(()) => Some(group), + Ok(()) => Some(Arc::new(group)), Err(e) => { tracing::warn!("Failed to attach MCP child to process group: {e}"); None @@ -2059,6 +2062,30 @@ impl SafeTokioChildProcess { None } }; + // Enrollment ties this child to the *spawning* session's lifetime. + // `SharedMcpPool` may hand the resulting client Arc to subagent + // sessions, but subagents inherit the root session's scope, so the + // root's kill_all cannot strand an in-tree subagent. Residual: any + // detached holder of the Arc loses the transport when the spawning + // session closes — session close is deliberately the reap boundary. + if let (Some(scope), Some(group)) = (scope, process_group.as_ref()) + && !scope.register(group) + { + // The scope latched closed (spawn raced session teardown), so + // `register` already killpg'd the child. Fail fast with a clear + // error instead of proceeding into a doomed rmcp handshake; the + // reap below mirrors `Drop`'s best-effort leader cleanup. + if let Ok(handle) = tokio::runtime::Handle::try_current() { + handle.spawn(async move { + let _ = child.kill().await; + }); + } else if let Err(e) = child.start_kill() { + tracing::warn!("Error signaling MCP child killed by closed scope: {e}"); + } + return Err(std::io::Error::other( + "session is closing (process scope already reclaimed); MCP server not started", + )); + } Ok(( Self { @@ -4043,14 +4070,45 @@ fn stdio_path_override(env: &[acp::EnvVariable]) -> Option<&str> { .map(|e| e.value.as_str()) } +/// Borrowed cross-cutting spawn context whose `scope`, when set, enrolls the stdio child for session-close reaping. +pub struct McpSpawnCtx<'a> { + pub(crate) session_id: Option<&'a str>, + pub(crate) event_writer: &'a xai_file_utils::events::EventWriter, + pub(crate) mode: OauthInteractivity, + pub(crate) scope: Option<&'a ProcessScope>, +} + +impl<'a> McpSpawnCtx<'a> { + pub fn for_session( + session_id: &'a str, + event_writer: &'a xai_file_utils::events::EventWriter, + mode: OauthInteractivity, + scope: Option<&'a ProcessScope>, + ) -> Self { + Self { + session_id: Some(session_id), + event_writer, + mode, + scope, + } + } + + pub fn session_less(event_writer: &'a xai_file_utils::events::EventWriter) -> Self { + Self { + session_id: None, + event_writer, + mode: OauthInteractivity::Interactive, + scope: None, + } + } +} + pub async fn start_mcp_server( mcp_server: acp::McpServer, - session_id: Option<&str>, overrides: Option<&McpClientTimeoutOverrides>, meta_config: Option<&McpServerMetaConfig>, byo_config: Option<&McpOAuthConfig>, - event_writer: &xai_file_utils::events::EventWriter, - mode: OauthInteractivity, + ctx: &McpSpawnCtx<'_>, ) -> Result { let _per_server_timer = xai_grok_telemetry::instrumentation::timer("mcp_start_one_server"); match mcp_server { @@ -4086,24 +4144,27 @@ pub async fn start_mcp_server( } xai_grok_tools::util::detach_command(&mut cmd); - let (transport, stderr_handle) = - SafeTokioChildProcess::spawn(cmd, name.clone(), event_writer.clone()).map_err( - |e| { - tracing::error!("Failed to spawn MCP server '{}': {}", name, e); - xai_grok_telemetry::session_ctx::log_event( - xai_grok_telemetry::events::McpServerFailed { - server_name: name.clone(), - error_type: xai_grok_telemetry::events::McpErrorType::SpawnFailed, - duration_ms: spawn_start.elapsed().as_millis() as u64, - timeout_sec: startup_timeout, - }, - ); - McpError::SpawnFailed { - server: name.clone(), - source: e, - } + let (transport, stderr_handle) = SafeTokioChildProcess::spawn( + cmd, + ctx.scope, + name.clone(), + ctx.event_writer.clone(), + ) + .map_err(|e| { + tracing::error!("Failed to spawn MCP server '{}': {}", name, e); + xai_grok_telemetry::session_ctx::log_event( + xai_grok_telemetry::events::McpServerFailed { + server_name: name.clone(), + error_type: xai_grok_telemetry::events::McpErrorType::SpawnFailed, + duration_ms: spawn_start.elapsed().as_millis() as u64, + timeout_sec: startup_timeout, }, - )?; + ); + McpError::SpawnFailed { + server: name.clone(), + source: e, + } + })?; tracing::debug!("MCP server '{}' spawned: PID={:?}", name, transport.id()); @@ -4128,7 +4189,7 @@ pub async fn start_mcp_server( tracing::info!(server = %name, %url, ?mc, "MCP http: meta config override"); } - let headers = expand_session_id_headers(headers, session_id); + let headers = expand_session_id_headers(headers, ctx.session_id); let http_config = HttpConfig { url: url.clone(), headers, @@ -4150,7 +4211,7 @@ pub async fn start_mcp_server( xai_grok_telemetry::instrumentation::timer("mcp_http_auth_discovery"); match tokio::time::timeout( OAUTH_DISCOVERY_TIMEOUT, - discover_and_prepare_auth(&name, &url, mode), + discover_and_prepare_auth(&name, &url, ctx.mode), ) .await { @@ -4159,17 +4220,17 @@ pub async fn start_mcp_server( tracing::warn!( server = %name, url = %url, - ?mode, + mode = ?ctx.mode, timeout_secs = OAUTH_DISCOVERY_TIMEOUT.as_secs(), "OAuth discovery timed out" ); - event_writer.emit( + ctx.event_writer.emit( xai_file_utils::events::Event::McpOAuthDiscoveryTimeout { server_name: name.clone(), url: url.clone(), }, ); - HttpOauthPrep::on_probe_failure(mode) + HttpOauthPrep::on_probe_failure(ctx.mode) } } }; @@ -4203,12 +4264,10 @@ pub async fn start_mcp_server( pub async fn start_mcp_servers( mcp_servers: Vec, - session_id: Option<&str>, overrides_map: &HashMap, meta_config_map: &McpMetaConfigMap, oauth_config_map: &crate::oauth_config::McpOAuthConfigMap, - event_writer: &xai_file_utils::events::EventWriter, - mode: OauthInteractivity, + ctx: &McpSpawnCtx<'_>, ) -> Vec> { let _mcp_start_timer = xai_grok_telemetry::instrumentation::timer("mcp_start_servers"); @@ -4226,7 +4285,7 @@ pub async fn start_mcp_servers( let overrides = overrides_map.get(server_name); let mc = meta_config_map.get(server_name); let byo = oauth_config_map.get(server_name); - start_mcp_server(server, session_id, overrides, mc, byo, event_writer, mode) + start_mcp_server(server, overrides, mc, byo, ctx) }) .buffer_unordered(8) .collect::>() @@ -4585,6 +4644,7 @@ mod tests { xai_grok_tools::util::detach_command(&mut cmd); let (transport, _stderr) = SafeTokioChildProcess::spawn( cmd, + None, "test".to_string(), xai_file_utils::events::EventWriter::noop(), ) @@ -4616,6 +4676,55 @@ mod tests { std::io::Error::last_os_error().raw_os_error() != Some(libc::ESRCH) } + /// `scope.kill_all()` reaps an enrolled MCP child even when its owner never + /// runs Drop. Non-vacuous: dropping the `Some(&scope)` enrollment makes this + /// time out. + #[cfg(unix)] + #[tokio::test] + async fn scope_kill_all_reaps_enrolled_mcp_child_while_owner_wedged() { + use std::time::Duration; + + let scope = ProcessScope::new(); + + let mut cmd = Command::new("sleep"); + cmd.arg("600").kill_on_drop(true); + xai_grok_tools::util::detach_command(&mut cmd); + let (mut child_process, _stderr) = SafeTokioChildProcess::spawn( + cmd, + Some(&scope), + "wedge-test".to_string(), + xai_file_utils::events::EventWriter::noop(), + ) + .expect("spawn enrolled MCP child"); + assert_eq!( + scope.live_count(), + 1, + "the enrolled MCP child group must be tracked by the scope" + ); + + // Wedge: owner never runs Drop, so kill_all is the only reclaim path. + scope.kill_all(); + + // Take only the handle, not the group, so kill-on-drop can't mask a + // missing enrollment. + let mut child = child_process.child.take().expect("child handle present"); + // Null the strong Arc before reaping the leader below: + // holding it across the reap would let `child_process`'s later Drop + // killpg a reusable pgid — the PID-reuse pattern the Weak ownership + // contract exists to prevent. + child_process.process_group = None; + let status = tokio::time::timeout(Duration::from_secs(5), child.wait()) + .await + .expect("scope.kill_all must have SIGKILL'd the enrolled MCP child group") + .expect("wait on the reclaimed child succeeds"); + use std::os::unix::process::ExitStatusExt; + assert_eq!( + status.signal(), + Some(libc::SIGKILL), + "the MCP child must have been SIGKILL'd by the scope, not have exited cleanly" + ); + } + #[test] fn test_mcp_state_new() { let configs = vec![make_stdio_server("test", "/bin/test")]; diff --git a/crates/codegen/xai-grok-pager-bin/src/main.rs b/crates/codegen/xai-grok-pager-bin/src/main.rs index e4937c9..eba0623 100644 --- a/crates/codegen/xai-grok-pager-bin/src/main.rs +++ b/crates/codegen/xai-grok-pager-bin/src/main.rs @@ -28,6 +28,7 @@ mod jemalloc_malloc_conf { use anyhow::Result; use std::env; use std::net::SocketAddr; +use std::num::NonZeroUsize; use tokio_util::sync::CancellationToken; use xai_grok_pager::app::{ AgentCmd, Command, HeadlessArgs, LeaderMgmtArgs, LeaderMgmtCommand, LeaderMode, @@ -1522,6 +1523,90 @@ fn flag_dashboard_at_startup_if_requested(args: &mut PagerArgs) -> Result<()> { Ok(()) } const RUNTIME_SHUTDOWN_GRACE: std::time::Duration = std::time::Duration::from_secs(2); +const GROK_WORKER_THREADS_ENV: &str = "GROK_WORKER_THREADS"; +/// tokio defaults to one worker per logical CPU. On a host with hundreds of +/// CPUs that can exhaust a cgroup thread budget at startup and abort under +/// `panic = "abort"`. A terminal UI is I/O-bound, so cap at 8. +const DEFAULT_MAX_WORKER_THREADS: NonZeroUsize = NonZeroUsize::new(8).unwrap(); +/// How `GROK_WORKER_THREADS` resolved. +#[derive(Debug, PartialEq, Eq)] +enum WorkerCount { + Accepted(NonZeroUsize), + Clamped { + requested: i128, + used: NonZeroUsize, + cores: NonZeroUsize, + }, + Ignored { + value: String, + used: NonZeroUsize, + }, +} +impl WorkerCount { + fn used(&self) -> NonZeroUsize { + match self { + Self::Accepted(used) | Self::Clamped { used, .. } | Self::Ignored { used, .. } => *used, + } + } + fn notice(&self) -> Option { + match self { + Self::Accepted(_) => None, + Self::Clamped { + requested, + used, + cores, + } => Some(format!( + "grok: clamped {GROK_WORKER_THREADS_ENV}={requested} to {used} (valid range is 1..={cores})" + )), + Self::Ignored { value, .. } => Some(format!( + "grok: ignoring {GROK_WORKER_THREADS_ENV}={value:?} (not a valid integer)" + )), + } + } +} +fn cli_worker_threads() -> NonZeroUsize { + let cores = std::thread::available_parallelism().unwrap_or(NonZeroUsize::MIN); + let resolved = match std::env::var(GROK_WORKER_THREADS_ENV) { + Ok(value) => worker_threads_from(Some(&value), cores), + Err(std::env::VarError::NotPresent) => worker_threads_from(None, cores), + Err(std::env::VarError::NotUnicode(value)) => WorkerCount::Ignored { + value: value.to_string_lossy().into_owned(), + used: default_worker_threads(cores), + }, + }; + if let Some(notice) = resolved.notice() { + eprintln!("{notice}"); + } + resolved.used() +} +fn worker_threads_from(env_override: Option<&str>, cores: NonZeroUsize) -> WorkerCount { + match env_override { + Some(value) => resolve_worker_override(value, cores), + None => WorkerCount::Accepted(default_worker_threads(cores)), + } +} +fn default_worker_threads(cores: NonZeroUsize) -> NonZeroUsize { + cores.min(DEFAULT_MAX_WORKER_THREADS) +} +fn resolve_worker_override(value: &str, cores: NonZeroUsize) -> WorkerCount { + let Ok(requested) = value.trim().parse::() else { + return WorkerCount::Ignored { + value: value.to_owned(), + used: default_worker_threads(cores), + }; + }; + let clamped = requested.clamp(1, cores.get() as i128) as usize; + let used = NonZeroUsize::new(clamped).expect("clamp floor of 1 guarantees non-zero"); + if requested == used.get() as i128 { + WorkerCount::Accepted(used) + } else { + WorkerCount::Clamped { + requested, + used, + cores, + } + } +} /// A plain runtime drop blocks forever on an uncancellable in-flight blocking /// task; `shutdown_timeout` abandons it after `grace` so exit can't hang. fn run_and_shutdown( @@ -1755,10 +1840,15 @@ fn main() { "Found crashed sessions from a previous run" ); } + let workers = cli_worker_threads(); let runtime = tokio::runtime::Builder::new_multi_thread() + .worker_threads(workers.get()) .enable_all() .build() - .unwrap_or_else(|e| panic!("failed to start tokio runtime: {e}")); + .unwrap_or_else(|e| { + eprintln!("grok: failed to start tokio runtime with {workers} workers: {e}"); + shutdown_and_flush_telemetry(1); + }); let result = run_and_shutdown(runtime, async_main(args), RUNTIME_SHUTDOWN_GRACE); xai_grok_telemetry::debug_log::flush(); if let Err(e) = result { @@ -2358,6 +2448,86 @@ async fn signal_leaders_to_relaunch(installed_version: &str) { mod tests { use super::*; #[test] + fn default_caps_the_core_count() { + let nz = |n| NonZeroUsize::new(n).unwrap(); + assert_eq!(default_worker_threads(nz(360)), DEFAULT_MAX_WORKER_THREADS); + assert_eq!(default_worker_threads(nz(4)), nz(4)); + } + #[test] + fn worker_threads_from_selects_default_or_override() { + let nz = |n| NonZeroUsize::new(n).unwrap(); + let cores = nz(360); + assert_eq!( + worker_threads_from(None, cores), + WorkerCount::Accepted(default_worker_threads(cores)) + ); + assert_eq!( + worker_threads_from(Some("16"), cores), + WorkerCount::Accepted(nz(16)) + ); + } + #[test] + fn override_in_range_is_used_without_a_notice() { + let nz = |n| NonZeroUsize::new(n).unwrap(); + let cores = nz(360); + assert_eq!( + resolve_worker_override("16", cores), + WorkerCount::Accepted(nz(16)) + ); + assert_eq!(resolve_worker_override("16", cores).notice(), None); + assert_eq!(resolve_worker_override(" 8 ", cores).used().get(), 8); + assert_eq!( + resolve_worker_override("360", cores), + WorkerCount::Accepted(cores) + ); + } + #[test] + fn override_out_of_range_is_clamped_with_a_notice() { + let nz = |n| NonZeroUsize::new(n).unwrap(); + let cores = nz(360); + assert_eq!( + resolve_worker_override("100000", cores), + WorkerCount::Clamped { + requested: 100000, + used: cores, + cores + } + ); + assert_eq!( + resolve_worker_override("0", cores), + WorkerCount::Clamped { + requested: 0, + used: nz(1), + cores + } + ); + assert_eq!( + resolve_worker_override("-1", cores), + WorkerCount::Clamped { + requested: -1, + used: nz(1), + cores + } + ); + assert_eq!( + resolve_worker_override("100000", cores).notice().unwrap(), + "grok: clamped GROK_WORKER_THREADS=100000 to 360 (valid range is 1..=360)" + ); + } + #[test] + fn override_unparseable_is_ignored_with_a_notice() { + let cores = NonZeroUsize::new(360).unwrap(); + for value in ["abc", "", "99999999999999999999999999999999999999999"] { + let ignored = resolve_worker_override(value, cores); + assert!(matches!(ignored, WorkerCount::Ignored { .. }), "{value}"); + assert_eq!(ignored.used(), default_worker_threads(cores), "{value}"); + } + assert_eq!( + resolve_worker_override("abc", cores).notice().unwrap(), + "grok: ignoring GROK_WORKER_THREADS=\"abc\" (not a valid integer)" + ); + } + #[test] fn version_output_writer_preserves_channel_aware_contract() { for (label, expected_suffix) in [ (" [alpha]", " [alpha]\n"), diff --git a/crates/codegen/xai-grok-pager-minimal/src/commit.rs b/crates/codegen/xai-grok-pager-minimal/src/commit.rs index 421ba78..7c20868 100644 --- a/crates/codegen/xai-grok-pager-minimal/src/commit.rs +++ b/crates/codegen/xai-grok-pager-minimal/src/commit.rs @@ -115,7 +115,15 @@ pub fn is_committable(entry: &ScrollbackEntry, turn_running: bool, is_last: bool } /// The display mode a block should be committed in (minimal mode, print-once). -pub fn minimal_commit_display_mode(block: &RenderBlock) -> DisplayMode { +/// +/// Stamps BOTH the entry being committed and the still-uncommitted live-tail +/// entries ([`commit_active`]), so a block's height is identical either side of +/// the commit frontier and the prompt does not jerk when it crosses. +pub fn minimal_commit_display_mode( + block: &RenderBlock, + appearance: &AppearanceConfig, +) -> DisplayMode { + let collapse_thinking = appearance.minimal_collapse_thinking; match block { RenderBlock::ToolCall(ToolCallBlock::Edit(_)) => DisplayMode::Expanded, RenderBlock::ToolCall( @@ -126,6 +134,7 @@ pub fn minimal_commit_display_mode(block: &RenderBlock) -> DisplayMode { | ToolCallBlock::IntegrationSearch(_)), ) if tc.is_success() => DisplayMode::Collapsed, RenderBlock::ToolCall(_) => DisplayMode::Truncated, + RenderBlock::Thinking(_) if collapse_thinking => DisplayMode::Collapsed, RenderBlock::Thinking(_) => DisplayMode::Expanded, _ => DisplayMode::Expanded, } @@ -245,37 +254,55 @@ pub fn commit_leading_run( /// on would make the reserved `insert_before` height disagree with the painted /// rows (design decision K5). Block horizontal padding is zeroed so committed /// content is flush-left with the welcome card (which paints edge-to-edge); -/// paired with [`EntryRenderer::with_hide_accent`] reclaiming the accent -/// column, glyphs start at column 0. The live region's prompt / status / -/// info rows mirror that via [`super::live::live_left_inset`]. +/// paired with [`minimal_renderer`] reclaiming the accent column, glyphs start +/// at column 0. The live region's prompt / status / info rows mirror that via +/// [`super::live::live_left_inset`]. +/// +/// The two reasoning-legibility toggles are set here rather than in +/// `pager.toml` so the full TUI stays provably untouched — design doc §6.16. pub(crate) fn committed_appearance(base: &AppearanceConfig) -> AppearanceConfig { let mut a = base.clone(); a.show_timestamps = false; - // Flush-left minimal look: no block horizontal padding (align with the - // welcome card, which paints edge-to-edge with no outer h-pad). a.scrollback.layout.block_pad_left = 0; a.scrollback.layout.block_pad_right = 0; + a.scrollback.blocks.thinking.body_dim_italic = true; + a.scrollback.blocks.thinking.collapsed_expand_hint = true; a } -/// Build the renderer used for a committed (print-once) block: no selection -/// highlight, a static tick (no running-wave animation), timestamps off. -fn committed_renderer<'a>( +pub(crate) const COMMITTED_TICK: u64 = 0; + +/// The renderer for one minimal-mode entry, on **either** side of the commit +/// frontier — `tick` is the only difference. Chrome here decides a block's +/// wrapped height, so both sides must agree or the prompt jumps on commit (K5); +/// keeping it one constructor is what makes that unbreakable. +/// +/// Reasoning alone keeps the accent column, as the marker that separates it +/// from the answer. Design doc §6.16. +pub(crate) fn minimal_renderer<'a>( entry: &'a ScrollbackEntry, theme: &'a Theme, appearance: AppearanceConfig, cwd: &'a std::path::Path, + tick: u64, ) -> EntryRenderer<'a> { + // Reserved only where it is actually painted: `ThinkingBlock::accent` + // returns `None` when collapsed, and reserving a column nothing paints + // would indent the header over a blank gutter. Collapsed reasoning has no + // body to delimit anyway — the folded `Thought for Xs` header cannot be + // mistaken for the answer. `only_thinking_spends_the_accent_column` pins + // reserved == painted so the two rules cannot drift apart. + let hide_accent = !matches!(entry.block, RenderBlock::Thinking(_)) + || entry.display_mode() == DisplayMode::Collapsed; EntryRenderer::new(entry, theme) .with_appearance(appearance) .with_cwd(Some(cwd)) - .with_tick(0) - // Blend committed blocks with the real terminal background (no - // user-message `bg_light` band etc.). + .with_tick(tick) .with_flat_background(true) - // Drop the left accent bar for a cleaner, un-gutter'd minimal look; the - // per-block `◆`/bullet marker still reads the block boundary. - .with_hide_accent(true) + .with_hide_accent(hide_accent) + // The accent resolves to `Color::Reset` under the terminal-native + // palette — full-brightness default fg, which would shout. + .with_dim_accent(true) } /// Emit one committed block into native scrollback via `insert_before`, capping @@ -430,7 +457,7 @@ pub fn commit_active(app: &mut AppView, terminal: &mut PagerTerminal) { } // Stamp the print-once display mode before measuring/rendering. if let Some(e) = sb.get_mut(i) { - let mode = minimal_commit_display_mode(&e.block); + let mode = minimal_commit_display_mode(&e.block, &appearance); e.set_display_mode(mode); } if let Some(e) = sb.get(i) { @@ -444,7 +471,7 @@ pub fn commit_active(app: &mut AppView, terminal: &mut PagerTerminal) { // place later (`get_by_id_mut` + edit, the `/recap` fill pattern) // will NOT reach the screen — append a fresh block instead (see // the `SessionRecap` handler in `acp_handler.rs`). - let renderer = committed_renderer(e, &theme, appearance.clone(), cwd); + let renderer = minimal_renderer(e, &theme, appearance.clone(), cwd, COMMITTED_TICK); if insert_committed(terminal, renderer, width, max_rows, footer_style).is_err() { return false; } @@ -470,7 +497,7 @@ pub fn commit_active(app: &mut AppView, terminal: &mut PagerTerminal) { // commit. Idempotent: `set_display_mode` no-ops when unchanged. let mut j = minimal_api::commit_scan_cursor(sb); while let Some(e) = sb.get_mut(j) { - let mode = minimal_commit_display_mode(&e.block); + let mode = minimal_commit_display_mode(&e.block, &appearance); e.set_display_mode(mode); j += 1; } @@ -538,7 +565,7 @@ pub fn expand_pending(app: &mut AppView, terminal: &mut PagerTerminal) { e.set_display_mode(DisplayMode::Expanded); } if let Some(e) = sb.get(idx) { - let renderer = committed_renderer(e, &theme, appearance.clone(), cwd); + let renderer = minimal_renderer(e, &theme, appearance.clone(), cwd, COMMITTED_TICK); if insert_committed(terminal, renderer, width, 0, footer_style).is_err() { // Terminal write failed: keep this id and the rest queued // so the request retries next frame instead of vanishing. @@ -573,823 +600,5 @@ pub fn sync_pending_marks(app: &mut AppView) { } #[cfg(test)] -mod tests { - use super::*; - use ratatui::style::Color; - use xai_grok_pager::scrollback::block::RenderBlock; - use xai_grok_pager::scrollback::entry::ScrollbackEntry; - use xai_grok_pager::scrollback::state::ScrollbackState; - - fn test_cwd() -> &'static std::path::Path { - std::path::Path::new("/test/session") - } - - fn finalized(text: &str) -> ScrollbackEntry { - ScrollbackEntry::new(RenderBlock::stub(text, Color::Blue)) - } - - fn running(text: &str) -> ScrollbackEntry { - ScrollbackEntry::running(RenderBlock::stub(text, Color::Blue)) - } - - /// Run a commit pass for a RUNNING turn, returning the emitted indices. - fn commit_collect(state: &mut ScrollbackState) -> Vec { - let mut seen = Vec::new(); - commit_leading_run(state, true, |_, i| { - seen.push(i); - true - }); - seen - } - - #[test] - fn commits_leading_finalized_run_and_stops_at_running() { - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(finalized("b")); - s.push(running("c")); - s.push(finalized("d")); // after the running block — must NOT commit yet - - assert_eq!(commit_collect(&mut s), vec![0, 1]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 2); - assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); - assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); - assert!(!minimal_api::is_committed(&s, s.get(2).unwrap())); - assert!(!minimal_api::is_committed(&s, s.get(3).unwrap())); - - // Finalize "c"; the next pass commits "c" then "d". - s.get_mut(2).unwrap().mark_completed(); - assert_eq!(commit_collect(&mut s), vec![2, 3]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 4); - } - - #[test] - fn pending_user_input_holds_the_frontier() { - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - let tool = s.push(finalized("tool")); // finalized but awaiting permission - s.push(finalized("after")); - assert!(s.set_pending_user_input(tool, true)); - - // Stops before the pending tool, even though it (and "after") are finalized. - assert_eq!(commit_collect(&mut s), vec![0]); - - // Resolving the prompt releases the rest of the run. - assert!(s.set_pending_user_input(tool, false)); - assert_eq!(commit_collect(&mut s), vec![1, 2]); - } - - #[test] - fn running_agent_message_commits_once_a_later_block_exists() { - // The tracker leaves an agent message's `is_running` flag set until turn - // end (handle_tool_call resets current_agent_msg without finishing the - // entry when a tool follows). Minimal must still commit that message - // mid-turn once a later block proves it's complete — otherwise the rest - // of the turn piles up in the fixed-height live tail and scrolls instead - // of accumulating into native scrollback. - let mut s = ScrollbackState::new(); - s.push(ScrollbackEntry::running(RenderBlock::agent_message( - "answer text", - ))); - - // While it's the last entry it may still be streaming → stays live. - assert_eq!(commit_collect(&mut s), Vec::::new()); - assert_eq!(minimal_api::commit_scan_cursor(&s), 0); - - // A later block (the tracker moved on) proves the message is done → it - // commits even though its is_running flag still lingers. The new - // last/running entry stays in the live tail. - s.push(running("tool")); - assert_eq!(commit_collect(&mut s), vec![0]); - assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); - assert!(!minimal_api::is_committed(&s, s.get(1).unwrap())); - } - - #[test] - fn running_tool_still_holds_the_frontier_even_with_a_later_block() { - // The agent-message relaxation must NOT extend to tools: a running tool - // can still update its result, so committing it (print-once) would lose - // the update. It holds the frontier regardless of later blocks. - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(running("running tool")); // stub == not an AgentMessage - s.push(finalized("after")); - assert_eq!(commit_collect(&mut s), vec![0]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 1); - } - - #[test] - fn bg_task_started_commits_while_running_and_does_not_wedge_frontier() { - // A fresh background task is pushed as a running "started" block - // (`set_last_running(true)`). Its `is_running` flag is animation-only — - // the block is a finalized lifecycle event whose content never changes — - // so it must commit immediately even mid-turn. Otherwise it wedges the - // frontier and the task (plus everything after it) stays hidden in the - // live tail until the task finishes (the reported dogfood bug). - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(ScrollbackEntry::running(RenderBlock::bg_task( - "sleep 60", "task-1", - ))); - s.push(running("later tool")); // more turn output after the bg task - - // "a" + the running bg task commit; only the trailing running tool stays. - assert_eq!(commit_collect(&mut s), vec![0, 1]); - assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); - assert!(!minimal_api::is_committed(&s, s.get(2).unwrap())); - } - - #[test] - fn bg_task_started_commits_as_last_running_entry() { - // Even as the last entry of a still-running turn the bg "started" block - // commits — a lifecycle block never streams more content (completion is - // a separate block). - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(ScrollbackEntry::running(RenderBlock::bg_task( - "sleep 60", "task-1", - ))); - assert_eq!(commit_collect(&mut s), vec![0, 1]); - } - - #[test] - fn no_double_commit_after_mid_list_shift_remove() { - let mut s = ScrollbackState::new(); - let a = s.push(finalized("a")); - s.push(finalized("b")); - s.push(finalized("c")); - assert_eq!(commit_collect(&mut s), vec![0, 1, 2]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 3); - - // Remove an already-committed entry below the cursor (shift_remove shifts - // the remaining indices down). The cursor is clamped; the per-entry - // `committed` flags travel with "b"/"c", so neither is re-emitted. - assert!(s.remove_entry(a)); - s.push(finalized("d")); // now at index 2 - - assert_eq!(commit_collect(&mut s), vec![2]); - assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); // b - assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); // c - assert!(minimal_api::is_committed(&s, s.get(2).unwrap())); // d - } - - #[test] - fn mid_list_removal_below_cursor_does_not_strand_uncommitted_entries() { - // Regression (review bug 2): a committed placeholder ("Loading - // session...") is removed AFTER new uncommitted entries were appended - // past the cursor — the `/resume` / reconnect `SessionLoaded` ordering. - // Removing below the cursor shifts the uncommitted entries down one; - // without the cursor decrement in `remove_entry` the first of them - // slid below the cursor and was never committed NOR drawn in the live - // tail (silently missing from minimal mode). - let mut s = ScrollbackState::new(); - s.push(finalized("old-1")); - let placeholder = s.push(finalized("Loading session...")); - - // A draw commits both; cursor = 2. - let mut seen = Vec::new(); - commit_leading_run(&mut s, false, |_, i| { - seen.push(i); - true - }); - assert_eq!(seen, vec![0, 1]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 2); - - // Replay appends entries, then the placeholder is removed in the same - // event cycle (before the next commit pass). - s.push(finalized("replayed-A")); - s.push(finalized("replayed-B")); - assert!(s.remove_entry(placeholder)); - // The cursor moved down with the shifted entries. - assert_eq!(minimal_api::commit_scan_cursor(&s), 1); - - // The next pass commits BOTH replayed entries — none stranded. - let mut seen = Vec::new(); - commit_leading_run(&mut s, false, |_, i| { - seen.push(i); - true - }); - assert_eq!(seen, vec![1, 2]); - assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); // replayed-A - assert!(minimal_api::is_committed(&s, s.get(2).unwrap())); // replayed-B - } - - #[test] - fn pending_user_input_holds_the_frontier_even_when_idle() { - // A block awaiting a permission / question answer must never commit, - // even if the turn state reads idle (e.g. a prompt outliving its turn): - // its rendered form still changes when the prompt resolves, and a - // committed copy is frozen. The idle relaxation only applies to - // *stale-running* flags, not pending-input marks. - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - let tool = s.push(finalized("tool")); - assert!(s.set_pending_user_input(tool, true)); - - let mut seen = Vec::new(); - commit_leading_run(&mut s, false, |_, i| { - seen.push(i); - true - }); - assert_eq!(seen, vec![0], "pending entry must hold the frontier"); - assert!(!minimal_api::is_committed(&s, s.get(1).unwrap())); - - // Resolving the prompt releases it. - assert!(s.set_pending_user_input(tool, false)); - let mut seen = Vec::new(); - commit_leading_run(&mut s, false, |_, i| { - seen.push(i); - true - }); - assert_eq!(seen, vec![1]); - } - - #[test] - fn failed_emit_leaves_entry_uncommitted_for_retry() { - // Regression (bugbot "Committed flag set on IO failure"): a terminal - // write failure must NOT mark the entry committed — print-once means a - // marked-but-unprinted block can never be emitted again. The walk stops - // with the cursor before the failed entry and retries next frame. - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(finalized("b")); - - // First pass: the emit fails on the first entry. - let mut calls = 0usize; - let n = commit_leading_run(&mut s, false, |_, _| { - calls += 1; - false - }); - assert_eq!(n, 0, "nothing committed on failure"); - assert_eq!(calls, 1, "walk stops at the first failure"); - assert!(!minimal_api::is_committed(&s, s.get(0).unwrap())); - assert_eq!(minimal_api::commit_scan_cursor(&s), 0, "cursor holds"); - - // Retry pass succeeds and commits both. - let n = commit_leading_run(&mut s, false, |_, _| true); - assert_eq!(n, 2); - assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); - assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); - } - - #[test] - fn scan_frontier_mirrors_commit_leading_run() { - // `scan_frontier` (read-only: viewport sizing + the will-commit gate) - // must agree exactly with the mutating walk, in every phase. - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(finalized("b")); - s.push(running("c")); - s.push(finalized("d")); - - // Pre-commit: the pass would commit a+b and stop at the running entry. - let scan = scan_frontier(&s, true); - assert!(scan.will_commit); - assert_eq!(scan.tail_start, 2); - - let n = commit_leading_run(&mut s, true, |_, _| true); - assert_eq!(n, 2); - assert_eq!(minimal_api::commit_scan_cursor(&s), scan.tail_start); - - // Post-commit: nothing left to commit; the tail starts at the cursor. - let scan = scan_frontier(&s, true); - assert!(!scan.will_commit); - assert_eq!(scan.tail_start, 2); - - // Idle with no entries pending: everything committable. - let scan = scan_frontier(&s, false); - assert!(scan.will_commit); - assert_eq!(scan.tail_start, 4); - } - - #[test] - fn remove_from_below_frontier_then_push_still_commits() { - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(finalized("b")); - s.push(finalized("c")); - assert_eq!(commit_collect(&mut s), vec![0, 1, 2]); - - // Rewind: drop everything from index 1 (keep only "a"). Without the - // cursor clamp this would strand the cursor at 3 and silently skip the - // next pushes. - let removed = s.remove_from(1); - assert_eq!(removed.len(), 2); - assert_eq!(minimal_api::commit_scan_cursor(&s), 1); - - s.push(finalized("d")); // index 1 - assert_eq!(commit_collect(&mut s), vec![1]); - } - - #[test] - fn btw_block_emits_once_across_repeated_frontier_passes() { - let mut s = ScrollbackState::new(); - s.push(ScrollbackEntry::new(RenderBlock::Btw( - xai_grok_pager::scrollback::blocks::BtwBlock::new( - "original question", - "original answer", - ), - ))); - - let mut emitted = Vec::new(); - assert_eq!( - commit_leading_run(&mut s, false, |state, i| { - let RenderBlock::Btw(block) = &state.get(i).unwrap().block else { - panic!("expected Btw block") - }; - assert_eq!(block.question, "original question"); - assert_eq!(block.content().text(), "original answer"); - emitted.push(i); - true - }), - 1 - ); - assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); - - assert_eq!( - commit_leading_run(&mut s, false, |_, i| { - emitted.push(i); - true - }), - 0 - ); - assert_eq!(emitted, vec![0]); - assert!(!scan_frontier(&s, false).will_commit); - } - - #[test] - fn commit_leading_run_advances_frontier_and_marks_committed_once() { - let mut s = ScrollbackState::new(); - s.push(finalized("h1")); - s.push(finalized("h2")); - s.push(finalized("h3")); - - // Advances the frontier, marking the leading finalized run committed. - let mut emitted = Vec::new(); - let n = commit_leading_run(&mut s, false, |_, i| { - emitted.push(i); - true - }); - assert_eq!(n, 3); - assert_eq!(emitted, vec![0, 1, 2]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 3); - assert!((0..3).all(|i| minimal_api::is_committed(&s, s.get(i).unwrap()))); - - // A second pass commits nothing (already-committed entries are skipped). - let mut again = Vec::new(); - commit_leading_run(&mut s, false, |_, i| { - again.push(i); - true - }); - assert!(again.is_empty()); - } - - #[test] - fn idle_turn_commits_past_stale_running_entry() { - // Regression for the missing-edit/stuck-spinner bug: the agent tracker - // can leave an entry's `is_running` flag set after the turn ends (e.g. a - // thinking block whose finalize was missed at the thinking→tool - // transition). While the turn runs, that entry correctly holds the - // frontier; once the turn is idle the frontier must advance past it. - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - s.push(running("stale")); // stale is_running flag - s.push(finalized("c")); - - // Running turn: blocked at the running entry. - let mut seen = Vec::new(); - commit_leading_run(&mut s, true, |_, i| { - seen.push(i); - true - }); - assert_eq!(seen, vec![0]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 1); - - // Idle turn: commit everything past the stale flag. - let mut seen = Vec::new(); - commit_leading_run(&mut s, false, |_, i| { - seen.push(i); - true - }); - assert_eq!(seen, vec![1, 2]); - assert_eq!(minimal_api::commit_scan_cursor(&s), 3); - } - - #[test] - fn clear_resets_the_frontier() { - let mut s = ScrollbackState::new(); - s.push(finalized("a")); - commit_collect(&mut s); - assert_eq!(minimal_api::commit_scan_cursor(&s), 1); - - s.clear(); - assert_eq!(minimal_api::commit_scan_cursor(&s), 0); - } - - /// Height-exactness guard (design K5 / risk #1). `commit_active` reserves - /// exactly `desired_height(width)` rows via `insert_before`; if `render` - /// paints real content beyond that, those rows are silently clipped (lost) - /// from native scrollback. Render each block type into an over-tall buffer - /// and assert no non-space glyph lands past `desired_height`. (Background - /// fill of blank spaces past `h` is fine — only real content matters.) - fn assert_committed_fits(label: &str, block: RenderBlock, width: u16) { - use ratatui::buffer::Buffer; - use ratatui::layout::Rect; - - let mut entry = ScrollbackEntry::new(block); - entry.set_display_mode(minimal_commit_display_mode(&entry.block)); - let theme = Theme::current(); - let appearance = committed_appearance(&AppearanceConfig::default()); - - let renderer = committed_renderer(&entry, &theme, appearance, test_cwd()); - let h = renderer.desired_height(width); - assert!(h > 0, "{label}@{width}: desired_height was 0"); - // The accent bar and background fill intentionally stretch to the given - // area height (chrome, not content). Only the content columns - // (x >= chrome_width) carry real text that `insert_before` would clip. - let chrome = renderer.chrome_width(); - - let extra = 8u16; - let area = Rect::new(0, 0, width, h + extra); - let mut buf = Buffer::empty(area); - renderer.render(area, &mut buf); - - for y in h..(h + extra) { - for x in chrome..width { - let sym = buf.cell((x, y)).map(|c| c.symbol()).unwrap_or(" "); - assert!( - sym.trim().is_empty(), - "{label}@{width}: content {sym:?} painted at row {y} col {x}, past \ - desired_height {h} — insert_before would clip it from scrollback" - ); - } - } - } - - #[test] - fn committed_renderer_uses_owning_session_cwd_for_tool_paths() { - use ratatui::buffer::Buffer; - - let cwd = std::path::Path::new("/alternate/worktree"); - let mut entry = - ScrollbackEntry::new(RenderBlock::edit("/alternate/worktree/src/main.rs", None)); - entry.set_display_mode(DisplayMode::Expanded); - let theme = Theme::current(); - let appearance = committed_appearance(&AppearanceConfig::default()); - let renderer = committed_renderer(&entry, &theme, appearance, cwd); - let width = 80; - let height = renderer.desired_height(width); - let area = Rect::new(0, 0, width, height); - let mut buf = Buffer::empty(area); - renderer.render(area, &mut buf); - - let mut text = String::new(); - for y in 0..height { - for x in 0..width { - text.push_str(buf[(x, y)].symbol()); - } - } - assert!(text.contains("src/main.rs"), "rendered text: {text:?}"); - assert!( - !text.contains("/alternate/worktree"), - "session prefix should be elided: {text:?}" - ); - } - - #[test] - fn committed_blocks_fit_desired_height() { - // Thinking blocks render zero rows unless `show_thinking_blocks` is on. - // The toggle is a thread-local, so pin it on here so the thinking - // block's committed height is actually exercised. - minimal_api::set_show_thinking_blocks(true); - - let long = "Hello there — this is a longer message that should wrap across \ - several lines at narrow widths to exercise the wrapping math, with \ - enough words to overflow eighty columns comfortably."; - for width in [40u16, 80, 120] { - assert_committed_fits("user_prompt", RenderBlock::user_prompt(long), width); - assert_committed_fits( - "agent_message", - RenderBlock::agent_message(format!( - "{long}\n\n- bullet one\n- bullet two\n\n```rust\nfn main() {{}}\n```" - )), - width, - ); - assert_committed_fits("thinking", RenderBlock::thinking(long), width); - assert_committed_fits( - "execute", - RenderBlock::execute_with_output( - "cargo build --release", - "line 1\nline 2\nline 3\nline 4\nline 5\nline 6", - None::, - ), - width, - ); - assert_committed_fits("edit", RenderBlock::edit("src/main.rs", None), width); - assert_committed_fits("read", RenderBlock::read("src/main.rs", None), width); - assert_committed_fits( - "list_dir", - RenderBlock::list_dir_with_output("src", "a.rs\nb.rs\nc.rs"), - width, - ); - assert_committed_fits("search", RenderBlock::search("TODO", 0, vec![]), width); - assert_committed_fits("system", RenderBlock::system("Session restored"), width); - assert_committed_fits("bg_task", RenderBlock::bg_task("sleep 30", "task-1"), width); - } - } - - /// Regression pinned: `md_style::to_anstyle` used to map `Color::Reset` - /// to a concrete ANSI-7 silver, washing out assistant/thinking markdown - /// body text on light terminals; `highlight_bash_command` leaked raw - /// syntect RGB. - #[test] - fn terminal_native_lock_paints_only_native_colors() { - use ratatui::buffer::Buffer; - use xai_grok_pager::theme::cache as theme_cache; - - let _guard = theme_cache::test_lock() - .lock() - .unwrap_or_else(|e| e.into_inner()); - struct LockReset; - impl Drop for LockReset { - fn drop(&mut self) { - xai_grok_pager::theme::cache::set_terminal_native_lock(false); - } - } - let _reset = LockReset; - theme_cache::set_terminal_native_lock(true); - minimal_api::set_show_thinking_blocks(true); - - let md = "Intro paragraph with **bold**, _italic_, `inline code`, and a \ - [link](https://example.com).\n\n# Heading one\n\n## Heading two\n\n\ - - item one\n- item two\n\n> a quote\n\n```rust\nfn main() { println!(\"hi\"); }\n```"; - use similar::ChangeTag; - let hunk = vec![ - xai_grok_pager::diff::DiffLine { - text: "let x = 1;\n".into(), - lo: 1, - ln: 1, - tag: ChangeTag::Equal, - }, - xai_grok_pager::diff::DiffLine { - text: "let y = 2;\n".into(), - lo: 2, - ln: 0, - tag: ChangeTag::Delete, - }, - xai_grok_pager::diff::DiffLine { - text: "let y = 3;\n".into(), - lo: 0, - ln: 2, - tag: ChangeTag::Insert, - }, - ]; - let blocks = vec![ - ("agent_message", RenderBlock::agent_message(md)), - ( - "thinking", - RenderBlock::thinking("Weighing the *options* with `care`."), - ), - ("user_prompt", RenderBlock::user_prompt("run the tests")), - ( - "edit", - RenderBlock::edit_with_hunks("src/main.rs", vec![hunk]), - ), - ( - "execute", - RenderBlock::execute_with_output("cargo build", "ok\n", None::), - ), - ("system", RenderBlock::system("Session restored")), - ]; - - for (label, block) in blocks { - let mut entry = ScrollbackEntry::new(block); - entry.set_display_mode(minimal_commit_display_mode(&entry.block)); - let theme = Theme::current(); - let appearance = committed_appearance(&AppearanceConfig::default()); - let renderer = committed_renderer(&entry, &theme, appearance, test_cwd()); - - let width = 100u16; - let h = renderer.desired_height(width).max(1); - let area = Rect::new(0, 0, width, h); - let mut buf = Buffer::empty(area); - renderer.render(area, &mut buf); - - for y in 0..h { - for x in 0..width { - let Some(cell) = buf.cell((x, y)) else { - continue; - }; - for (which, c) in [("fg", cell.fg), ("bg", cell.bg)] { - assert!( - !matches!(c, Color::Rgb(..) | Color::Indexed(_)), - "{label}: non-native {which} {c:?} at ({x},{y}) under \ - symbol {:?} — minimal must only use Reset / named \ - ANSI-16 so the terminal palette controls rendering", - cell.symbol() - ); - } - } - } - } - } - - #[test] - fn large_commit_is_capped_with_footer() { - use ratatui::buffer::Buffer; - use ratatui::layout::Rect; - - let theme = Theme::current(); - let appearance = committed_appearance(&AppearanceConfig::default()); - // A tall block: a fenced code block keeps each line on its own row - // (markdown would otherwise join soft-wrapped prose into one paragraph), - // so the block is comfortably taller than the cap. - let lines: Vec = (0..60).map(|i| format!("line {i}")).collect(); - let body = format!("```\n{}\n```", lines.join("\n")); - let mut entry = ScrollbackEntry::new(RenderBlock::agent_message(body)); - entry.set_display_mode(minimal_commit_display_mode(&entry.block)); - - let width = 80u16; - let renderer = committed_renderer(&entry, &theme, appearance, test_cwd()); - let full_h = renderer.desired_height(width); - assert!(full_h > 12, "expected a tall block, got {full_h}"); - - // Paint into a cap-height buffer (what `insert_committed` allocates). - let cap = 12u16; - let area = Rect::new(0, 0, width, cap); - let mut buf = Buffer::empty(area); - paint_committed(&mut buf, renderer, width, full_h, theme.dim()); - - // The final row is the overflow footer naming the hidden line count and - // pointing at /transcript; the buffer is exactly `cap` rows (bounded). - let last: String = (0..width) - .filter_map(|x| buf.cell((x, cap - 1)).map(|c| c.symbol().to_string())) - .collect(); - assert!(last.contains("more lines"), "footer row: {last:?}"); - assert!(last.contains("/transcript"), "footer row: {last:?}"); - // A hidden-line count is present (full_h minus the kept content rows). - let hidden = full_h - (cap - 1); - assert!( - last.contains(&hidden.to_string()), - "footer should name {hidden} hidden lines: {last:?}" - ); - } - - #[test] - fn small_commit_is_not_capped() { - use ratatui::buffer::Buffer; - use ratatui::layout::Rect; - - let theme = Theme::current(); - let appearance = committed_appearance(&AppearanceConfig::default()); - let mut entry = ScrollbackEntry::new(RenderBlock::agent_message("one short line")); - entry.set_display_mode(minimal_commit_display_mode(&entry.block)); - - let width = 80u16; - let renderer = committed_renderer(&entry, &theme, appearance, test_cwd()); - let full_h = renderer.desired_height(width); - - // Buffer is exactly the block's height → no footer (uncapped path). - let area = Rect::new(0, 0, width, full_h); - let mut buf = Buffer::empty(area); - paint_committed(&mut buf, renderer, width, full_h, theme.dim()); - - let mut all = String::new(); - for y in 0..full_h { - for x in 0..width { - all.push_str(buf.cell((x, y)).map(|c| c.symbol()).unwrap_or(" ")); - } - } - assert!( - !all.contains("more lines"), - "no footer when uncapped: {all:?}" - ); - } - - #[test] - fn committed_edit_keeps_diff_line_backgrounds() { - use ratatui::buffer::Buffer; - use ratatui::layout::Rect; - use similar::ChangeTag; - use xai_grok_pager::diff::DiffLine; - - let hunk = vec![ - DiffLine { - text: "let x = 1;\n".into(), - lo: 10, - ln: 10, - tag: ChangeTag::Equal, - }, - DiffLine { - text: "let y = 2;\n".into(), - lo: 11, - ln: 0, - tag: ChangeTag::Delete, - }, - DiffLine { - text: "let y = 3;\n".into(), - lo: 0, - ln: 11, - tag: ChangeTag::Insert, - }, - DiffLine { - text: "let z = 4;\n".into(), - lo: 12, - ln: 12, - tag: ChangeTag::Equal, - }, - ]; - let block = RenderBlock::edit_with_hunks("src/main.rs", vec![hunk]); - let mut entry = ScrollbackEntry::new(block); - entry.set_display_mode(minimal_commit_display_mode(&entry.block)); - let theme = Theme::current(); - let appearance = committed_appearance(&AppearanceConfig::default()); - let renderer = committed_renderer(&entry, &theme, appearance, test_cwd()); - - let width = 80u16; - let h = renderer.desired_height(width); - let area = Rect::new(0, 0, width, h); - let mut buf = Buffer::empty(area); - renderer.render(area, &mut buf); - - // The committed edit uses a flat background (terminal transparency), but - // must still paint the per-line diff backgrounds — otherwise an added / - // removed line is indistinguishable from context. - let mut saw_insert = false; - let mut saw_delete = false; - for y in 0..h { - for x in 0..width { - if let Some(cell) = buf.cell((x, y)) { - saw_insert |= cell.bg == theme.diff_insert_bg; - saw_delete |= cell.bg == theme.diff_delete_bg; - } - } - } - assert!( - saw_insert, - "committed edit lost the insert (green) diff background" - ); - assert!( - saw_delete, - "committed edit lost the delete (red) diff background" - ); - } - - #[test] - fn commit_display_mode_policy() { - assert_eq!( - minimal_commit_display_mode(&RenderBlock::thinking("reasoning")), - DisplayMode::Expanded - ); - assert_eq!( - minimal_commit_display_mode(&RenderBlock::edit("file.rs", None)), - DisplayMode::Expanded - ); - assert_eq!( - minimal_commit_display_mode(&RenderBlock::execute("ls")), - DisplayMode::Truncated - ); - assert_eq!( - minimal_commit_display_mode(&RenderBlock::agent_message("hi")), - DisplayMode::Expanded - ); - } - - #[test] - fn commit_display_mode_lookups_collapse_on_success_only() { - use xai_grok_pager::scrollback::blocks::{ - ListDirToolCallBlock, ReadToolCallBlock, SearchToolCallBlock, - }; - - assert_eq!( - minimal_commit_display_mode(&RenderBlock::search("pat", 3, vec![])), - DisplayMode::Collapsed - ); - assert_eq!( - minimal_commit_display_mode(&RenderBlock::read("src/lib.rs", None)), - DisplayMode::Collapsed - ); - assert_eq!( - minimal_commit_display_mode(&RenderBlock::list_dir_with_output("src", "a.rs\nb.rs")), - DisplayMode::Collapsed - ); - - for failed in [ - RenderBlock::ToolCall(ToolCallBlock::Search( - SearchToolCallBlock::new("pat").with_error("regex parse error"), - )), - RenderBlock::ToolCall(ToolCallBlock::Read( - ReadToolCallBlock::new("gone.rs").with_error("file not found"), - )), - RenderBlock::ToolCall(ToolCallBlock::ListDir( - ListDirToolCallBlock::new("gone/").with_error("no such directory"), - )), - ] { - assert_eq!( - minimal_commit_display_mode(&failed), - DisplayMode::Truncated, - "failed lookup must stay truncated: {failed:?}" - ); - } - } -} +#[path = "commit_tests.rs"] +mod tests; diff --git a/crates/codegen/xai-grok-pager-minimal/src/commit_tests.rs b/crates/codegen/xai-grok-pager-minimal/src/commit_tests.rs new file mode 100644 index 0000000..3678022 --- /dev/null +++ b/crates/codegen/xai-grok-pager-minimal/src/commit_tests.rs @@ -0,0 +1,1158 @@ +//! Unit tests for [`super`] — minimal mode's commit pipeline. +//! +//! Split out of `commit.rs` to keep the production module scannable; the +//! `#[path]` attribute there keeps this a plain child module (`super::*` still +//! reaches the private items under test). + +use super::*; +use ratatui::style::Color; +use xai_grok_pager::scrollback::block::RenderBlock; +use xai_grok_pager::scrollback::entry::ScrollbackEntry; +use xai_grok_pager::scrollback::state::ScrollbackState; + +fn test_cwd() -> &'static std::path::Path { + std::path::Path::new("/test/session") +} + +fn finalized(text: &str) -> ScrollbackEntry { + ScrollbackEntry::new(RenderBlock::stub(text, Color::Blue)) +} + +fn running(text: &str) -> ScrollbackEntry { + ScrollbackEntry::running(RenderBlock::stub(text, Color::Blue)) +} + +fn default_appearance() -> AppearanceConfig { + committed_appearance(&AppearanceConfig::default()) +} + +fn collapsed_appearance() -> AppearanceConfig { + committed_appearance(&AppearanceConfig { + minimal_collapse_thinking: true, + ..Default::default() + }) +} + +/// Run a commit pass for a RUNNING turn, returning the emitted indices. +fn commit_collect(state: &mut ScrollbackState) -> Vec { + let mut seen = Vec::new(); + commit_leading_run(state, true, |_, i| { + seen.push(i); + true + }); + seen +} + +#[test] +fn commits_leading_finalized_run_and_stops_at_running() { + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(finalized("b")); + s.push(running("c")); + s.push(finalized("d")); // after the running block — must NOT commit yet + + assert_eq!(commit_collect(&mut s), vec![0, 1]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 2); + assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); + assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); + assert!(!minimal_api::is_committed(&s, s.get(2).unwrap())); + assert!(!minimal_api::is_committed(&s, s.get(3).unwrap())); + + // Finalize "c"; the next pass commits "c" then "d". + s.get_mut(2).unwrap().mark_completed(); + assert_eq!(commit_collect(&mut s), vec![2, 3]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 4); +} + +#[test] +fn pending_user_input_holds_the_frontier() { + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + let tool = s.push(finalized("tool")); // finalized but awaiting permission + s.push(finalized("after")); + assert!(s.set_pending_user_input(tool, true)); + + // Stops before the pending tool, even though it (and "after") are finalized. + assert_eq!(commit_collect(&mut s), vec![0]); + + // Resolving the prompt releases the rest of the run. + assert!(s.set_pending_user_input(tool, false)); + assert_eq!(commit_collect(&mut s), vec![1, 2]); +} + +#[test] +fn running_agent_message_commits_once_a_later_block_exists() { + // The tracker leaves an agent message's `is_running` flag set until turn + // end (handle_tool_call resets current_agent_msg without finishing the + // entry when a tool follows). Minimal must still commit that message + // mid-turn once a later block proves it's complete — otherwise the rest + // of the turn piles up in the fixed-height live tail and scrolls instead + // of accumulating into native scrollback. + let mut s = ScrollbackState::new(); + s.push(ScrollbackEntry::running(RenderBlock::agent_message( + "answer text", + ))); + + // While it's the last entry it may still be streaming → stays live. + assert_eq!(commit_collect(&mut s), Vec::::new()); + assert_eq!(minimal_api::commit_scan_cursor(&s), 0); + + // A later block (the tracker moved on) proves the message is done → it + // commits even though its is_running flag still lingers. The new + // last/running entry stays in the live tail. + s.push(running("tool")); + assert_eq!(commit_collect(&mut s), vec![0]); + assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); + assert!(!minimal_api::is_committed(&s, s.get(1).unwrap())); +} + +#[test] +fn running_tool_still_holds_the_frontier_even_with_a_later_block() { + // The agent-message relaxation must NOT extend to tools: a running tool + // can still update its result, so committing it (print-once) would lose + // the update. It holds the frontier regardless of later blocks. + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(running("running tool")); // stub == not an AgentMessage + s.push(finalized("after")); + assert_eq!(commit_collect(&mut s), vec![0]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 1); +} + +#[test] +fn plan_body_anchored_above_a_parked_tool_commits_while_it_is_still_running() { + let mut s = ScrollbackState::new(); + s.push(finalized("user prompt")); + let tool = s.push(running("exit_plan_mode")); // parked on the decision + s.insert_block_before(tool, RenderBlock::agent_message("PLAN BODY")); + + // Prompt + plan commit; the running tool row still holds the frontier. + assert_eq!(commit_collect(&mut s), vec![0, 1]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 2); + assert!(matches!( + s.get(1).map(|e| &e.block), + Some(RenderBlock::AgentMessage(_)) + )); + assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); + assert!(!minimal_api::is_committed(&s, s.get(2).unwrap())); + + // Answering the prompt finalizes the tool row, which then commits once, + // in its finished form — and the plan is NOT re-emitted. + s.get_mut(2).unwrap().mark_completed(); + assert_eq!(commit_collect(&mut s), vec![2]); + assert!(!scan_frontier(&s, false).will_commit); +} + +#[test] +fn anchored_plan_body_is_not_left_in_the_live_tail() { + // Whatever the commit pass prints must leave the live tail, or the plan is + // painted under the prompt AND printed above it. + let mut s = ScrollbackState::new(); + s.push(finalized("user prompt")); + let tool = s.push(running("exit_plan_mode")); + s.insert_block_before(tool, RenderBlock::agent_message("PLAN BODY")); + + // Sizing pass: the tail is just the tool row (index 2), and a commit is + // pending for the two entries above it. + let before = scan_frontier(&s, true); + assert_eq!(before.tail_start, 2, "plan is excluded from the live tail"); + assert!(before.will_commit); + + commit_leading_run(&mut s, true, |_, _| true); + + // Commit pass: same tail, nothing left to print. + let after = scan_frontier(&s, true); + assert_eq!(after.tail_start, before.tail_start, "tail must not move"); + assert!(!after.will_commit); +} + +#[test] +fn revised_plan_anchors_to_its_own_tool_row_and_neither_plan_re_emits() { + let mut s = ScrollbackState::new(); + let tool1 = s.push(running("exit_plan_mode #1")); + s.insert_block_before(tool1, RenderBlock::agent_message("PLAN ONE")); + assert_eq!(commit_collect(&mut s), vec![0]); // plan one + + s.get_mut(1).unwrap().mark_completed(); + let tool2 = s.push(running("exit_plan_mode #2")); + s.insert_block_before(tool2, RenderBlock::agent_message("PLAN TWO")); + + // Tool #1 and plan two commit; tool #2 holds the frontier. + assert_eq!(commit_collect(&mut s), vec![1, 2]); + // A third pass re-emits nothing. + assert!(commit_collect(&mut s).is_empty()); + assert_eq!(minimal_api::commit_scan_cursor(&s), 3); +} + +#[test] +fn bg_task_started_commits_while_running_and_does_not_wedge_frontier() { + // A fresh background task is pushed as a running "started" block + // (`set_last_running(true)`). Its `is_running` flag is animation-only — + // the block is a finalized lifecycle event whose content never changes — + // so it must commit immediately even mid-turn. Otherwise it wedges the + // frontier and the task (plus everything after it) stays hidden in the + // live tail until the task finishes (the reported dogfood bug). + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(ScrollbackEntry::running(RenderBlock::bg_task( + "sleep 60", "task-1", + ))); + s.push(running("later tool")); // more turn output after the bg task + + // "a" + the running bg task commit; only the trailing running tool stays. + assert_eq!(commit_collect(&mut s), vec![0, 1]); + assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); + assert!(!minimal_api::is_committed(&s, s.get(2).unwrap())); +} + +#[test] +fn bg_task_started_commits_as_last_running_entry() { + // Even as the last entry of a still-running turn the bg "started" block + // commits — a lifecycle block never streams more content (completion is + // a separate block). + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(ScrollbackEntry::running(RenderBlock::bg_task( + "sleep 60", "task-1", + ))); + assert_eq!(commit_collect(&mut s), vec![0, 1]); +} + +#[test] +fn no_double_commit_after_mid_list_shift_remove() { + let mut s = ScrollbackState::new(); + let a = s.push(finalized("a")); + s.push(finalized("b")); + s.push(finalized("c")); + assert_eq!(commit_collect(&mut s), vec![0, 1, 2]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 3); + + // Remove an already-committed entry below the cursor (shift_remove shifts + // the remaining indices down). The cursor is clamped; the per-entry + // `committed` flags travel with "b"/"c", so neither is re-emitted. + assert!(s.remove_entry(a)); + s.push(finalized("d")); // now at index 2 + + assert_eq!(commit_collect(&mut s), vec![2]); + assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); // b + assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); // c + assert!(minimal_api::is_committed(&s, s.get(2).unwrap())); // d +} + +#[test] +fn mid_list_removal_below_cursor_does_not_strand_uncommitted_entries() { + // Regression (review bug 2): a committed placeholder ("Loading + // session...") is removed AFTER new uncommitted entries were appended + // past the cursor — the `/resume` / reconnect `SessionLoaded` ordering. + // Removing below the cursor shifts the uncommitted entries down one; + // without the cursor decrement in `remove_entry` the first of them + // slid below the cursor and was never committed NOR drawn in the live + // tail (silently missing from minimal mode). + let mut s = ScrollbackState::new(); + s.push(finalized("old-1")); + let placeholder = s.push(finalized("Loading session...")); + + // A draw commits both; cursor = 2. + let mut seen = Vec::new(); + commit_leading_run(&mut s, false, |_, i| { + seen.push(i); + true + }); + assert_eq!(seen, vec![0, 1]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 2); + + // Replay appends entries, then the placeholder is removed in the same + // event cycle (before the next commit pass). + s.push(finalized("replayed-A")); + s.push(finalized("replayed-B")); + assert!(s.remove_entry(placeholder)); + // The cursor moved down with the shifted entries. + assert_eq!(minimal_api::commit_scan_cursor(&s), 1); + + // The next pass commits BOTH replayed entries — none stranded. + let mut seen = Vec::new(); + commit_leading_run(&mut s, false, |_, i| { + seen.push(i); + true + }); + assert_eq!(seen, vec![1, 2]); + assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); // replayed-A + assert!(minimal_api::is_committed(&s, s.get(2).unwrap())); // replayed-B +} + +#[test] +fn pending_user_input_holds_the_frontier_even_when_idle() { + // A block awaiting a permission / question answer must never commit, + // even if the turn state reads idle (e.g. a prompt outliving its turn): + // its rendered form still changes when the prompt resolves, and a + // committed copy is frozen. The idle relaxation only applies to + // *stale-running* flags, not pending-input marks. + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + let tool = s.push(finalized("tool")); + assert!(s.set_pending_user_input(tool, true)); + + let mut seen = Vec::new(); + commit_leading_run(&mut s, false, |_, i| { + seen.push(i); + true + }); + assert_eq!(seen, vec![0], "pending entry must hold the frontier"); + assert!(!minimal_api::is_committed(&s, s.get(1).unwrap())); + + // Resolving the prompt releases it. + assert!(s.set_pending_user_input(tool, false)); + let mut seen = Vec::new(); + commit_leading_run(&mut s, false, |_, i| { + seen.push(i); + true + }); + assert_eq!(seen, vec![1]); +} + +#[test] +fn failed_emit_leaves_entry_uncommitted_for_retry() { + // Regression (bugbot "Committed flag set on IO failure"): a terminal + // write failure must NOT mark the entry committed — print-once means a + // marked-but-unprinted block can never be emitted again. The walk stops + // with the cursor before the failed entry and retries next frame. + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(finalized("b")); + + // First pass: the emit fails on the first entry. + let mut calls = 0usize; + let n = commit_leading_run(&mut s, false, |_, _| { + calls += 1; + false + }); + assert_eq!(n, 0, "nothing committed on failure"); + assert_eq!(calls, 1, "walk stops at the first failure"); + assert!(!minimal_api::is_committed(&s, s.get(0).unwrap())); + assert_eq!(minimal_api::commit_scan_cursor(&s), 0, "cursor holds"); + + // Retry pass succeeds and commits both. + let n = commit_leading_run(&mut s, false, |_, _| true); + assert_eq!(n, 2); + assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); + assert!(minimal_api::is_committed(&s, s.get(1).unwrap())); +} + +#[test] +fn scan_frontier_mirrors_commit_leading_run() { + // `scan_frontier` (read-only: viewport sizing + the will-commit gate) + // must agree exactly with the mutating walk, in every phase. + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(finalized("b")); + s.push(running("c")); + s.push(finalized("d")); + + // Pre-commit: the pass would commit a+b and stop at the running entry. + let scan = scan_frontier(&s, true); + assert!(scan.will_commit); + assert_eq!(scan.tail_start, 2); + + let n = commit_leading_run(&mut s, true, |_, _| true); + assert_eq!(n, 2); + assert_eq!(minimal_api::commit_scan_cursor(&s), scan.tail_start); + + // Post-commit: nothing left to commit; the tail starts at the cursor. + let scan = scan_frontier(&s, true); + assert!(!scan.will_commit); + assert_eq!(scan.tail_start, 2); + + // Idle with no entries pending: everything committable. + let scan = scan_frontier(&s, false); + assert!(scan.will_commit); + assert_eq!(scan.tail_start, 4); +} + +#[test] +fn remove_from_below_frontier_then_push_still_commits() { + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(finalized("b")); + s.push(finalized("c")); + assert_eq!(commit_collect(&mut s), vec![0, 1, 2]); + + // Rewind: drop everything from index 1 (keep only "a"). Without the + // cursor clamp this would strand the cursor at 3 and silently skip the + // next pushes. + let removed = s.remove_from(1); + assert_eq!(removed.len(), 2); + assert_eq!(minimal_api::commit_scan_cursor(&s), 1); + + s.push(finalized("d")); // index 1 + assert_eq!(commit_collect(&mut s), vec![1]); +} + +#[test] +fn btw_block_emits_once_across_repeated_frontier_passes() { + let mut s = ScrollbackState::new(); + s.push(ScrollbackEntry::new(RenderBlock::Btw( + xai_grok_pager::scrollback::blocks::BtwBlock::new("original question", "original answer"), + ))); + + let mut emitted = Vec::new(); + assert_eq!( + commit_leading_run(&mut s, false, |state, i| { + let RenderBlock::Btw(block) = &state.get(i).unwrap().block else { + panic!("expected Btw block") + }; + assert_eq!(block.question, "original question"); + assert_eq!(block.content().text(), "original answer"); + emitted.push(i); + true + }), + 1 + ); + assert!(minimal_api::is_committed(&s, s.get(0).unwrap())); + + assert_eq!( + commit_leading_run(&mut s, false, |_, i| { + emitted.push(i); + true + }), + 0 + ); + assert_eq!(emitted, vec![0]); + assert!(!scan_frontier(&s, false).will_commit); +} + +#[test] +fn commit_leading_run_advances_frontier_and_marks_committed_once() { + let mut s = ScrollbackState::new(); + s.push(finalized("h1")); + s.push(finalized("h2")); + s.push(finalized("h3")); + + // Advances the frontier, marking the leading finalized run committed. + let mut emitted = Vec::new(); + let n = commit_leading_run(&mut s, false, |_, i| { + emitted.push(i); + true + }); + assert_eq!(n, 3); + assert_eq!(emitted, vec![0, 1, 2]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 3); + assert!((0..3).all(|i| minimal_api::is_committed(&s, s.get(i).unwrap()))); + + // A second pass commits nothing (already-committed entries are skipped). + let mut again = Vec::new(); + commit_leading_run(&mut s, false, |_, i| { + again.push(i); + true + }); + assert!(again.is_empty()); +} + +#[test] +fn idle_turn_commits_past_stale_running_entry() { + // Regression for the missing-edit/stuck-spinner bug: the agent tracker + // can leave an entry's `is_running` flag set after the turn ends (e.g. a + // thinking block whose finalize was missed at the thinking→tool + // transition). While the turn runs, that entry correctly holds the + // frontier; once the turn is idle the frontier must advance past it. + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + s.push(running("stale")); // stale is_running flag + s.push(finalized("c")); + + // Running turn: blocked at the running entry. + let mut seen = Vec::new(); + commit_leading_run(&mut s, true, |_, i| { + seen.push(i); + true + }); + assert_eq!(seen, vec![0]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 1); + + // Idle turn: commit everything past the stale flag. + let mut seen = Vec::new(); + commit_leading_run(&mut s, false, |_, i| { + seen.push(i); + true + }); + assert_eq!(seen, vec![1, 2]); + assert_eq!(minimal_api::commit_scan_cursor(&s), 3); +} + +#[test] +fn clear_resets_the_frontier() { + let mut s = ScrollbackState::new(); + s.push(finalized("a")); + commit_collect(&mut s); + assert_eq!(minimal_api::commit_scan_cursor(&s), 1); + + s.clear(); + assert_eq!(minimal_api::commit_scan_cursor(&s), 0); +} + +/// Height-exactness guard (design K5 / risk #1). `commit_active` reserves +/// exactly `desired_height(width)` rows via `insert_before`; if `render` +/// paints real content beyond that, those rows are silently clipped (lost) +/// from native scrollback. Render each block type into an over-tall buffer +/// and assert no non-space glyph lands past `desired_height`. (Background +/// fill of blank spaces past `h` is fine — only real content matters.) +fn assert_committed_fits(label: &str, block: RenderBlock, width: u16) { + let mut entry = ScrollbackEntry::new(block); + entry.set_display_mode(minimal_commit_display_mode( + &entry.block, + &default_appearance(), + )); + assert_committed_fits_entry(label, &entry, width); +} + +/// [`assert_committed_fits`] for a caller-stamped display mode. +fn assert_committed_fits_entry(label: &str, entry: &ScrollbackEntry, width: u16) { + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + + let renderer = minimal_renderer(entry, &theme, appearance, test_cwd(), COMMITTED_TICK); + let h = renderer.desired_height(width); + assert!(h > 0, "{label}@{width}: desired_height was 0"); + // The accent bar and background fill intentionally stretch to the given + // area height (chrome, not content). Only the content columns + // (x >= chrome_width) carry real text that `insert_before` would clip. + let chrome = renderer.chrome_width(); + + let extra = 8u16; + let area = Rect::new(0, 0, width, h + extra); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + + for y in h..(h + extra) { + for x in chrome..width { + let sym = buf.cell((x, y)).map(|c| c.symbol()).unwrap_or(" "); + assert!( + sym.trim().is_empty(), + "{label}@{width}: content {sym:?} painted at row {y} col {x}, past \ + desired_height {h} — insert_before would clip it from scrollback" + ); + } + } +} + +#[test] +fn committed_block_uses_owning_session_cwd_for_tool_paths() { + use ratatui::buffer::Buffer; + + let cwd = std::path::Path::new("/alternate/worktree"); + let mut entry = + ScrollbackEntry::new(RenderBlock::edit("/alternate/worktree/src/main.rs", None)); + entry.set_display_mode(DisplayMode::Expanded); + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + let renderer = minimal_renderer(&entry, &theme, appearance, cwd, COMMITTED_TICK); + let width = 80; + let height = renderer.desired_height(width); + let area = Rect::new(0, 0, width, height); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + + let mut text = String::new(); + for y in 0..height { + for x in 0..width { + text.push_str(buf[(x, y)].symbol()); + } + } + assert!(text.contains("src/main.rs"), "rendered text: {text:?}"); + assert!( + !text.contains("/alternate/worktree"), + "session prefix should be elided: {text:?}" + ); +} + +#[test] +fn committed_blocks_fit_desired_height() { + // Thinking blocks render zero rows unless `show_thinking_blocks` is on. + // The toggle is a thread-local, so pin it on here so the thinking + // block's committed height is actually exercised. + minimal_api::set_show_thinking_blocks(true); + + let long = "Hello there — this is a longer message that should wrap across \ + several lines at narrow widths to exercise the wrapping math, with \ + enough words to overflow eighty columns comfortably."; + for width in [40u16, 80, 120] { + assert_committed_fits("user_prompt", RenderBlock::user_prompt(long), width); + assert_committed_fits( + "agent_message", + RenderBlock::agent_message(format!( + "{long}\n\n- bullet one\n- bullet two\n\n```rust\nfn main() {{}}\n```" + )), + width, + ); + assert_committed_fits("thinking", RenderBlock::thinking(long), width); + assert_committed_fits( + "execute", + RenderBlock::execute_with_output( + "cargo build --release", + "line 1\nline 2\nline 3\nline 4\nline 5\nline 6", + None::, + ), + width, + ); + assert_committed_fits("edit", RenderBlock::edit("src/main.rs", None), width); + assert_committed_fits("read", RenderBlock::read("src/main.rs", None), width); + assert_committed_fits( + "list_dir", + RenderBlock::list_dir_with_output("src", "a.rs\nb.rs\nc.rs"), + width, + ); + assert_committed_fits("search", RenderBlock::search("TODO", 0, vec![]), width); + assert_committed_fits("system", RenderBlock::system("Session restored"), width); + assert_committed_fits("bg_task", RenderBlock::bg_task("sleep 30", "task-1"), width); + } +} + +/// Regression pinned: `md_style::to_anstyle` used to map `Color::Reset` +/// to a concrete ANSI-7 silver, washing out assistant/thinking markdown +/// body text on light terminals; `highlight_bash_command` leaked raw +/// syntect RGB. +#[test] +fn terminal_native_lock_paints_only_native_colors() { + use ratatui::buffer::Buffer; + use xai_grok_pager::theme::cache as theme_cache; + + let _guard = theme_cache::test_lock() + .lock() + .unwrap_or_else(|e| e.into_inner()); + struct LockReset; + impl Drop for LockReset { + fn drop(&mut self) { + xai_grok_pager::theme::cache::set_terminal_native_lock(false); + } + } + let _reset = LockReset; + theme_cache::set_terminal_native_lock(true); + minimal_api::set_show_thinking_blocks(true); + + let md = "Intro paragraph with **bold**, _italic_, `inline code`, and a \ + [link](https://example.com).\n\n# Heading one\n\n## Heading two\n\n\ + - item one\n- item two\n\n> a quote\n\n```rust\nfn main() { println!(\"hi\"); }\n```"; + use similar::ChangeTag; + let hunk = vec![ + xai_grok_pager::diff::DiffLine { + text: "let x = 1;\n".into(), + lo: 1, + ln: 1, + tag: ChangeTag::Equal, + }, + xai_grok_pager::diff::DiffLine { + text: "let y = 2;\n".into(), + lo: 2, + ln: 0, + tag: ChangeTag::Delete, + }, + xai_grok_pager::diff::DiffLine { + text: "let y = 3;\n".into(), + lo: 0, + ln: 2, + tag: ChangeTag::Insert, + }, + ]; + let blocks = vec![ + ("agent_message", RenderBlock::agent_message(md)), + ( + "thinking", + RenderBlock::thinking("Weighing the *options* with `care`."), + ), + ("user_prompt", RenderBlock::user_prompt("run the tests")), + ( + "edit", + RenderBlock::edit_with_hunks("src/main.rs", vec![hunk]), + ), + ( + "execute", + RenderBlock::execute_with_output("cargo build", "ok\n", None::), + ), + ("system", RenderBlock::system("Session restored")), + ]; + + for (label, block) in blocks { + let mut entry = ScrollbackEntry::new(block); + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + entry.set_display_mode(minimal_commit_display_mode(&entry.block, &appearance)); + let renderer = minimal_renderer(&entry, &theme, appearance, test_cwd(), COMMITTED_TICK); + + let width = 100u16; + let h = renderer.desired_height(width).max(1); + let area = Rect::new(0, 0, width, h); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + + for y in 0..h { + for x in 0..width { + let Some(cell) = buf.cell((x, y)) else { + continue; + }; + for (which, c) in [("fg", cell.fg), ("bg", cell.bg)] { + assert!( + !matches!(c, Color::Rgb(..) | Color::Indexed(_)), + "{label}: non-native {which} {c:?} at ({x},{y}) under \ + symbol {:?} — minimal must only use Reset / named \ + ANSI-16 so the terminal palette controls rendering", + cell.symbol() + ); + } + } + } + } +} + +#[test] +fn large_commit_is_capped_with_footer() { + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + // A tall block: a fenced code block keeps each line on its own row + // (markdown would otherwise join soft-wrapped prose into one paragraph), + // so the block is comfortably taller than the cap. + let lines: Vec = (0..60).map(|i| format!("line {i}")).collect(); + let body = format!("```\n{}\n```", lines.join("\n")); + let mut entry = ScrollbackEntry::new(RenderBlock::agent_message(body)); + entry.set_display_mode(minimal_commit_display_mode(&entry.block, &appearance)); + + let width = 80u16; + let renderer = minimal_renderer(&entry, &theme, appearance, test_cwd(), COMMITTED_TICK); + let full_h = renderer.desired_height(width); + assert!(full_h > 12, "expected a tall block, got {full_h}"); + + // Paint into a cap-height buffer (what `insert_committed` allocates). + let cap = 12u16; + let area = Rect::new(0, 0, width, cap); + let mut buf = Buffer::empty(area); + paint_committed(&mut buf, renderer, width, full_h, theme.dim()); + + // The final row is the overflow footer naming the hidden line count and + // pointing at /transcript; the buffer is exactly `cap` rows (bounded). + let last: String = (0..width) + .filter_map(|x| buf.cell((x, cap - 1)).map(|c| c.symbol().to_string())) + .collect(); + assert!(last.contains("more lines"), "footer row: {last:?}"); + assert!(last.contains("/transcript"), "footer row: {last:?}"); + // A hidden-line count is present (full_h minus the kept content rows). + let hidden = full_h - (cap - 1); + assert!( + last.contains(&hidden.to_string()), + "footer should name {hidden} hidden lines: {last:?}" + ); +} + +#[test] +fn small_commit_is_not_capped() { + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + let mut entry = ScrollbackEntry::new(RenderBlock::agent_message("one short line")); + entry.set_display_mode(minimal_commit_display_mode(&entry.block, &appearance)); + + let width = 80u16; + let renderer = minimal_renderer(&entry, &theme, appearance, test_cwd(), COMMITTED_TICK); + let full_h = renderer.desired_height(width); + + // Buffer is exactly the block's height → no footer (uncapped path). + let area = Rect::new(0, 0, width, full_h); + let mut buf = Buffer::empty(area); + paint_committed(&mut buf, renderer, width, full_h, theme.dim()); + + let mut all = String::new(); + for y in 0..full_h { + for x in 0..width { + all.push_str(buf.cell((x, y)).map(|c| c.symbol()).unwrap_or(" ")); + } + } + assert!( + !all.contains("more lines"), + "no footer when uncapped: {all:?}" + ); +} + +#[test] +fn committed_edit_keeps_diff_line_backgrounds() { + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + use similar::ChangeTag; + use xai_grok_pager::diff::DiffLine; + + let hunk = vec![ + DiffLine { + text: "let x = 1;\n".into(), + lo: 10, + ln: 10, + tag: ChangeTag::Equal, + }, + DiffLine { + text: "let y = 2;\n".into(), + lo: 11, + ln: 0, + tag: ChangeTag::Delete, + }, + DiffLine { + text: "let y = 3;\n".into(), + lo: 0, + ln: 11, + tag: ChangeTag::Insert, + }, + DiffLine { + text: "let z = 4;\n".into(), + lo: 12, + ln: 12, + tag: ChangeTag::Equal, + }, + ]; + let block = RenderBlock::edit_with_hunks("src/main.rs", vec![hunk]); + let mut entry = ScrollbackEntry::new(block); + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + entry.set_display_mode(minimal_commit_display_mode(&entry.block, &appearance)); + let renderer = minimal_renderer(&entry, &theme, appearance, test_cwd(), COMMITTED_TICK); + + let width = 80u16; + let h = renderer.desired_height(width); + let area = Rect::new(0, 0, width, h); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + + // The committed edit uses a flat background (terminal transparency), but + // must still paint the per-line diff backgrounds — otherwise an added / + // removed line is indistinguishable from context. + let mut saw_insert = false; + let mut saw_delete = false; + for y in 0..h { + for x in 0..width { + if let Some(cell) = buf.cell((x, y)) { + saw_insert |= cell.bg == theme.diff_insert_bg; + saw_delete |= cell.bg == theme.diff_delete_bg; + } + } + } + assert!( + saw_insert, + "committed edit lost the insert (green) diff background" + ); + assert!( + saw_delete, + "committed edit lost the delete (red) diff background" + ); +} + +/// Asserted through `chrome_width` because that is what both `desired_height` +/// and `render` subtract from the wrap width — one column is the whole cost of +/// the rail, which is why restoring it is height-safe (K5). +#[test] +fn only_thinking_spends_the_accent_column() { + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + let chrome = |entry: &ScrollbackEntry| { + minimal_renderer( + entry, + &theme, + appearance.clone(), + test_cwd(), + COMMITTED_TICK, + ) + .chrome_width() + }; + + assert_eq!( + chrome(&ScrollbackEntry::new(RenderBlock::thinking("reasoning"))), + 1, + "reasoning reserves the 1-col accent gutter" + ); + + for block in [ + RenderBlock::agent_message("answer"), + RenderBlock::user_prompt("ask"), + RenderBlock::execute("ls"), + RenderBlock::read("src/lib.rs", None), + RenderBlock::edit("src/main.rs", None), + RenderBlock::system("Session restored"), + ] { + let entry = ScrollbackEntry::new(block); + assert_eq!( + chrome(&entry), + 0, + "only reasoning may spend the accent column: {:?}", + entry.block + ); + } + + // A column is reserved only where the block actually paints one. Reserving + // without painting leaves the content indented over a blank gutter — which + // is what a collapsed reasoning header did while `hide_accent` keyed off + // the block type alone. + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + let rail = xai_grok_pager::glyphs::accent_bar(); + minimal_api::set_show_thinking_blocks(true); + for mode in [ + DisplayMode::Collapsed, + DisplayMode::Truncated, + DisplayMode::Expanded, + ] { + let mut entry = ScrollbackEntry::new(RenderBlock::thinking( + "reasoning long enough to wrap over a couple of rows at sixty columns", + )); + entry.set_display_mode(mode); + + let renderer = minimal_renderer( + &entry, + &theme, + appearance.clone(), + test_cwd(), + COMMITTED_TICK, + ); + let reserved = renderer.chrome_width(); + let h = renderer.desired_height(60); + let area = Rect::new(0, 0, 60, h); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + let painted = buf.cell((0, 0)).expect("first cell").symbol() == rail; + + assert_eq!( + reserved == 1, + painted, + "{mode:?}: reserved a column={} but painted the rail={painted} — a \ + reserved-but-unpainted column is a blank indent", + reserved == 1, + ); + } +} + +#[test] +fn committed_thinking_paints_a_dim_rail_in_column_zero() { + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + use ratatui::style::Modifier; + use xai_grok_pager::theme::cache as theme_cache; + + let _guard = theme_cache::test_lock() + .lock() + .unwrap_or_else(|e| e.into_inner()); + struct LockReset; + impl Drop for LockReset { + fn drop(&mut self) { + xai_grok_pager::theme::cache::set_terminal_native_lock(false); + } + } + let _reset = LockReset; + theme_cache::set_terminal_native_lock(true); + minimal_api::set_show_thinking_blocks(true); + + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + let mut entry = ScrollbackEntry::new(RenderBlock::thinking( + "a reasoning body long enough to wrap over several rows at this width", + )); + entry.set_display_mode(minimal_commit_display_mode(&entry.block, &appearance)); + + let width = 40u16; + let renderer = minimal_renderer( + &entry, + &theme, + appearance.clone(), + test_cwd(), + COMMITTED_TICK, + ); + let h = renderer.desired_height(width); + assert!(h > 1, "expected a multi-row reasoning block, got {h}"); + let area = Rect::new(0, 0, width, h); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + + let rail = xai_grok_pager::glyphs::accent_bar(); + for y in 0..h { + let cell = buf.cell((0, y)).expect("accent cell"); + assert_eq!(cell.symbol(), rail, "row {y} lost the rail"); + assert!( + cell.modifier.contains(Modifier::DIM), + "row {y}: rail must be dim, got {:?}", + cell.modifier + ); + } + + let answer = ScrollbackEntry::new(RenderBlock::agent_message("the answer")); + let renderer = minimal_renderer(&answer, &theme, appearance, test_cwd(), COMMITTED_TICK); + let area = Rect::new(0, 0, width, renderer.desired_height(width).max(1)); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + assert_ne!( + buf.cell((0, 0)).expect("first cell").symbol(), + rail, + "assistant output must not wear a rail" + ); +} + +#[test] +fn commit_display_mode_policy() { + assert_eq!( + minimal_commit_display_mode(&RenderBlock::thinking("reasoning"), &default_appearance()), + DisplayMode::Expanded + ); + assert_eq!( + minimal_commit_display_mode(&RenderBlock::edit("file.rs", None), &default_appearance()), + DisplayMode::Expanded + ); + assert_eq!( + minimal_commit_display_mode(&RenderBlock::execute("ls"), &default_appearance()), + DisplayMode::Truncated + ); + assert_eq!( + minimal_commit_display_mode(&RenderBlock::agent_message("hi"), &default_appearance()), + DisplayMode::Expanded + ); +} + +#[test] +fn commit_display_mode_lookups_collapse_on_success_only() { + use xai_grok_pager::scrollback::blocks::{ + ListDirToolCallBlock, ReadToolCallBlock, SearchToolCallBlock, + }; + + assert_eq!( + minimal_commit_display_mode( + &RenderBlock::search("pat", 3, vec![]), + &default_appearance() + ), + DisplayMode::Collapsed + ); + assert_eq!( + minimal_commit_display_mode( + &RenderBlock::read("src/lib.rs", None), + &default_appearance() + ), + DisplayMode::Collapsed + ); + assert_eq!( + minimal_commit_display_mode( + &RenderBlock::list_dir_with_output("src", "a.rs\nb.rs"), + &default_appearance() + ), + DisplayMode::Collapsed + ); + + for failed in [ + RenderBlock::ToolCall(ToolCallBlock::Search( + SearchToolCallBlock::new("pat").with_error("regex parse error"), + )), + RenderBlock::ToolCall(ToolCallBlock::Read( + ReadToolCallBlock::new("gone.rs").with_error("file not found"), + )), + RenderBlock::ToolCall(ToolCallBlock::ListDir( + ListDirToolCallBlock::new("gone/").with_error("no such directory"), + )), + ] { + assert_eq!( + minimal_commit_display_mode(&failed, &default_appearance()), + DisplayMode::Truncated, + "failed lookup must stay truncated: {failed:?}" + ); + } +} + +#[test] +fn collapse_thinking_toggle_flips_only_reasoning() { + assert_eq!( + minimal_commit_display_mode(&RenderBlock::thinking("reasoning"), &default_appearance()), + DisplayMode::Expanded, + "default (K9): reasoning commits in full" + ); + assert_eq!( + minimal_commit_display_mode(&RenderBlock::thinking("reasoning"), &collapsed_appearance()), + DisplayMode::Collapsed + ); + + // Everything else is unaffected by the toggle. + for block in [ + RenderBlock::agent_message("hi"), + RenderBlock::edit("file.rs", None), + RenderBlock::execute("ls"), + RenderBlock::read("src/lib.rs", None), + ] { + assert_eq!( + minimal_commit_display_mode(&block, &collapsed_appearance()), + minimal_commit_display_mode(&block, &default_appearance()), + "the toggle must only touch reasoning: {block:?}" + ); + } +} + +#[test] +fn collapsed_thinking_commit_is_one_advertised_row() { + use ratatui::buffer::Buffer; + use ratatui::layout::Rect; + + minimal_api::set_show_thinking_blocks(true); + let theme = Theme::current(); + let appearance = committed_appearance(&AppearanceConfig::default()); + + let mut entry = ScrollbackEntry::new(RenderBlock::thinking( + "a long reasoning body that would otherwise occupy many rows in the transcript", + )); + entry.set_display_mode(minimal_commit_display_mode( + &entry.block, + &collapsed_appearance(), + )); + assert_eq!(entry.display_mode(), DisplayMode::Collapsed); + // The mode `commit_active` records for the Ctrl+E ring. + assert!(matches!( + entry.display_mode(), + DisplayMode::Collapsed | DisplayMode::Truncated + )); + + for width in [40u16, 80, 120] { + let renderer = minimal_renderer( + &entry, + &theme, + appearance.clone(), + test_cwd(), + COMMITTED_TICK, + ); + let h = renderer.desired_height(width); + assert_eq!(h, 1, "collapsed reasoning is one row @{width}"); + let area = Rect::new(0, 0, width, h); + let mut buf = Buffer::empty(area); + renderer.render(area, &mut buf); + let row: String = (0..width) + .filter_map(|x| buf.cell((x, 0)).map(|c| c.symbol().to_string())) + .collect(); + assert!(row.contains("Thought"), "@{width}: {row:?}"); + assert!( + row.contains("ctrl+e to expand"), + "@{width}: the only way into a print-once folded block must be \ + advertised: {row:?}" + ); + } + + // Too narrow for the hint: the header still wins, still one row. + let renderer = minimal_renderer(&entry, &theme, appearance, test_cwd(), COMMITTED_TICK); + assert_eq!(renderer.desired_height(16), 1); +} + +/// K5 guard: reasoning committed collapsed must still fit its reserved +/// `insert_before` height. +#[test] +fn collapsed_thinking_commit_fits_desired_height() { + minimal_api::set_show_thinking_blocks(true); + let long = "Reasoning that is long enough to wrap several times at forty \ + columns, with a `code span` and **emphasis** to exercise the \ + markdown spans under the dim+italic patch."; + for width in [40u16, 80, 120] { + let mut entry = ScrollbackEntry::new(RenderBlock::thinking(long)); + entry.set_display_mode(minimal_commit_display_mode( + &entry.block, + &collapsed_appearance(), + )); + assert_committed_fits_entry("thinking_collapsed", &entry, width); + } +} diff --git a/crates/codegen/xai-grok-pager-minimal/src/full_view.rs b/crates/codegen/xai-grok-pager-minimal/src/full_view.rs index 64beec5..7ad5b82 100644 --- a/crates/codegen/xai-grok-pager-minimal/src/full_view.rs +++ b/crates/codegen/xai-grok-pager-minimal/src/full_view.rs @@ -367,6 +367,41 @@ mod tests { ); } + /// `/transcript` is the escape hatch `minimal_collapse_thinking` leans on, + /// so it must ignore the committed display mode. + #[test] + fn transcript_expands_thinking_committed_collapsed() { + let theme = Theme::current(); + let appearance = super::super::commit::committed_appearance( + &xai_grok_pager::appearance::AppearanceConfig { + minimal_collapse_thinking: true, + ..Default::default() + }, + ); + let mut entry = ScrollbackEntry::new(RenderBlock::thinking( + "REASONINGBODY folded away at commit time", + )); + entry.set_display_mode(super::super::commit::minimal_commit_display_mode( + &entry.block, + &appearance, + )); + assert_eq!(entry.display_mode(), DisplayMode::Collapsed); + + xai_grok_pager::appearance::cache::set_show_thinking_blocks(true); + let mut out = String::new(); + render_entry_to_ansi(&entry, &theme, &appearance, test_cwd(), &mut out); + xai_grok_pager::appearance::cache::set_show_thinking_blocks(false); + + assert!( + out.contains("REASONINGBODY"), + "a collapsed commit must still expand in /transcript: {out:?}" + ); + assert!( + !out.contains("ctrl+e to expand"), + "no expand hint in the fully-expanded transcript: {out:?}" + ); + } + #[test] fn transcript_uses_owning_session_cwd_for_tool_paths() { let theme = Theme::current(); diff --git a/crates/codegen/xai-grok-pager-minimal/src/live.rs b/crates/codegen/xai-grok-pager-minimal/src/live.rs index e69301e..c7264e0 100644 --- a/crates/codegen/xai-grok-pager-minimal/src/live.rs +++ b/crates/codegen/xai-grok-pager-minimal/src/live.rs @@ -403,12 +403,7 @@ fn live_tail_renderer<'a>( cwd: &'a std::path::Path, tick: u64, ) -> EntryRenderer<'a> { - EntryRenderer::new(entry, theme) - .with_appearance(appearance.clone()) - .with_cwd(Some(cwd)) - .with_tick(tick) - .with_flat_background(true) - .with_hide_accent(true) + super::commit::minimal_renderer(entry, theme, appearance.clone(), cwd, tick) } /// Render the uncommitted tail (entries past the commit frontier), bottom-anchored /// so the most recent output is always visible; the topmost visible entry is @@ -813,6 +808,46 @@ mod tests { painted_height.saturating_add(super::super::commit::MINIMAL_BLOCK_GAP) ); } + /// The tail and the committed footprint are one builder with a different + /// tick; this is the net for anyone tempted to fork them again. + #[test] + fn the_animation_tick_never_changes_a_blocks_height() { + use xai_grok_pager::scrollback::RenderBlock; + use xai_grok_pager::scrollback::entry::ScrollbackEntry; + minimal_api::set_show_thinking_blocks(true); + let theme = Theme::current(); + let cwd = std::path::PathBuf::from("/tmp"); + let appearance = super::super::commit::committed_appearance( + &xai_grok_pager::appearance::AppearanceConfig::default(), + ); + let long = "reasoning that wraps a good few times even at a hundred and \ + twenty columns because it simply keeps going and going and going"; + for block in [ + RenderBlock::thinking(long), + RenderBlock::agent_message(long), + RenderBlock::execute("ls -la"), + ] { + let entry = ScrollbackEntry::new(block); + for width in [20u16, 40, 80, 120] { + let live = + live_tail_renderer(&entry, &theme, &appearance, &cwd, 7).desired_height(width); + let committed = live_tail_renderer( + &entry, + &theme, + &appearance, + &cwd, + super::super::commit::COMMITTED_TICK, + ) + .desired_height(width); + assert_eq!( + live, committed, + "{:?} @{width}: a block's height must not depend on the tick, or the \ + prompt jumps on commit", + entry.block + ); + } + } + } #[test] fn minimal_status_shows_rich_activity_and_idle_hint() { use xai_grok_pager::acp::tracker::TurnActivity; diff --git a/crates/codegen/xai-grok-pager-minimal/src/plan.rs b/crates/codegen/xai-grok-pager-minimal/src/plan.rs index 0d77fb4..e111046 100644 --- a/crates/codegen/xai-grok-pager-minimal/src/plan.rs +++ b/crates/codegen/xai-grok-pager-minimal/src/plan.rs @@ -74,6 +74,12 @@ fn plan_scrollback_body(plan_content: Option<&str>) -> String { /// a short notice so the user sees *why* approval is parked (otherwise only the /// controls strip appears and the session looks stuck). /// +/// The block is anchored **above** the still-running `exit_plan_mode` tool row, +/// not appended after it, so the commit frontier reaches the plan while the +/// approval is still parked. Users reported losing the head of a plan to the +/// clipped live tail; design doc §6.16 has the full argument and the rejected +/// alternatives. +/// /// NOTE (draw-path state mutation + replay durability): this pushes into /// `ScrollbackState` from the render path — a deliberate exception, since the /// plan block must enter the normal commit pipeline. The pushed block is @@ -110,9 +116,17 @@ pub fn maybe_commit_plan(app: &mut AppView) { // if it ever did, stamping the id anyway would treat the plan as committed // while nothing ever reaches native scrollback. if let Some(agent) = app.agents.get_mut(&id) { - agent - .scrollback - .push_block(RenderBlock::agent_message(content)); + let block = RenderBlock::agent_message(content); + // No anchor (the tool was reaped): append, and the plan commits at turn + // end — the pre-fix behavior, still better than dropping it. + match minimal_api::pending_tool_entry_id(agent, &tool_call_id) { + Some(anchor) => { + agent.scrollback.insert_block_before(anchor, block); + } + None => { + agent.scrollback.push_block(block); + } + } minimal_api::set_minimal_committed_plan_id(app, Some(tool_call_id)); } } diff --git a/crates/codegen/xai-grok-pager-pty-harness/tests/privacy_banner_e2e.rs b/crates/codegen/xai-grok-pager-pty-harness/tests/privacy_banner_e2e.rs index b464c14..ad34fac 100644 --- a/crates/codegen/xai-grok-pager-pty-harness/tests/privacy_banner_e2e.rs +++ b/crates/codegen/xai-grok-pager-pty-harness/tests/privacy_banner_e2e.rs @@ -1,9 +1,10 @@ //! E2E: the coding-data privacy upsell banner — shown on the welcome screen //! for an opted-out OAuth user under the `privacy_notice_rollout` flag, //! persisting into the agent view, and acked (never re-shown) via both -//! buttons: `[Customize in settings]` opens the settings chooser and stamps -//! `[privacy].privacy_banner_acked`; `[Accept]` opts the user in through the -//! shell's `PUT /privacy/coding-data-retention` round trip before acking. +//! buttons: `[Opt out]` dismisses on the spot, stamping +//! `[privacy].privacy_banner_acked` without waiting on the server; `[Opt in]` +//! opts the user in through the shell's `PUT /privacy/coding-data-retention` +//! round trip and acks only once that succeeds. //! //! Drives the real pager binary through a PTY against the shared mock //! inference server (isolated `$HOME`), with a seeded opted-out OAuth entry @@ -27,20 +28,20 @@ use xai_grok_pager_pty_harness::{ const ROWS: u16 = 50; const COLS: u16 = 120; const BANNER_TITLE: &str = "Help improve Grok"; -const CUSTOMIZE: &str = "[Customize in settings]"; -const ACCEPT: &str = "[Accept]"; +const OPT_OUT: &str = "[Opt out]"; +const OPT_IN: &str = "[Opt in]"; const ACK: &str = "BANNERACK"; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[ignore] // opt-in: spawns the real pager binary in a PTY (CI runs with --ignored) -async fn privacy_banner_welcome_customize_ack_persists() { - run_customize().await.expect("privacy banner customize e2e"); +async fn privacy_banner_welcome_opt_out_ack_persists() { + run_opt_out().await.expect("privacy banner opt-out e2e"); } #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[ignore] // opt-in: spawns the real pager binary in a PTY (CI runs with --ignored) -async fn privacy_banner_persists_into_agent_view_and_accept_opts_in() { - run_accept().await.expect("privacy banner accept e2e"); +async fn privacy_banner_persists_into_agent_view_and_opt_in_shares() { + run_opt_in().await.expect("privacy banner opt-in e2e"); } /// Rollout flag forced on (env override beats remote settings) and the @@ -53,7 +54,7 @@ fn banner_env_ops() -> [EnvOp<'static>; 2] { ] } -async fn run_customize() -> Result<()> { +async fn run_opt_out() -> Result<()> { let content = ContentController::start() .await .context("start mock server")?; @@ -66,29 +67,27 @@ async fn run_customize() -> Result<()> { let mut pager = spawn_pager(&binary, &content, project.path()).context("spawn pager")?; wait_for_banner(&mut pager)?; assert!( - pager.contains_text(ACCEPT), - "welcome banner is missing {ACCEPT}:\n{}", + pager.contains_text(OPT_IN), + "welcome banner is missing {OPT_IN}:\n{}", pager.screen_contents() ); - click_text(&mut pager, CUSTOMIZE).context("click Customize")?; + click_text(&mut pager, OPT_OUT).context("click Opt out")?; + + // Dismissal is local and immediate — it must not wait on the server, and + // must not detour into settings. pager - .wait_for_text("Coding data sharing", Duration::from_secs(20)) - .context("settings chooser opened on Coding data sharing")?; + .wait_for_text_absent(BANNER_TITLE, Duration::from_secs(10)) + .context("banner dismissed by [Opt out]")?; assert!( - pager.contains_text("Opt in") && pager.contains_text("Opt out"), - "chooser is missing the Opt in / Opt out choices:\n{}", + !pager.contains_text("Coding data,"), + "[Opt out] must answer the question, not open settings:\n{}", pager.screen_contents() ); - // Customize acks immediately; the config write is async — poll for it. + // The config write is async — poll for it. wait_for_ack_on_disk(&mut pager, content.home(), Duration::from_secs(10))?; - // Close the chooser, then the settings list, then quit gracefully. - pager.inject_keys(keys::ESC).context("close chooser")?; - pager.update(Duration::from_millis(300)); - pager.inject_keys(keys::ESC).context("close settings")?; - pager.update(Duration::from_millis(300)); quit_via_double_ctrl_c(&mut pager)?; drop(pager); @@ -110,7 +109,7 @@ async fn run_customize() -> Result<()> { Ok(()) } -async fn run_accept() -> Result<()> { +async fn run_opt_in() -> Result<()> { let content = ContentController::start() .await .context("start mock server")?; @@ -136,12 +135,12 @@ async fn run_accept() -> Result<()> { pager.screen_contents() ); - click_text(&mut pager, ACCEPT).context("click Accept")?; + click_text(&mut pager, OPT_IN).context("click Opt in")?; // Ack only lands after the shell's PUT round trip confirms 2xx. pager .wait_for_text_absent(BANNER_TITLE, Duration::from_secs(20)) - .context("banner disappeared after Accept")?; + .context("banner disappeared after [Opt in]")?; wait_for_ack_on_disk(&mut pager, content.home(), Duration::from_secs(10))?; let put_bodies: Vec<_> = content diff --git a/crates/codegen/xai-grok-pager-pty-harness/tests/settings_locked_row_e2e.rs b/crates/codegen/xai-grok-pager-pty-harness/tests/settings_locked_row_e2e.rs index f6ac27f..ea61651 100644 --- a/crates/codegen/xai-grok-pager-pty-harness/tests/settings_locked_row_e2e.rs +++ b/crates/codegen/xai-grok-pager-pty-harness/tests/settings_locked_row_e2e.rs @@ -1,4 +1,4 @@ -//! E2E: the settings modal's locked `Coding data sharing` row, driven off +//! E2E: the settings modal's locked coding-data row, driven off //! the seeded auth entry through the full pipeline (auth.json → shell //! `GrokAuth` → auth meta → `AppView::coding_data_sharing_lock()` → //! `PagerLocalSnapshot` → render): @@ -33,11 +33,16 @@ use xai_grok_pager_pty_harness::{ const ROWS: u16 = 50; const COLS: u16 = 120; const BANNER_TITLE: &str = "Help improve Grok"; -const ROW_LABEL: &str = "Coding data sharing"; +/// Head of the row's label (`Coding data, retention, and training`). The +/// modal truncates long labels, so match the stable prefix. +const ROW_LABEL: &str = "Coding data"; const CHEVRON: &str = "\u{203A}"; // › const ZDR_REASON: &str = "Your team has Zero Data Retention."; const TEAM_REASON: &str = "Managed by your team admin."; -const DESCRIPTION_PREFIX: &str = "Controls whether"; +/// Head of the row's description in `settings/defs.rs`. Kept short so it +/// can't span one of the modal's word wraps — `contains_text` joins rows +/// with `\n`, so a match on wrapped copy would silently never fire. +const DESCRIPTION_PREFIX: &str = "Opt-in to provide SpaceXAI"; #[tokio::test(flavor = "multi_thread", worker_threads = 2)] #[ignore] // opt-in: spawns the real pager binary in a PTY (CI runs with --ignored) @@ -222,7 +227,7 @@ fn open_settings_and_grab_row_line(pager: &mut PtyHarness) -> Result { pager.inject_keys(keys::ENTER).context("commit filter")?; pager .wait_for_text(ROW_LABEL, Duration::from_secs(20)) - .context("Coding data sharing row visible")?; + .context("coding-data row visible")?; pager.update(Duration::from_millis(500)); let screen = pager.screen_contents(); screen @@ -235,7 +240,7 @@ fn open_settings_and_grab_row_line(pager: &mut PtyHarness) -> Result { /// Expand the focused row with `→` (Browse-mode `KeyCode::Right` inserts the /// focused key into `expanded_keys`) and wait for `reason` to render. /// Callers reach here from [`open_settings_and_grab_row_line`], which leaves -/// the Coding data sharing row focused. +/// the coding-data row focused. fn expand_focused_row(pager: &mut PtyHarness, reason: &str) -> Result<()> { pager.inject_keys(keys::RIGHT).context("expand row")?; pager.update(Duration::from_millis(300)); diff --git a/crates/codegen/xai-grok-pager-render/src/appearance/config.rs b/crates/codegen/xai-grok-pager-render/src/appearance/config.rs index 4653d29..76da40f 100644 --- a/crates/codegen/xai-grok-pager-render/src/appearance/config.rs +++ b/crates/codegen/xai-grok-pager-render/src/appearance/config.rs @@ -52,6 +52,8 @@ pub struct AppearanceConfig { /// Maximum rows a single committed block may occupy in minimal mode before /// it is truncated with a "… N more lines" footer. pub minimal_max_commit_rows: u16, + /// Resolved `[terminal] minimal_collapse_thinking`. + pub minimal_collapse_thinking: bool, } impl Default for AppearanceConfig { @@ -559,6 +561,16 @@ pub struct ThinkingConfig { /// (matching tool block title style), and respects muted_collapsed when collapsed. /// When false (default), the header is always dim/muted gray. pub header_bright: bool, + /// Render the reasoning body de-emphasized (SGR dim + italic) on top of the + /// `bg_blend` fade, for surfaces where the fade alone cannot separate + /// reasoning from the answer. **Not a TOML key** — minimal mode sets it; + /// see the minimal-mode design doc §6.16. + pub body_dim_italic: bool, + /// Append a dim "(ctrl+e to expand)" affordance to the *collapsed* header + /// when it fits on the same row (never adds a row). **Not a TOML key** — + /// minimal mode sets it, being the only surface where a folded block cannot + /// be unfolded in place. + pub collapsed_expand_hint: bool, } impl Default for ThinkingConfig { @@ -571,6 +583,8 @@ impl Default for ThinkingConfig { animate: true, header: true, header_bright: false, + body_dim_italic: false, + collapsed_expand_hint: false, } } } @@ -768,6 +782,12 @@ pub struct RawTerminalConfig { pub minimal_live_rows: Option, /// Maximum rows for a single committed block in minimal mode. Default 2000. pub minimal_max_commit_rows: Option, + /// Commit reasoning ("Thought for Xs") to native scrollback COLLAPSED to + /// its one-line header instead of in full. Default false — minimal + /// deliberately keeps the whole reasoning body in the transcript (K9); this + /// is the opt-out for a terser scrollback. The body stays reachable with + /// `Ctrl+E` / `/expand` and `/transcript`. + pub minimal_collapse_thinking: bool, } impl Default for RawTerminalConfig { @@ -777,6 +797,7 @@ impl Default for RawTerminalConfig { minimal: false, minimal_live_rows: None, minimal_max_commit_rows: None, + minimal_collapse_thinking: false, } } } @@ -1435,6 +1456,7 @@ impl From for AppearanceConfig { minimal: raw.terminal.minimal, minimal_live_rows: raw.terminal.minimal_live_rows.unwrap_or(10), minimal_max_commit_rows: raw.terminal.minimal_max_commit_rows.unwrap_or(2000), + minimal_collapse_thinking: raw.terminal.minimal_collapse_thinking, } } } @@ -1603,6 +1625,8 @@ impl From for ThinkingConfig { animate: raw.animate, header: raw.header, header_bright: raw.header_bright, + body_dim_italic: false, + collapsed_expand_hint: false, } } } @@ -2464,4 +2488,44 @@ gutter_bg = true "Missing alt_screen in generated config:\n{toml}" ); } + + /// A config written before the key existed must still parse and keep K9. + #[test] + fn minimal_collapse_thinking_defaults_off_and_old_configs_parse() { + let empty: RawAppearanceConfig = toml::from_str("").expect("empty config must parse"); + assert!(!empty.terminal.minimal_collapse_thinking); + assert!(!AppearanceConfig::from(empty).minimal_collapse_thinking); + + let legacy: RawAppearanceConfig = + toml::from_str("[terminal]\nminimal = true\nminimal_live_rows = 12\n") + .expect("legacy config must parse"); + let cfg: AppearanceConfig = legacy.into(); + assert!(cfg.minimal); + assert_eq!(cfg.minimal_live_rows, 12); + assert!( + !cfg.minimal_collapse_thinking, + "a config written before the key existed must keep the K9 default" + ); + + assert!(!AppearanceConfig::default().minimal_collapse_thinking); + } + + #[test] + fn minimal_collapse_thinking_opt_in_parses() { + let raw: RawAppearanceConfig = + toml::from_str("[terminal]\nminimal_collapse_thinking = true\n").unwrap(); + assert!(AppearanceConfig::from(raw).minimal_collapse_thinking); + } + + /// The reasoning-legibility toggles must stay un-settable from pager.toml. + #[test] + fn thinking_body_treatment_is_off_by_default_and_not_a_toml_key() { + let cfg = AppearanceConfig::default(); + assert!(!cfg.scrollback.blocks.thinking.body_dim_italic); + assert!(!cfg.scrollback.blocks.thinking.collapsed_expand_hint); + + let template = RawAppearanceConfig::to_toml_with_comments(); + assert!(!template.contains("body_dim_italic")); + assert!(!template.contains("collapsed_expand_hint")); + } } diff --git a/crates/codegen/xai-grok-pager-render/src/gboom/mod.rs b/crates/codegen/xai-grok-pager-render/src/gboom/mod.rs index 02b1068..10de551 100644 --- a/crates/codegen/xai-grok-pager-render/src/gboom/mod.rs +++ b/crates/codegen/xai-grok-pager-render/src/gboom/mod.rs @@ -106,6 +106,8 @@ impl GboomState { // On terminals that report key releases (Kitty keyboard protocol), // latch keys on press/release so the player can move and turn at // once; otherwise fall back to the repeat-bridging timer model. + // Deliberately `kitty_flags_pushed`, not `kitty_releases_reported`: the + // game pushes its own REPORT_ALL_KEYS layer over a downgraded base. game.set_release_aware(crate::terminal::kitty_flags_pushed()); Self { game, diff --git a/crates/codegen/xai-grok-pager-render/src/terminal/da2.rs b/crates/codegen/xai-grok-pager-render/src/terminal/da2.rs new file mode 100644 index 0000000..7d28ffb --- /dev/null +++ b/crates/codegen/xai-grok-pager-render/src/terminal/da2.rs @@ -0,0 +1,251 @@ +//! Runtime DA2 (Secondary Device Attributes) probe: `CSI > 0 c` → +//! `CSI > Pp ; Pv ; Pc c`, where `Pv` is a version packed as +//! `major * 10000 + minor * 100 + patch`. +//! +//! Alacritty is the reason this exists: it exports no version environment +//! variable and refuses XTVERSION on principle. What it answers with is the +//! `alacritty_terminal` library version — see [`unpack_version`]. +//! +//! Unlike [`super::xtversion`] the reply is read at the fd rather than +//! recognized by an event-loop filter, because no filter could see it: +//! crossterm has no `CSI >` arm, so it errors and clears its buffer, dropping +//! the intro and leaving digits indistinguishable from typing. +//! +//! The read owns stdin, so it must run after `enable_raw_mode()` and before +//! crossterm's `EventStream` exists. A late reply that arrives partially is +//! drained to quiet by [`super::probe`]; one of which *no* byte arrives before +//! the deadline is left for crossterm, which types it into the composer — +//! `REPLY_TIMEOUT` is sized to keep that out of reach. + +use std::sync::OnceLock; +#[cfg(unix)] +use std::time::Duration; + +static DA2_VERSION: OnceLock> = OnceLock::new(); + +/// Both forms of one reply. The packed integer is kept rather than recovered +/// from `text`, so version gates compare what the terminal sent instead of +/// re-parsing what this module formatted. +#[derive(Debug, Eq, PartialEq)] +struct Da2Version { + packed: u32, + text: String, +} + +#[cfg(unix)] +const QUERY: &[u8] = b"\x1b[>0c"; + +/// Sized for a slow link, not for a silent terminal: a reply that misses the +/// deadline entirely is typed into the composer, not merely lost. +#[cfg(unix)] +const REPLY_TIMEOUT: Duration = Duration::from_millis(500); + +/// Rejects a packed value that cannot be a real release (major ≥ 100) instead +/// of folding it into a plausible-looking version. +#[cfg(any(unix, test))] +const MAX_PACKED_VERSION: u32 = 999_999; + +/// Returns the version the terminal reported over DA2, if it answered. +pub fn detected() -> Option<&'static str> { + Some(DA2_VERSION.get()?.as_ref()?.text.as_str()) +} + +/// [`detected`]'s reply as the packed integer the terminal sent +/// (`major * 10000 + minor * 100 + patch`). +pub fn detected_packed() -> Option { + Some(DA2_VERSION.get()?.as_ref()?.packed) +} + +/// Query DA2 once at startup and read the reply under a bounded deadline; +/// no-ops when the gate rejects the brand/multiplexer or stdin is not a TTY. +pub fn probe_at_startup() { + use std::io::IsTerminal; + + if DA2_VERSION.get().is_some() { + return; + } + let ctx = super::terminal_context(); + if !gate_allows_probe(ctx) || !std::io::stdin().is_terminal() { + let _ = DA2_VERSION.set(None); + return; + } + query_and_read(); +} + +/// Deliberately narrow: Alacritty is the only brand whose version is otherwise +/// unreachable, and it is excluded from [`super::xtversion`]'s allowlist, which +/// the synchronous read depends on. CSI-intercepting multiplexers skip — tmux +/// answers DA2 as itself, and passthrough still returns the reply through it. +fn gate_allows_probe(ctx: &super::TerminalContext) -> bool { + ctx.brand == super::TerminalName::Alacritty && !ctx.multiplexer.intercepts_csi_queries() +} + +#[cfg(unix)] +fn query_and_read() { + if !super::probe::write_query(QUERY) { + tracing::debug!("DA2 probe skipped: query write failed or output is not a TTY"); + let _ = DA2_VERSION.set(None); + return; + } + // Only the DA2 intro ends the read: startup typeahead can already hold a + // `>` and a `c` (`ls > out.c`), and a late DA1 reply has the escape but a + // `?`. Both are consumed instead, and the read continues to the reply. + let reply = super::probe::read_tty_reply(REPLY_TIMEOUT, |buf, byte| { + byte == b'c' && buf.windows(3).any(|w| w == b"\x1b[>") + }); + let version = reply.as_deref().and_then(parse_version); + if let Some(bytes) = reply.as_deref() + && version.is_none() + { + // A bare `None` cannot distinguish a rejected reply from silence. + let text = String::from_utf8_lossy(bytes); + tracing::debug!(reply = %text.escape_debug(), "DA2 reply rejected"); + } + tracing::info!(version = ?version.as_ref().map(|v| &v.text), "DA2 probe"); + let _ = DA2_VERSION.set(version); +} + +#[cfg(not(unix))] +fn query_and_read() { + // The timed read is Unix-only, and ConPTY does not answer DA2. + let _ = DA2_VERSION.set(None); +} + +/// Decode `CSI > Pp ; Pv ; Pc c`, rejecting anything that is not Alacritty's +/// exact reply shape. +/// +/// `Pv` means whatever its emulator decided — xterm puts a patch level there, +/// so `> 0 ; 388 ; 0 c` would decode to a confident, wrong `0.3.88`. The brand +/// evidence here is only `TERM=alacritty`, so the shape upstream hardcodes +/// (`Pp == 0`, `Pc == 1`) is what makes the number trustworthy. +#[cfg(any(unix, test))] +fn parse_version(reply: &[u8]) -> Option { + let text = String::from_utf8_lossy(reply); + // Split at the last `>` so a keystroke racing the reply cannot shift the + // parameter list. + let (_, params) = text.rsplit_once('>')?; + let mut fields = params.trim_end().trim_end_matches('c').split(';'); + if fields.next()?.trim() != "0" { + return None; + } + let packed: u32 = fields.next()?.trim().parse().ok()?; + if fields.next()?.trim() != "1" { + return None; + } + unpack_version(packed) +} + +/// For Alacritty the decoded value is the `alacritty_terminal` **library** +/// version, not the application release: upstream packs the library crate's own +/// `CARGO_PKG_VERSION`, and the two diverged after 0.5 — release 0.15.1 answers +/// `2500`. Reported as-is. Pre-release suffixes are stripped upstream, so a +/// `-dev` build is indistinguishable from the matching release. +#[cfg(any(unix, test))] +fn unpack_version(packed: u32) -> Option { + if packed == 0 || packed > MAX_PACKED_VERSION { + return None; + } + let major = packed / 10_000; + let minor = (packed / 100) % 100; + let patch = packed % 100; + Some(Da2Version { + packed, + text: format!("{major}.{minor}.{patch}"), + }) +} + +#[cfg(test)] +mod tests { + use super::*; + use crate::terminal::{MultiplexerKind, TerminalContext, TerminalName}; + + fn parsed(reply: &[u8]) -> Option<(u32, String)> { + parse_version(reply).map(|v| (v.packed, v.text)) + } + + #[test] + fn packed_version_round_trips() { + // Real `alacritty_terminal` versions, not the releases they ship in: + // 0.21 is Alacritty 0.13.x; 0.25 is 0.15.1+ (0.15.0 still shipped 0.24.2). + assert_eq!( + parsed(b"\x1b[>0;2100;1c"), + Some((2100, "0.21.0".to_owned())) + ); + assert_eq!( + parsed(b"\x1b[>0;2500;1c"), + Some((2500, "0.25.0".to_owned())) + ); + assert_eq!( + parsed(b"\x1b[>0;2601;1c"), + Some((2601, "0.26.1".to_owned())) + ); + // Typeahead consumed ahead of the reply: the last `>` is still the + // reply's, so its parameters are what get parsed. + assert_eq!( + parsed(b"ls > out.c\x1b[>0;2500;1c"), + Some((2500, "0.25.0".to_owned())) + ); + } + + #[test] + fn another_emulators_da2_is_not_a_version() { + // xterm's `Pv` is a patch level and VTE's is its own numbering; both + // would otherwise decode cleanly. + assert_eq!(parse_version(b"\x1b[>41;389;0c"), None); + assert_eq!(parse_version(b"\x1b[>0;388;0c"), None); + assert_eq!(parse_version(b"\x1b[>65;6003;1c"), None); + } + + #[test] + fn undecodable_payloads_are_none() { + // Absent, empty, or truncated parameter lists. + assert_eq!(parse_version(b""), None); + assert_eq!(parse_version(b"c"), None); + assert_eq!(parse_version(b"\x1b[>0c"), None); + assert_eq!(parse_version(b"\x1b[>0;;1c"), None); + assert_eq!(parse_version(b"\x1b[?62;1;6c"), None); + // Non-numeric, signed, and absurd values must not wrap or panic. + assert_eq!(parse_version(b"\x1b[>0;abc;1c"), None); + assert_eq!(parse_version(b"\x1b[>0;-1;1c"), None); + assert_eq!(parse_version(b"\x1b[>0;0;1c"), None); + assert_eq!(parse_version(b"\x1b[>0;4294967295;1c"), None); + assert_eq!(parse_version(b"\x1b[>0;99999999999999999999;1c"), None); + assert_eq!(parse_version(b"\x1b[>0;1000000;1c"), None); + assert_eq!(parse_version(b"\x1b[>0;\xff\xfe;1c"), None); + } + + /// The DA2 read would eat an XTVERSION reply in flight for the same brand. + /// Widening this gate onto a brand XTVERSION already probes is the edit + /// that would break it, so the complement is what gets asserted. + #[test] + fn no_brand_is_probed_by_both_xtversion_and_da2() { + use crate::terminal::xtversion; + + let ctx = |brand| TerminalContext { + brand, + multiplexer: MultiplexerKind::Undetected, + ..Default::default() + }; + assert!(gate_allows_probe(&ctx(TerminalName::Alacritty))); + assert!(!xtversion::gate_allows_probe(&ctx(TerminalName::Alacritty))); + + for brand in [ + TerminalName::Unknown, + TerminalName::Kitty, + TerminalName::WezTerm, + TerminalName::Ghostty, + TerminalName::Iterm2, + TerminalName::Rio, + ] { + // Keeps this hardcoded copy of the allowlist from going vacuous. + assert!( + xtversion::gate_allows_probe(&ctx(brand)), + "{brand:?} left the XTVERSION allowlist this asserts against" + ); + assert!( + !gate_allows_probe(&ctx(brand)), + "{brand:?} would be probed by both" + ); + } + } +} diff --git a/crates/codegen/xai-grok-pager-render/src/terminal/kitty_keyboard.rs b/crates/codegen/xai-grok-pager-render/src/terminal/kitty_keyboard.rs new file mode 100644 index 0000000..42532a1 --- /dev/null +++ b/crates/codegen/xai-grok-pager-render/src/terminal/kitty_keyboard.rs @@ -0,0 +1,167 @@ +//! Which Kitty keyboard enhancement flags the pager negotiates at startup, and +//! the process-global record of the set it actually pushed. +//! +//! "Flags pushed" is not "releases arrive", and conflating them is the bug this +//! module exists to prevent: Alacritty ≤ 0.14.x is pushed +//! `DISAMBIGUATE_ESCAPE_CODES` without `REPORT_EVENT_TYPES`, so the protocol is +//! live — `Shift+Enter` works, teardown owes a pop — yet no release ever comes, +//! and a hold-to-talk started there could only end on Esc. +//! +//! The gate reads the reported version rather than watching behaviour because +//! there is nothing to watch: a conforming terminal reports no release for these +//! keys either (kitty spec), so healthy and broken differ by one byte per +//! keystroke, gone by the time events are decoded. An earlier design died on it. +//! +//! Deliberately uncovered: [`super::da2`] is skipped under CSI-intercepting +//! multiplexers but [`super::TerminalContext::kitty_skip_reason`] is not, so an +//! affected Alacritty inside tmux ≥ 3.3 answers nothing, keeps +//! `REPORT_EVENT_TYPES` and still double-submits Enter. Downgrading everything +//! that answers nothing would cost far more healthy sessions than that slice. +//! +//! What losing `REPORT_EVENT_TYPES` costs an affected session: +//! +//! - Voice hold-to-talk degrades to a tap toggle (`voice_chord_action`; the +//! `voice_capture_mode` setting hides its `hold` choice). +//! - `is_link_modifier_for_key`'s non-macOS Ctrl-release case (`xai-grok-pager` +//! `src/app/agent_view/mod.rs`) never fires, so link-hover clears on the next +//! non-Ctrl key instead of when Ctrl lifts. +//! - `KeyEventKind::Repeat` disappears: held keys arrive as repeated `Press`, as +//! on every non-KKP terminal — but `is_pasteable_key_event` (`xai-grok-pager` +//! `src/app/event_loop.rs`) excludes `Repeat` on purpose, so auto-repeat +//! counts toward paste coalescing again. + +use std::sync::atomic::{AtomicU8, Ordering}; + +use crossterm::event::KeyboardEnhancementFlags; + +/// Highest packed `alacritty_terminal` version that mis-encodes +/// `REPORT_EVENT_TYPES`: the *release* of Backspace, Tab, Enter and Escape comes +/// back as a duplicate legacy byte, which carries no event type, so crossterm +/// reads it as a second `Press` and one keypress acts twice — Enter submits +/// twice. (Upstream's CHANGELOG omits Escape; its `key_release` arm does not.) +/// +/// DA2 reports the **library** version, not the Alacritty release: 0.14.0 ships +/// 0.24.1 → `2401`, 0.15.0 ships 0.24.2 → `2402`. Fixed by `7bda13b8aa` +/// (2025-01-04); CHANGELOG **v0.15.0 → Fixed**: *"Report of Enter/Tab/Backspace +/// in kitty keyboard's report event types mode."* There is no `v0.14.1`, so +/// 0.14.0 is the whole affected *release* population — this threshold never +/// moves, it only gets retired. +/// +/// Git builds escape it: master carried `0.24.2-dev` from 2024-10-18 to +/// 2025-01-09 and the suffix is stripped before packing, so a pre-fix build from +/// that window reports `2402`. +pub const ALACRITTY_BROKEN_EVENT_TYPES_MAX_PACKED: u32 = 2401; + +/// The flags to push at startup; empty means push nothing. +/// +/// An unknown version never downgrades: DA2 is skipped under multiplexers and +/// off unix, so `None` is common and the downgrade costs the features listed in +/// the module docs. Only a positively identified affected version gets it. +/// +/// The missing brand check is load-bearing: [`super::da2`]'s probe gate admits +/// Alacritty alone, so a version in hand already implies the brand, and widening +/// that gate silently widens this one. +pub fn negotiated_kitty_flags( + skip_reason: Option<&str>, + da2_packed: Option, +) -> KeyboardEnhancementFlags { + if skip_reason.is_some() { + return KeyboardEnhancementFlags::empty(); + } + let mut flags = KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES; + let mis_encodes_releases = + da2_packed.is_some_and(|packed| packed <= ALACRITTY_BROKEN_EVENT_TYPES_MAX_PACKED); + if !mis_encodes_releases { + flags |= KeyboardEnhancementFlags::REPORT_EVENT_TYPES; + } + flags +} + +/// Bits of the [`KeyboardEnhancementFlags`] `init_terminal` pushed; `0` is +/// `empty()`. Storing the set actually sent, rather than a classification of it, +/// is what keeps the predicates below from drifting apart. +/// +/// `Relaxed`: the cell publishes no other memory, and readers are already +/// ordered after `init_terminal` by the task creation between them. +static PUSHED_KITTY_FLAGS: AtomicU8 = AtomicU8::new(0); + +fn pushed_kitty_flags() -> KeyboardEnhancementFlags { + KeyboardEnhancementFlags::from_bits_truncate(PUSHED_KITTY_FLAGS.load(Ordering::Relaxed)) +} + +pub fn set_pushed_kitty_flags(flags: KeyboardEnhancementFlags) { + PUSHED_KITTY_FLAGS.store(flags.bits(), Ordering::Relaxed); +} + +/// Whether Kitty keyboard enhancement flags were actually pushed during +/// `init_terminal` — i.e. the brand wasn't in the skip list *and* the +/// runtime probe (`supports_keyboard_enhancement`) succeeded. False means +/// modified keys (Shift+Enter, Ctrl+.) arrive as legacy bytes. +/// +/// This is **not** "key releases arrive" — use [`kitty_releases_reported`]. +pub fn kitty_flags_pushed() -> bool { + !pushed_kitty_flags().is_empty() +} + +/// Whether the terminal reports key *release* events. Every feature that waits +/// for a release (hold-to-talk, modifier-lift tracking) must gate on this, not +/// on [`kitty_flags_pushed`]: the two differ on Alacritty ≤ 0.14.x. +pub fn kitty_releases_reported() -> bool { + pushed_kitty_flags().contains(KeyboardEnhancementFlags::REPORT_EVENT_TYPES) +} + +/// Whether the version workaround engaged: pushed, but without +/// `REPORT_EVENT_TYPES`. Not `!kitty_releases_reported()`, which is also true +/// when nothing was pushed at all. +pub fn kitty_event_types_withheld() -> bool { + let flags = pushed_kitty_flags(); + !flags.is_empty() && !flags.contains(KeyboardEnhancementFlags::REPORT_EVENT_TYPES) +} + +/// Clears the record as it reads, so concurrent teardown paths cannot both pop. +pub fn take_kitty_flags_pushed() -> bool { + PUSHED_KITTY_FLAGS.swap(0, Ordering::Relaxed) != 0 +} + +#[cfg(test)] +mod tests { + use super::*; + + const DISAMBIGUATE: KeyboardEnhancementFlags = + KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES; + const EVENT_TYPES: KeyboardEnhancementFlags = KeyboardEnhancementFlags::REPORT_EVENT_TYPES; + + /// The exact boundary a careless `<`/`<=` edit breaks: `alacritty_terminal` + /// 0.24.1 is Alacritty 0.14.0 (broken), 0.24.2 is 0.15.0 (fixed). + #[test] + fn downgrade_boundary_is_the_last_broken_library_version() { + // Non-empty either side: a downgrade is still a push, teardown owes a pop. + assert_eq!(negotiated_kitty_flags(None, Some(2401)), DISAMBIGUATE); + assert_eq!( + negotiated_kitty_flags(None, Some(2402)), + DISAMBIGUATE | EVENT_TYPES + ); + } + + /// DA2 is skipped under multiplexers and off unix, so "no answer" is the + /// common case and must not cost a healthy terminal its release events. + #[test] + fn absent_version_does_not_downgrade() { + assert_eq!( + negotiated_kitty_flags(None, None), + DISAMBIGUATE | EVENT_TYPES + ); + } + + /// A skip reason outranks any version, so teardown owes no pop. + #[test] + fn a_skip_reason_pushes_nothing() { + for packed in [None, Some(2401), Some(2402)] { + assert_eq!( + negotiated_kitty_flags(Some("vscode"), packed), + KeyboardEnhancementFlags::empty(), + "da2_packed={packed:?}" + ); + } + } +} diff --git a/crates/codegen/xai-grok-pager-render/src/terminal/mod.rs b/crates/codegen/xai-grok-pager-render/src/terminal/mod.rs index 79a1e47..467d13f 100644 --- a/crates/codegen/xai-grok-pager-render/src/terminal/mod.rs +++ b/crates/codegen/xai-grok-pager-render/src/terminal/mod.rs @@ -5,14 +5,15 @@ use std::collections::HashMap; use std::sync::OnceLock; -use std::sync::atomic::{AtomicBool, Ordering}; use crate::host::HostOs; +pub mod da2; pub mod embedded_editor; pub mod hyperlinks; pub mod image; pub mod keyboard; +pub mod kitty_keyboard; pub mod overlay; pub(crate) mod probe; pub mod term_version; @@ -28,6 +29,10 @@ pub use keyboard::{ KeyboardCapabilities, ModifierDelivery, ModifierFate, keyboard_capabilities, keyboard_capabilities_for_host, }; +pub use kitty_keyboard::{ + kitty_event_types_withheld, kitty_flags_pushed, kitty_releases_reported, + negotiated_kitty_flags, set_pushed_kitty_flags, take_kitty_flags_pushed, +}; pub use term_version::{TermVersion, TermVersionSource}; #[cfg(test)] @@ -43,32 +48,6 @@ pub(crate) fn env_from(pairs: &[(&str, &str)]) -> HashMap { .collect() } -// TODO: make term seq codes invariant in a crate. -/// Tracks whether Kitty keyboard enhancement flags were pushed during -/// `init_terminal`, so teardown paths (`restore_terminal`, panic hook) -/// only pop when flags were actually pushed. -static KITTY_FLAGS_PUSHED: AtomicBool = AtomicBool::new(false); - -/// Whether Kitty keyboard enhancement flags were actually pushed during -/// `init_terminal` — i.e. the brand wasn't in the skip list *and* the -/// runtime probe (`supports_keyboard_enhancement`) succeeded. False means -/// modified keys (Shift+Enter, Ctrl+.) arrive as legacy bytes. -pub fn kitty_flags_pushed() -> bool { - KITTY_FLAGS_PUSHED.load(Ordering::Acquire) -} - -/// Record whether Kitty keyboard enhancement flags were pushed during -/// `init_terminal`. -pub fn set_kitty_flags_pushed(v: bool) { - KITTY_FLAGS_PUSHED.store(v, Ordering::Release) -} - -/// Atomically clear the Kitty-flags-pushed state, returning the prior value. -/// Used by teardown paths so concurrent callers cannot both pop. -pub fn take_kitty_flags_pushed() -> bool { - KITTY_FLAGS_PUSHED.swap(false, Ordering::AcqRel) -} - /// Known terminal emulator categories. #[derive(Clone, Copy, Debug, Default, Eq, PartialEq, strum::Display)] pub enum TerminalName { @@ -573,14 +552,10 @@ impl TerminalContext { /// The best available terminal version and the source that reported it. /// - /// Only the environment arm exists today. Final precedence is - /// `da2 > xtversion > env` — probe arms insert **above** it, since a live - /// self-report cannot be inherited or go stale. + /// Not pure: the DA2 arm reads process-global probe state, so env-precedence + /// tests hold only while no reply has been recorded in the process. pub fn term_version(&self) -> (String, TermVersionSource) { - match &self.env_term_version { - Some(v) => (v.version.clone(), v.source), - None => (String::new(), TermVersionSource::None), - } + term_version::best_term_version(da2::detected(), self.env_term_version.as_ref()) } /// Extract a flat snapshot of terminal details for telemetry. @@ -605,6 +580,7 @@ impl TerminalContext { xtversion: xtversion::detected().unwrap_or("").to_owned(), term_version, term_version_source: term_version_source.to_string(), + kitty_event_types_withheld: kitty_event_types_withheld(), hyperlink_osc8: self.hyperlink_capabilities().osc8.to_string(), hyperlink_skip_reason: self.hyperlink_skip_reason().unwrap_or("none").to_owned(), clipboard_route: route.to_string(), @@ -622,12 +598,16 @@ impl TerminalContext { Some(v) if self.brand == TerminalName::Unknown => format!("Unknown (XTVERSION: {v})"), _ => self.brand.to_string(), }; + // Raw and unlabeled by design: no provenance, and no rewrite of DA2's + // library version into an Alacritty release number. + let (term_version, _source) = self.term_version(); FeedbackTerminalInfo { brand, multiplexer: self.multiplexer.to_string(), is_ssh: self.is_ssh, is_byobu: self.is_byobu(), term_var: self.term_var_or_na().to_owned(), + term_version: (!term_version.is_empty()).then_some(term_version), tmux_version: if self.is_tmux_backed() { self.tmux_version.clone() } else { diff --git a/crates/codegen/xai-grok-pager-render/src/terminal/term_version.rs b/crates/codegen/xai-grok-pager-render/src/terminal/term_version.rs index ef33962..ac723cb 100644 --- a/crates/codegen/xai-grok-pager-render/src/terminal/term_version.rs +++ b/crates/codegen/xai-grok-pager-render/src/terminal/term_version.rs @@ -15,13 +15,15 @@ use std::collections::HashMap; use super::{TerminalName, terminal_name_from_term_program}; -/// Which environment variable produced a [`TermVersion`]. +/// Which source produced a [`TermVersion`]. /// /// The rendered labels are stable telemetry values — do not rename them. #[derive(Clone, Copy, Debug, Eq, PartialEq, strum::Display)] #[strum(serialize_all = "snake_case")] pub enum TermVersionSource { None, + /// The runtime [`crate::terminal::da2`] probe — the only non-env source. + Da2, /// `TERM_PROGRAM_VERSION`, or its SSH-surviving `LC_TERMINAL_VERSION` /// mirror (iTerm2 only). TermProgram, @@ -64,6 +66,19 @@ fn corroborates(named: TerminalName, env_brand: TerminalName) -> bool { )) } +/// Pick the best available version: a runtime probe outranks the environment, +/// since a live self-report cannot be inherited across a process, SSH or +/// multiplexer boundary, nor go stale. XTVERSION has no arm — its payload is a +/// name-and-version string, and it rides `TerminalTelemetry::xtversion`. +pub(super) fn best_term_version( + da2: Option<&str>, + env_version: Option<&TermVersion>, +) -> (String, TermVersionSource) { + da2.map(|version| (version.to_owned(), TermVersionSource::Da2)) + .or_else(|| env_version.map(|v| (v.version.clone(), v.source))) + .unwrap_or_else(|| (String::new(), TermVersionSource::None)) +} + /// Look up an env value, trimmed; `env_get` alone would pass whitespace. fn env_trimmed<'a>(env: &'a HashMap, key: &str) -> Option<&'a str> { let value = super::env_get(env, key)?.trim(); @@ -136,11 +151,45 @@ mod tests { #[test] fn source_labels_are_pinned() { assert_eq!(TermVersionSource::None.to_string(), "none"); + assert_eq!(TermVersionSource::Da2.to_string(), "da2"); assert_eq!(TermVersionSource::TermProgram.to_string(), "term_program"); assert_eq!(TermVersionSource::WezTerm.to_string(), "wezterm"); assert_eq!(TermVersionSource::Vte.to_string(), "vte"); } + /// Driven through `best_term_version` rather than the probe's process-global + /// `OnceLock`: this crate's tests share one process, so recording a reply + /// would race every env-precedence assertion below. + #[test] + fn a_probed_version_outranks_env() { + let env = TermVersion::new("7402", TermVersionSource::Vte); + assert_eq!( + best_term_version(Some("0.25.0"), Some(&env)), + ("0.25.0".to_owned(), TermVersionSource::Da2) + ); + assert_eq!( + best_term_version(None, Some(&env)), + ("7402".to_owned(), TermVersionSource::Vte) + ); + assert_eq!( + best_term_version(None, None), + (String::new(), TermVersionSource::None) + ); + } + + /// The version has to reach the feedback card; its source has no field on + /// the wire type to reach it through. + #[test] + fn feedback_info_carries_the_version() { + let present = + build_terminal_context_from_env(&env_from(&[("VTE_VERSION", "7402")])).feedback_info(); + assert_eq!(present.term_version.as_deref(), Some("7402")); + + let absent = build_terminal_context_from_env(&env_from(&[("TERM", "xterm-256color")])) + .feedback_info(); + assert_eq!(absent.term_version, None); + } + #[test] fn term_program_version_wins_when_term_program_names_the_brand() { let (version, source) = resolved(&[ diff --git a/crates/codegen/xai-grok-pager-render/src/terminal/xtversion.rs b/crates/codegen/xai-grok-pager-render/src/terminal/xtversion.rs index 2c7fbce..317200a 100644 --- a/crates/codegen/xai-grok-pager-render/src/terminal/xtversion.rs +++ b/crates/codegen/xai-grok-pager-render/src/terminal/xtversion.rs @@ -96,7 +96,9 @@ pub fn probe_at_startup() { /// CSI-intercepting multiplexers skip — the innermost layer answers as /// itself, which the `multiplexer` field already records. Transparent muxes /// (e.g. cmux) need no special case. -fn gate_allows_probe(ctx: &super::TerminalContext) -> bool { +/// +/// `pub(super)` for [`super::da2`], which must stay disjoint from this list. +pub(super) fn gate_allows_probe(ctx: &super::TerminalContext) -> bool { use super::TerminalName::*; matches!( ctx.brand, diff --git a/crates/codegen/xai-grok-pager/docs/user-guide/02-authentication.md b/crates/codegen/xai-grok-pager/docs/user-guide/02-authentication.md index 08313b9..3e23ca4 100644 --- a/crates/codegen/xai-grok-pager/docs/user-guide/02-authentication.md +++ b/crates/codegen/xai-grok-pager/docs/user-guide/02-authentication.md @@ -271,7 +271,8 @@ During a session, the active method handles all mid-session refreshes. ## Related settings -`/privacy` does not change these config knobs: +Coding-data sharing — **Coding data, retention, and training** in Settings, +which `/privacy` opens — does not change these config knobs: | Setting | How to set it | |---------|---------------| @@ -279,10 +280,11 @@ During a session, the active method handles all mid-session refreshes. | `[telemetry] trace_upload` | `config.toml` or `GROK_TELEMETRY_TRACE_UPLOAD` | | External OpenTelemetry | `GROK_EXTERNAL_OTEL` / `[telemetry] otel_*`. See [Monitoring Usage](24-monitoring-usage.md). | -On team accounts, only a team admin can toggle privacy with `/privacy`. +On team accounts, only a team admin can change coding-data sharing. Team admins can also enable or disable Zero Data Retention (ZDR) for their team. See [How to enable ZDR](https://docs.x.ai/developers/faq/security#how-to-enable-zdr). -When ZDR is on, `/privacy` cannot change coding-data sharing. +When ZDR is on, coding-data sharing cannot be changed at all — the settings +row shows `ZDR` in place of the value. See [Monitoring Usage](24-monitoring-usage.md#related-settings) and [Configuration](05-configuration.md#telemetry). diff --git a/crates/codegen/xai-grok-pager/docs/user-guide/04-slash-commands.md b/crates/codegen/xai-grok-pager/docs/user-guide/04-slash-commands.md index c642b48..8602330 100644 --- a/crates/codegen/xai-grok-pager/docs/user-guide/04-slash-commands.md +++ b/crates/codegen/xai-grok-pager/docs/user-guide/04-slash-commands.md @@ -396,15 +396,14 @@ View credit usage or manage billing. Alias: `/cost`. ### `/privacy` -Show or toggle privacy and data-retention status. +Open Settings on **Coding data, retention, and training**, where you choose +**Opt in** or **Opt out**. Takes no arguments. ``` /privacy -/privacy opt-in -/privacy opt-out ``` -`/privacy` doesn't touch `[features] telemetry`, `trace_upload`, or your external OTEL settings — see [Monitoring Usage](24-monitoring-usage.md#related-settings). On team accounts, only a team admin can toggle privacy this way, and admins can also enable or disable Zero Data Retention for the team ([how to enable ZDR](https://docs.x.ai/developers/faq/security#how-to-enable-zdr)). +This setting doesn't touch `[features] telemetry`, `trace_upload`, or your external OTEL settings — see [Monitoring Usage](24-monitoring-usage.md#related-settings). On team accounts only a team admin can change it, and admins can also enable or disable Zero Data Retention for the team ([how to enable ZDR](https://docs.x.ai/developers/faq/security#how-to-enable-zdr)). When the choice isn't yours to make, the row says so — `ZDR` or `· Admin Managed` — instead of opening the chooser. --- diff --git a/crates/codegen/xai-grok-pager/docs/user-guide/05-configuration.md b/crates/codegen/xai-grok-pager/docs/user-guide/05-configuration.md index e6d4311..a746dc6 100644 --- a/crates/codegen/xai-grok-pager/docs/user-guide/05-configuration.md +++ b/crates/codegen/xai-grok-pager/docs/user-guide/05-configuration.md @@ -486,7 +486,7 @@ Keyboard shortcuts are **not** configurable — all bindings are built in. See [ These are independent knobs (see [Monitoring Usage](24-monitoring-usage.md#related-settings)): - **`[features] telemetry`** / `GROK_TELEMETRY_ENABLED` — the product-analytics master switch. `/privacy` doesn't change it. -- **`/privacy`** / Settings — coding-data sharing, separate from telemetry. +- **Coding data, retention, and training** — the Settings row `/privacy` opens; coding-data sharing, separate from telemetry. - **`[telemetry] trace_upload`** / `GROK_TELEMETRY_TRACE_UPLOAD` — session traces; follows telemetry when unset. - **`[telemetry] otel_*`** / `GROK_EXTERNAL_OTEL` — external OTEL to your own collector (below). diff --git a/crates/codegen/xai-grok-pager/docs/user-guide/07-mcp-servers.md b/crates/codegen/xai-grok-pager/docs/user-guide/07-mcp-servers.md index 5fae679..751b083 100644 --- a/crates/codegen/xai-grok-pager/docs/user-guide/07-mcp-servers.md +++ b/crates/codegen/xai-grok-pager/docs/user-guide/07-mcp-servers.md @@ -109,6 +109,10 @@ grok mcp add --transport sse linear https://mcp.linear.app/sse # Remove a server grok mcp remove github +# Enable or disable a local/TOML (or compat-sourced) server +grok mcp enable github +grok mcp disable github + # Diagnose a server's configuration and connectivity grok mcp doctor # Check every configured server grok mcp doctor github # Check one server @@ -117,10 +121,16 @@ grok mcp doctor --json # Machine-readable output The transport defaults to `stdio`; pass `--transport http` or `--transport sse` for remote servers. -By default `grok mcp add` writes to `~/.grok/config.toml` (`--scope user`). Use `--scope project` to write to `.grok/config.toml` in the current directory instead, which can be committed and shared with your team (see [Project-Scoped MCP Servers](#project-scoped-mcp-servers)). Header and environment variable values are stored verbatim, so reference secrets as `${VAR}` instead of pasting them into a committed project config (see [Example Configurations](#example-configurations)). `grok mcp list` shows servers from both scopes, marking project-scoped ones with `(project)`. +By default `grok mcp add` writes to `~/.grok/config.toml` (`--scope user`). Use `--scope project` to write to `.grok/config.toml` in the current directory instead, which can be committed and shared with your team (see [Project-Scoped MCP Servers](#project-scoped-mcp-servers)). Header and environment variable values are stored verbatim, so reference secrets as `${VAR}` instead of pasting them into a committed project config (see [Example Configurations](#example-configurations)). `grok mcp list` shows servers from both scopes, marking project-scoped ones with `(project)` and disabled ones with `(disabled)`. `grok mcp remove` searches both scopes and exits 0 after removing the server. It exits 1 when the name is not found, or when the name is defined in both user and project scope — pass `--scope` to say which one to remove. +`grok mcp enable` / `disable` persist the personal on/off state to user `~/.grok/config.toml` (`disabled_mcp_servers`, and `[mcp_servers.].enabled` when that entry exists). Scope: + +- **Known names:** user/project Grok TOML, names already on the disabled list, compat sources (`.mcp.json`, Claude, Cursor), **plugin** MCP servers (same discovery as doctor/`/mcps`), and legacy managed `grok_com_*` (no local entry required). +- **Enable only:** if the cwd-nearest project definition has sticky `enabled = false`, that single key is cleared (comments preserved); disable never rewrites project configs. +- **Not full `/mcps` parity:** gateway connectors (`managed_gateway:…`, stored under `disabled_mcp_tools.__managed_gateway_connectors`) stay Space-only in the TUI. Idempotent; unknown names exit 1. + Breaking changes from earlier releases: `--env` now takes one `KEY=value` per flag (use `-e A=1 -e B=2`, not `--env A=1 B=2`), and server names may only contain letters, numbers, hyphens, and underscores. --- @@ -171,7 +181,7 @@ MCP tools are namespaced with the server name to avoid collisions: ## Toggle Servers at Runtime -You can enable or disable MCP servers during a session without restarting Grok. +You can enable or disable MCP servers without restarting Grok (TUI `/mcps` or CLI — see [CLI Management](#cli-management)). ### The /mcps Modal diff --git a/crates/codegen/xai-grok-pager/docs/user-guide/24-monitoring-usage.md b/crates/codegen/xai-grok-pager/docs/user-guide/24-monitoring-usage.md index bd19e05..7172fed 100644 --- a/crates/codegen/xai-grok-pager/docs/user-guide/24-monitoring-usage.md +++ b/crates/codegen/xai-grok-pager/docs/user-guide/24-monitoring-usage.md @@ -16,7 +16,7 @@ These knobs are independent of each other (and of this guide's external OTEL str | Setting | How to set it | |---------|---------------| | Telemetry master switch | `[features] telemetry` / `GROK_TELEMETRY_ENABLED` | -| `/privacy` | `/privacy opt-in` / `/privacy opt-out`, or Settings | +| Coding data, retention, and training | Settings — `/privacy` opens the row | | Trace upload | `[telemetry] trace_upload` / `GROK_TELEMETRY_TRACE_UPLOAD` | | External OpenTelemetry | `GROK_EXTERNAL_OTEL` / `[telemetry] otel_*` (this guide) | diff --git a/crates/codegen/xai-grok-pager/src/app/actions.rs b/crates/codegen/xai-grok-pager/src/app/actions.rs index 292ec95..8c9b9bb 100644 --- a/crates/codegen/xai-grok-pager/src/app/actions.rs +++ b/crates/codegen/xai-grok-pager/src/app/actions.rs @@ -595,14 +595,15 @@ pub enum Action { /// Open the settings modal (F2, `/settings`, command palette). /// If already open, closes it instead of stacking. OpenSettings, - /// Open settings focused on a registry key (e.g. privacy banner Customize). + /// Open settings on a registry key: its chooser, or the browse row when + /// the setting is locked. OpenSettingsFocus { key: &'static str, }, - /// Welcome privacy banner Accept (opt-in; ack after ACP success). - PrivacyBannerAccept, - /// Welcome privacy banner Customize (ack + open settings on coding_data_sharing). - PrivacyBannerCustomize, + /// Privacy banner `[Opt in]` (ack only after ACP success). + PrivacyBannerOptIn, + /// Privacy banner `[Opt out]` (ack now, then record the decline). + PrivacyBannerOptOut, /// Open the command palette (`/help`). The keybinding path (Ctrl+P) opens it /// directly in `handle_agent_action`; this lets a slash command reach the /// same modal through dispatch. @@ -735,8 +736,6 @@ pub enum Action { TriggerDeepSearch, /// Force an immediate deep content search, skipping the debounce. ForceDeepSearch, - /// Show privacy and data retention status. - ShowPrivacyInfo, SetCodingDataSharing { opted_in: bool, }, @@ -1994,6 +1993,11 @@ pub enum Effect { opted_in: bool, /// Pre-toggle value to revert to on failure. rollback_to_opted_in: bool, + /// Write generation, echoed back on the `TaskResult`. Writes to this + /// endpoint are concurrent, so a result that isn't the newest must + /// not touch state: its `rollback_to_opted_in` was captured against + /// a world that has since moved on. + seq: u64, }, /// Rename the current session. RenameSession { @@ -2560,12 +2564,14 @@ pub enum TaskResult { CodingDataSharingUpdated { agent_id: AgentId, opted_in: bool, + seq: u64, }, /// Coding data sharing update failed. CodingDataSharingFailed { agent_id: AgentId, error: String, rollback_to_opted_in: bool, + seq: u64, }, /// Session rename completed successfully. RenameSessionComplete { diff --git a/crates/codegen/xai-grok-pager/src/app/agent_view/links.rs b/crates/codegen/xai-grok-pager/src/app/agent_view/links.rs index 5f03ac4..741357d 100644 --- a/crates/codegen/xai-grok-pager/src/app/agent_view/links.rs +++ b/crates/codegen/xai-grok-pager/src/app/agent_view/links.rs @@ -606,39 +606,51 @@ mod link_click_tests { ); let rect = agent .privacy_banner - .hit_accept + .hit_opt_in .rect .expect("accept rect armed"); let outcome = agent.handle_input(&Event::Mouse(mouse_down(rect.x + 1, rect.y)), ®); assert!(matches!( outcome, - InputOutcome::Action(Action::PrivacyBannerAccept) + InputOutcome::Action(Action::PrivacyBannerOptIn) )); let rect = agent .privacy_banner - .hit_customize + .hit_opt_out .rect .expect("customize rect armed"); let outcome = agent.handle_input(&Event::Mouse(mouse_down(rect.x + 1, rect.y)), ®); assert!(matches!( outcome, - InputOutcome::Action(Action::PrivacyBannerCustomize) + InputOutcome::Action(Action::PrivacyBannerOptOut) )); let rect = agent .privacy_banner - .hit_legal + .hit_terms .rect - .expect("legal rect armed"); + .expect("terms rect armed"); let outcome = agent.handle_input(&Event::Mouse(mouse_down(rect.x + 1, rect.y)), ®); assert!(matches!( outcome, InputOutcome::Action(Action::OpenUrl(ref url)) - if url == crate::views::privacy_banner::PRIVACY_BANNER_LEGAL_URL + if url == crate::views::privacy_banner::PRIVACY_BANNER_TERMS_URL + )); + let rect = agent + .privacy_banner + .hit_policy + .rect + .expect("privacy policy rect armed"); + let outcome = agent.handle_input(&Event::Mouse(mouse_down(rect.x + 1, rect.y)), ®); + assert!(matches!( + outcome, + InputOutcome::Action(Action::OpenUrl(ref url)) + if url == crate::views::privacy_banner::PRIVACY_BANNER_POLICY_URL )); draw_frame_privacy(&mut agent, ®, &critical, 2, 80, false); - assert!(agent.privacy_banner.hit_accept.rect.is_none()); - assert!(agent.privacy_banner.hit_customize.rect.is_none()); - assert!(agent.privacy_banner.hit_legal.rect.is_none()); + assert!(agent.privacy_banner.hit_opt_in.rect.is_none()); + assert!(agent.privacy_banner.hit_opt_out.rect.is_none()); + assert!(agent.privacy_banner.hit_terms.rect.is_none()); + assert!(agent.privacy_banner.hit_policy.rect.is_none()); assert!(agent.hit_announcement_hide.rect.is_some()); } /// Promo twin of the [hide] suppression test: the [label] CTA rect must diff --git a/crates/codegen/xai-grok-pager/src/app/agent_view/mod.rs b/crates/codegen/xai-grok-pager/src/app/agent_view/mod.rs index 5f5ea13..19be97d 100644 --- a/crates/codegen/xai-grok-pager/src/app/agent_view/mod.rs +++ b/crates/codegen/xai-grok-pager/src/app/agent_view/mod.rs @@ -275,23 +275,26 @@ pub struct HitArea { /// Privacy upsell banner state on the agent view: whether the banner owns /// the banner slot this frame (`active`, set at draw start like /// `session_banner_active`; persists until acted on, so it is a tip -/// occluder AND a tip-tick freezer) plus the three click targets. +/// occluder AND a tip-tick freezer) plus the four click targets. #[derive(Debug, Default)] pub struct PrivacyBannerState { pub(crate) active: bool, - /// `[Accept]` (opt in; ack after ACP success). - pub(crate) hit_accept: HitArea, - /// `[Customize in settings]` (ack + open settings on coding_data_sharing). - pub(crate) hit_customize: HitArea, - /// Legal links line (opens the legal URL). - pub(crate) hit_legal: HitArea, + /// `[Opt in]` (opt in; ack only after ACP success). + pub(crate) hit_opt_in: HitArea, + /// `[Opt out]` (ack now; record the decline). + pub(crate) hit_opt_out: HitArea, + /// "Terms" link (opens the terms of service). + pub(crate) hit_terms: HitArea, + /// "Privacy Policy" link (opens the privacy policy). + pub(crate) hit_policy: HitArea, } impl PrivacyBannerState { /// Drop all click targets (slot not painted this frame). pub fn clear_hits(&mut self) { - self.hit_accept.clear(); - self.hit_customize.clear(); - self.hit_legal.clear(); + self.hit_opt_in.clear(); + self.hit_opt_out.clear(); + self.hit_terms.clear(); + self.hit_policy.clear(); } } /// Banner-slot inputs to [`AgentView::draw`]. Slot precedence is computed diff --git a/crates/codegen/xai-grok-pager/src/app/agent_view/render.rs b/crates/codegen/xai-grok-pager/src/app/agent_view/render.rs index 20576f1..50e30cd 100644 --- a/crates/codegen/xai-grok-pager/src/app/agent_view/render.rs +++ b/crates/codegen/xai-grok-pager/src/app/agent_view/render.rs @@ -870,6 +870,11 @@ impl AgentView { } else { banner_height }; + let banner_height = if privacy_banner { + banner_height.max(crate::views::privacy_banner::height(inner_width)) + } else { + banner_height + }; let tip_row_visible = self.ephemeral_tip_renderable(area.height) && self.ephemeral_tip.is_active(); let banner_height = banner_height.max(u16::from(tip_row_visible)); @@ -2063,7 +2068,8 @@ impl AgentView { self.hit_watching_cue.clear(); self.hit_plan_approval_status.clear(); } - let privacy_banner_owns_slot = privacy_banner && layout.banner.height >= 2; + let privacy_banner_owns_slot = + privacy_banner && layout.banner.height >= crate::views::privacy_banner::MIN_HEIGHT; if !privacy_banner_owns_slot { self.privacy_banner.clear_hits(); } @@ -2072,14 +2078,17 @@ impl AgentView { self.hit_announcement_cta.clear(); let rects = crate::views::privacy_banner::render(layout.banner, buf, &theme, mouse_pos); self.privacy_banner - .hit_accept - .set_unless_dropdown(Some(rects.accept), dropdown_open); + .hit_opt_in + .set_unless_dropdown(Some(rects.opt_in), dropdown_open); self.privacy_banner - .hit_customize - .set_unless_dropdown(Some(rects.customize), dropdown_open); + .hit_opt_out + .set_unless_dropdown(Some(rects.opt_out), dropdown_open); self.privacy_banner - .hit_legal - .set_unless_dropdown(Some(rects.legal), dropdown_open); + .hit_terms + .set_unless_dropdown(Some(rects.terms), dropdown_open); + self.privacy_banner + .hit_policy + .set_unless_dropdown(Some(rects.policy), dropdown_open); } else if let Some((ref msg, remaining)) = self.mode_switch_banner { self.hit_announcement_hide.clear(); self.hit_announcement_cta.clear(); diff --git a/crates/codegen/xai-grok-pager/src/app/app_view.rs b/crates/codegen/xai-grok-pager/src/app/app_view.rs index 73576dd..ee0726e 100644 --- a/crates/codegen/xai-grok-pager/src/app/app_view.rs +++ b/crates/codegen/xai-grok-pager/src/app/app_view.rs @@ -858,9 +858,10 @@ pub struct AppView { /// Hit-test rect for the welcome hero upgrade CTA `[label]` button /// (click → `AnnouncementsOpenCta(Welcome)`). pub welcome_upgrade_cta_rect: Option, - pub welcome_privacy_banner_accept_rect: Option, - pub welcome_privacy_banner_customize_rect: Option, - pub welcome_privacy_banner_legal_rect: Option, + pub welcome_privacy_banner_opt_in_rect: Option, + pub welcome_privacy_banner_opt_out_rect: Option, + pub welcome_privacy_banner_terms_rect: Option, + pub welcome_privacy_banner_policy_rect: Option, /// Transient welcome toast: (message, wall-clock expiry). pub welcome_toast: Option<(String, std::time::Instant)>, /// Sticky hover flag for the privacy banner buttons (redraw on enter/leave). @@ -1064,7 +1065,12 @@ pub struct AppView { /// Local `[privacy].privacy_banner_acked` (RFC 3339 UTC). pub privacy_banner_acked: Option, /// Accept awaits ACP success before ack. - pub privacy_banner_accept_inflight: bool, + pub privacy_banner_opt_in_inflight: bool, + /// Newest `SetCodingDataSharing` write. Bumped per dispatch and echoed + /// on the `TaskResult`, so an older write's late reply — whose + /// `rollback_to_opted_in` was captured before the newer one — cannot + /// clobber the current value. + pub coding_data_write_seq: u64, /// Persisted `[cli].show_tips` mirror. `None` = no override (default `true`). pub show_tips: Option, /// Persisted `[cli].auto_update` mirror. `None` = no override (default `true`). @@ -1442,9 +1448,10 @@ impl AppView { welcome_refresh_rect: None, welcome_gate_url_rect: None, welcome_upgrade_cta_rect: None, - welcome_privacy_banner_accept_rect: None, - welcome_privacy_banner_customize_rect: None, - welcome_privacy_banner_legal_rect: None, + welcome_privacy_banner_opt_in_rect: None, + welcome_privacy_banner_opt_out_rect: None, + welcome_privacy_banner_terms_rect: None, + welcome_privacy_banner_policy_rect: None, welcome_toast: None, welcome_on_privacy_banner: false, welcome_on_upgrade_cta: false, @@ -1519,7 +1526,8 @@ impl AppView { privacy_notice_rollout: false, privacy_banner_reshow_days: None, privacy_banner_acked: None, - privacy_banner_accept_inflight: false, + privacy_banner_opt_in_inflight: false, + coding_data_write_seq: 0, show_tips: None, auto_update: None, ask_user_question_timeout_enabled: None, @@ -2459,11 +2467,10 @@ impl AppView { refresh_rect: self.welcome_refresh_rect.as_ref(), gate_url_rect: self.welcome_gate_url_rect.as_ref(), upgrade_cta_rect: self.welcome_upgrade_cta_rect.as_ref(), - privacy_banner_accept_rect: self.welcome_privacy_banner_accept_rect.as_ref(), - privacy_banner_customize_rect: self - .welcome_privacy_banner_customize_rect - .as_ref(), - privacy_banner_legal_rect: self.welcome_privacy_banner_legal_rect.as_ref(), + privacy_banner_opt_in_rect: self.welcome_privacy_banner_opt_in_rect.as_ref(), + privacy_banner_opt_out_rect: self.welcome_privacy_banner_opt_out_rect.as_ref(), + privacy_banner_terms_rect: self.welcome_privacy_banner_terms_rect.as_ref(), + privacy_banner_policy_rect: self.welcome_privacy_banner_policy_rect.as_ref(), on_privacy_banner: &mut self.welcome_on_privacy_banner, on_upgrade_cta: &mut self.welcome_on_upgrade_cta, upgrade_cta_keyboard: welcome_pinned_upgrade_cta, @@ -3057,9 +3064,10 @@ struct WelcomeInputCtx<'a> { /// Hit-test rect for the welcome hero upgrade CTA `[label]` button /// (click → open the promo url). upgrade_cta_rect: Option<&'a ratatui::layout::Rect>, - privacy_banner_accept_rect: Option<&'a ratatui::layout::Rect>, - privacy_banner_customize_rect: Option<&'a ratatui::layout::Rect>, - privacy_banner_legal_rect: Option<&'a ratatui::layout::Rect>, + privacy_banner_opt_in_rect: Option<&'a ratatui::layout::Rect>, + privacy_banner_opt_out_rect: Option<&'a ratatui::layout::Rect>, + privacy_banner_terms_rect: Option<&'a ratatui::layout::Rect>, + privacy_banner_policy_rect: Option<&'a ratatui::layout::Rect>, /// Sticky hover flag for the privacy banner buttons (redraw on /// enter/leave/crossing so they brighten/dim). on_privacy_banner: &'a mut bool, @@ -3698,21 +3706,28 @@ fn handle_welcome_input(ev: &Event, ctx: &mut WelcomeInputCtx<'_>) -> InputOutco xai_grok_telemetry::events::AnnouncementCtaSurface::Welcome, )); } - if let Some(rect) = ctx.privacy_banner_accept_rect + if let Some(rect) = ctx.privacy_banner_opt_in_rect && rect.contains(ratatui::layout::Position::new(mouse.column, mouse.row)) { - return InputOutcome::Action(Action::PrivacyBannerAccept); + return InputOutcome::Action(Action::PrivacyBannerOptIn); } - if let Some(rect) = ctx.privacy_banner_customize_rect + if let Some(rect) = ctx.privacy_banner_opt_out_rect && rect.contains(ratatui::layout::Position::new(mouse.column, mouse.row)) { - return InputOutcome::Action(Action::PrivacyBannerCustomize); + return InputOutcome::Action(Action::PrivacyBannerOptOut); } - if let Some(rect) = ctx.privacy_banner_legal_rect + if let Some(rect) = ctx.privacy_banner_terms_rect && rect.contains(ratatui::layout::Position::new(mouse.column, mouse.row)) { return InputOutcome::Action(Action::OpenUrl( - crate::views::privacy_banner::PRIVACY_BANNER_LEGAL_URL.to_string(), + crate::views::privacy_banner::PRIVACY_BANNER_TERMS_URL.to_string(), + )); + } + if let Some(rect) = ctx.privacy_banner_policy_rect + && rect.contains(ratatui::layout::Position::new(mouse.column, mouse.row)) + { + return InputOutcome::Action(Action::OpenUrl( + crate::views::privacy_banner::PRIVACY_BANNER_POLICY_URL.to_string(), )); } if let Some(rect) = ctx.changelog_cta_rect @@ -3794,13 +3809,16 @@ fn handle_welcome_input(ev: &Event, ctx: &mut WelcomeInputCtx<'_>) -> InputOutco return InputOutcome::Changed; } let over_banner = ctx - .privacy_banner_accept_rect + .privacy_banner_opt_in_rect .is_some_and(|r| r.contains(pos)) || ctx - .privacy_banner_customize_rect + .privacy_banner_opt_out_rect .is_some_and(|r| r.contains(pos)) || ctx - .privacy_banner_legal_rect + .privacy_banner_terms_rect + .is_some_and(|r| r.contains(pos)) + || ctx + .privacy_banner_policy_rect .is_some_and(|r| r.contains(pos)); if over_banner || *ctx.on_privacy_banner { *ctx.on_privacy_banner = over_banner; @@ -4348,10 +4366,11 @@ impl AppView { self.welcome_refresh_rect = result.refresh_rect; self.welcome_gate_url_rect = result.gate_url_rect; self.welcome_upgrade_cta_rect = result.upgrade_cta_rect; - self.welcome_privacy_banner_accept_rect = result.privacy_banner_accept_rect; - self.welcome_privacy_banner_customize_rect = - result.privacy_banner_customize_rect; - self.welcome_privacy_banner_legal_rect = result.privacy_banner_legal_rect; + self.welcome_privacy_banner_opt_in_rect = result.privacy_banner_opt_in_rect; + self.welcome_privacy_banner_opt_out_rect = + result.privacy_banner_opt_out_rect; + self.welcome_privacy_banner_terms_rect = result.privacy_banner_terms_rect; + self.welcome_privacy_banner_policy_rect = result.privacy_banner_policy_rect; self.welcome_changelog_cta_rect = result.changelog_cta_rect; if let Some((ref msg, _)) = self.welcome_toast { paint_welcome_toast(f.buffer_mut(), view_area, msg); @@ -4538,7 +4557,7 @@ impl AppView { !privacy_banner && self.tip.is_some() && agent.should_show_tip(); let has_mode_banner = agent.mode_switch_banner.is_some(); let banner_height = if privacy_banner { - 2 + crate::views::privacy_banner::MIN_HEIGHT } else if has_mode_banner { 1 } else if announcement_banner_h > 0 { @@ -5731,7 +5750,8 @@ pub(crate) mod tests { privacy_notice_rollout: false, privacy_banner_reshow_days: None, privacy_banner_acked: None, - privacy_banner_accept_inflight: false, + privacy_banner_opt_in_inflight: false, + coding_data_write_seq: 0, show_tips: None, auto_update: None, ask_user_question_timeout_enabled: None, @@ -5774,9 +5794,10 @@ pub(crate) mod tests { welcome_refresh_rect: None, welcome_gate_url_rect: None, welcome_upgrade_cta_rect: None, - welcome_privacy_banner_accept_rect: None, - welcome_privacy_banner_customize_rect: None, - welcome_privacy_banner_legal_rect: None, + welcome_privacy_banner_opt_in_rect: None, + welcome_privacy_banner_opt_out_rect: None, + welcome_privacy_banner_terms_rect: None, + welcome_privacy_banner_policy_rect: None, welcome_toast: None, welcome_on_privacy_banner: false, welcome_on_upgrade_cta: false, @@ -10020,9 +10041,10 @@ pub(crate) mod tests { fn welcome_privacy_banner_hover_triggers_redraw() { let mut app = test_app(); app.active_view = ActiveView::Welcome; - app.welcome_privacy_banner_accept_rect = Some(ratatui::layout::Rect::new(50, 10, 8, 1)); - app.welcome_privacy_banner_customize_rect = Some(ratatui::layout::Rect::new(25, 10, 24, 1)); - app.welcome_privacy_banner_legal_rect = Some(ratatui::layout::Rect::new(2, 11, 45, 1)); + app.welcome_privacy_banner_opt_in_rect = Some(ratatui::layout::Rect::new(50, 10, 8, 1)); + app.welcome_privacy_banner_opt_out_rect = Some(ratatui::layout::Rect::new(25, 10, 24, 1)); + app.welcome_privacy_banner_terms_rect = Some(ratatui::layout::Rect::new(7, 11, 5, 1)); + app.welcome_privacy_banner_policy_rect = Some(ratatui::layout::Rect::new(17, 11, 14, 1)); let over = left_mouse(MouseEventKind::Moved, 52, 10); assert!(matches!(app.handle_input(&over), InputOutcome::Changed)); assert!(app.welcome_on_privacy_banner); diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/router.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/router.rs index cfa1e56..6989457 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/router.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/router.rs @@ -96,10 +96,9 @@ use super::settings::ui::{ }; use super::status::{ dispatch_copy_session_id, dispatch_manage_billing, dispatch_open_gboom, dispatch_open_tutorial, - dispatch_privacy_banner_accept, dispatch_privacy_banner_customize, dispatch_share_session, - dispatch_show_context_info, dispatch_show_privacy_info, dispatch_show_queue, - dispatch_show_release_notes, dispatch_show_session_info, dispatch_show_tasks, - dispatch_show_usage, set_coding_data_sharing, + dispatch_privacy_banner_opt_in, dispatch_privacy_banner_opt_out, dispatch_share_session, + dispatch_show_context_info, dispatch_show_queue, dispatch_show_release_notes, + dispatch_show_session_info, dispatch_show_tasks, dispatch_show_usage, set_coding_data_sharing, }; use super::task_result::{dispatch_task_result, unregister_all_active_sessions}; use super::transcript::{ @@ -950,7 +949,6 @@ pub(crate) fn dispatch(action: Action, app: &mut AppView) -> Vec { Action::SaveRememberNoteFromModal => dispatch_save_remember_note_from_modal(app), Action::SendBtw(question) => dispatch_send_btw(app, question), Action::SendRecap { auto } => dispatch_send_recap(app, auto), - Action::ShowPrivacyInfo => dispatch_show_privacy_info(app), Action::SetCodingDataSharing { opted_in } => set_coding_data_sharing(app, opted_in), Action::ToggleYolo => dispatch_toggle_yolo(app), Action::ToggleMultiline => dispatch_toggle_multiline(app), @@ -1011,8 +1009,8 @@ pub(crate) fn dispatch(action: Action, app: &mut AppView) -> Vec { Action::PreviewAutoLightTheme(v) => preview_auto_light_theme(app, v), Action::OpenSettings => dispatch_open_settings(app, None), Action::OpenSettingsFocus { key } => dispatch_open_settings(app, Some(key)), - Action::PrivacyBannerAccept => dispatch_privacy_banner_accept(app), - Action::PrivacyBannerCustomize => dispatch_privacy_banner_customize(app), + Action::PrivacyBannerOptIn => dispatch_privacy_banner_opt_in(app), + Action::PrivacyBannerOptOut => dispatch_privacy_banner_opt_out(app), Action::OpenCommandPalette => dispatch_open_command_palette(app), Action::OpenHowtoGuides => dispatch_open_howto_guides(app), Action::OpenResetConfirm { key } => dispatch_open_reset_confirm(app, key), diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/settings/ui.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/settings/ui.rs index 79126fb..da33972 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/settings/ui.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/settings/ui.rs @@ -255,8 +255,8 @@ pub(in crate::app::dispatch) fn dispatch_open_settings( if let Some(key) = focus_key && state.focus_key(key) { - // Land directly on the setting's chooser page (e.g. the coding data - // sharing opt-in/out picker), not just the focused browse row. + // Try the chooser; a locked row keeps Browse (`try_enter_picking_enum` + // refuses when `row_lock` is set). state.try_enter_picking_enum(); } agent.active_modal = Some(ActiveModal::Settings { state }); diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/status.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/status.rs index 6c3ec9c..7ddd5c3 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/status.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/status.rs @@ -68,53 +68,48 @@ pub(super) fn dispatch_show_session_info(app: &mut AppView) -> Vec { }] } -/// Show privacy and data retention status as a system message in scrollback. -/// -/// Three-state display: Enterprise ZDR, coding data sharing opted out, -/// or opted in. Labels align with `CODING_DATA_SHARING_CHOICES` in -/// `settings/defs.rs` and the `coding_data_sharing_toast` format. -/// -/// Also lists config knobs that `/privacy` does not change (technical -/// pointers only; no policy claims). -pub(super) fn dispatch_show_privacy_info(app: &mut AppView) -> Vec { - let mut lines = Vec::new(); - - if app.is_zdr { - // Enterprise ZDR -- the team has disabled retention entirely. - lines.push(" Zero Data Retention: enabled"); - lines.push(" Your data is not retained or used for training (ZDR enabled)."); - } else if app.coding_data_retention_opt_out { - // Coding data sharing opted out -- matches desktop's "Privacy mode" state. - lines.push(" Privacy: privacy mode"); - lines.push(" Your code data will not be trained on or used to improve the product."); - lines.push(""); - lines.push(" Use /privacy opt-in to share data and help improve the product."); - } else { - // Coding data sharing opted in -- matches desktop's "Share data" state. - lines.push(" Privacy: share data"); - lines.push(" Usage and code data may be used by SpaceXAI to improve the product."); - lines.push(""); - lines.push(" Use /privacy opt-out to enable privacy mode."); - } - - // Config keys only; do not describe retention/training/analytics policy here. - lines.push(""); - lines.push(" Other settings (not changed by /privacy):"); - lines.push(" - [features] telemetry / GROK_TELEMETRY_ENABLED"); - lines.push(" - [telemetry] trace_upload / GROK_TELEMETRY_TRACE_UPLOAD"); - lines.push(" - GROK_EXTERNAL_OTEL / OTEL_*"); - lines.push(""); - lines.push(" Learn more: https://x.ai/legal"); - let text = lines.join("\n"); - push_system_to_any_agent(app, &text); - vec![] -} - /// State-only mutation for `coding_data_sharing`. SHELL-owned. pub(super) fn set_coding_data_sharing_inner(app: &mut AppView, opted_in: bool) { app.coding_data_retention_opt_out = !opted_in; } +/// Agent the coding-data ACP write is attributed to. Privacy is app-level, +/// so the id only routes the result back; `AgentId(0)` is the synthetic +/// stand-in for the welcome screen, where the banner is reachable before a +/// session exists. +fn coding_data_sharing_agent_id(app: &AppView) -> AgentId { + match app.active_view { + ActiveView::Agent(id) => id, + _ => app.agents.keys().next().copied().unwrap_or(AgentId(0)), + } +} + +/// Claim the next write generation. Every `SetCodingDataSharing` must take +/// one so its reply can be matched against the newest write. +fn next_coding_data_write_seq(app: &mut AppView) -> u64 { + app.coding_data_write_seq += 1; + app.coding_data_write_seq +} + +/// Is this reply from the newest write? Writes to this endpoint run +/// concurrently and can land out of order, so an older reply must not touch +/// state: its `rollback_to_opted_in` predates the newer write, and applying +/// it would silently undo whatever the user did since. +fn is_current_coding_data_write(app: &AppView, seq: u64, agent_id: AgentId) -> bool { + if seq == app.coding_data_write_seq { + return true; + } + tracing::debug!( + target: "settings", + key = "coding_data_sharing", + ?agent_id, + seq, + current = app.coding_data_write_seq, + "dropping superseded coding-data reply", + ); + false +} + /// Set coding-data-sharing preference. SHELL-owned, auth-metadata-backed /// (persists via ACP ext-request, NOT `~/.grok/config.toml`). pub(super) fn set_coding_data_sharing(app: &mut AppView, opted_in: bool) -> Vec { @@ -134,29 +129,18 @@ pub(super) fn set_coding_data_sharing(app: &mut AppView, opted_in: bool) -> Vec< return vec![]; } } - // Synthetic AgentId(0) when no agents (welcome banner Accept). - let agent_id = match app.active_view { - crate::app::app_view::ActiveView::Agent(id) => id, - _ => app - .agents - .keys() - .next() - .copied() - .unwrap_or(crate::app::agent::AgentId(0)), - }; - + let agent_id = coding_data_sharing_agent_id(app); let prev = !app.coding_data_retention_opt_out; - // ── Idempotent path: toast but skip the ACP round-trip. ────────── + // ── Idempotent path: skip the ACP round-trip. ──────────────────── if prev == opted_in { - app.show_toast(&coding_data_sharing_toast(opted_in)); return vec![]; } - // ── Optimistic mutation: state, then UI feedback, then effect. ─── + // Optimistic mutation. Success is silent; only the refusals above and + // the failure handler toast. set_coding_data_sharing_inner(app, opted_in); refresh_open_settings_modals(app); - app.show_toast(&coding_data_sharing_toast(opted_in)); tracing::info!( target: "settings", @@ -169,32 +153,10 @@ pub(super) fn set_coding_data_sharing(app: &mut AppView, opted_in: bool) -> Vec< agent_id, opted_in, rollback_to_opted_in: prev, + seq: next_coding_data_write_seq(app), }] } -/// Format the `Coding data sharing` toast. Asymmetric: opt-in -/// (privacy-degrading) uses ⚠ + consequence text; opt-out (safe -/// default) uses ✓. Uses display names from the registry catalog. -pub(super) fn coding_data_sharing_toast(opted_in: bool) -> String { - let display = display_for_coding_data_sharing_canonical(opted_in); - if opted_in { - // Privacy-degrading: warn glyph + spelled-out consequence. - format!( - "\u{26A0} Coding data sharing: {display} \u{2014} code samples may be retained \ - for training" - ) - } else { - // Safe default — uniform ✓ glyph. - format!("\u{2713} Coding data sharing: {display}") - } -} - -/// Display string for the canonical bool. Keep aligned with -/// `CODING_DATA_SHARING_CHOICES` in `settings/defs.rs`. -fn display_for_coding_data_sharing_canonical(opted_in: bool) -> &'static str { - if opted_in { "Opt in" } else { "Opt out" } -} - /// Scrub an untrusted error string for toast display. Substitutes a /// generic placeholder when the input exceeds 120 chars or contains /// control / bidi-override characters (prevents escape-sequence @@ -212,21 +174,6 @@ pub(super) fn scrub_error_for_toast(error: &str) -> String { } } -/// Push a system message to the active agent's scrollback, or to any available -/// agent if on the welcome screen. -fn push_system_to_any_agent(app: &mut AppView, msg: &str) { - let block = crate::scrollback::block::RenderBlock::system(msg.to_string()); - if let ActiveView::Agent(id) = app.active_view - && let Some(agent) = app.agents.get_mut(&id) - { - agent.scrollback.push_block(block); - return; - } - if let Some(agent) = app.agents.values_mut().next() { - agent.scrollback.push_block(block); - } -} - /// Show context info: fetch via x.ai/session/info and display rich breakdown. /// /// Produces Effect::ShowContextInfo which spawns an async ACP ext request. @@ -430,16 +377,16 @@ pub(super) fn handle_coding_data_sharing_updated( app: &mut AppView, agent_id: AgentId, opted_in: bool, + seq: u64, ) -> Vec { + if !is_current_coding_data_write(app, seq, agent_id) { + return vec![]; + } // Re-anchor mirror to server-confirmed value (defense-in-depth against // server reshaping the boolean). `agent_id` discarded — privacy is // app-level, not per-agent. set_coding_data_sharing_inner(app, opted_in); refresh_open_settings_modals(app); - // Re-toast on confirmation. Without this, a slow ACP round-trip would - // leave the user with only the optimistic toast (already faded) and no - // server-confirmed feedback. - app.show_toast(&coding_data_sharing_toast(opted_in)); tracing::info!( target: "settings", key = "coding_data_sharing", @@ -448,9 +395,9 @@ pub(super) fn handle_coding_data_sharing_updated( "ACP update confirmed; mirror re-anchored", ); let mut effects = vec![]; - // Ack only after successful opt-in from the privacy banner Accept path. - if app.privacy_banner_accept_inflight { - app.privacy_banner_accept_inflight = false; + // Ack only after a successful opt-in from the banner's [Opt in]. + if app.privacy_banner_opt_in_inflight { + app.privacy_banner_opt_in_inflight = false; if opted_in { effects.extend(ack_privacy_banner(app)); } @@ -463,7 +410,15 @@ pub(super) fn handle_coding_data_sharing_failed( agent_id: AgentId, error: String, rollback_to_opted_in: bool, + seq: u64, ) -> Vec { + // A superseded failure must not revert: `rollback_to_opted_in` predates + // the newer write, so applying it would undo a change the user made + // after this one was sent. It must not toast either — nothing the user + // is looking at failed. + if !is_current_coding_data_write(app, seq, agent_id) { + return vec![]; + } // Revert optimistic mutation: inner → refresh → toast. `agent_id` // discarded — privacy is global. set_coding_data_sharing_inner(app, rollback_to_opted_in); @@ -481,8 +436,8 @@ pub(super) fn handle_coding_data_sharing_failed( %error, "ACP update failed; reverted optimistic mutation", ); - // Accept failure: no ack; clear inflight so the banner stays. - app.privacy_banner_accept_inflight = false; + // Opt-in failure: no ack; clear inflight so the banner stays. + app.privacy_banner_opt_in_inflight = false; vec![] } @@ -493,31 +448,43 @@ pub(in crate::app::dispatch) fn ack_privacy_banner(app: &mut AppView) -> Vec Vec { - if app.privacy_banner_accept_inflight || !app.privacy_banner_should_show() { +/// `[Opt in]`: opt in via the settings path; ack only after ACP success, so +/// a failed round trip leaves the banner up instead of recording a change +/// that did not happen. +pub(in crate::app::dispatch) fn dispatch_privacy_banner_opt_in(app: &mut AppView) -> Vec { + if app.privacy_banner_opt_in_inflight || !app.privacy_banner_should_show() { return vec![]; } let effects = set_coding_data_sharing(app, true); // should_show guarantees opted-out + unguarded, so effects is only empty - // if a guard regresses; leaving inflight false keeps Accept clickable. - app.privacy_banner_accept_inflight = !effects.is_empty(); + // if a guard regresses; leaving inflight false keeps [Opt in] clickable. + app.privacy_banner_opt_in_inflight = !effects.is_empty(); effects } -/// Customize: ack, then open settings on coding_data_sharing -/// (creates/switches agent when opened from welcome). -pub(in crate::app::dispatch) fn dispatch_privacy_banner_customize( - app: &mut AppView, -) -> Vec { - if app.privacy_banner_accept_inflight || !app.privacy_banner_should_show() { +/// `[Opt out]`: ack locally, then record the decline. +/// +/// The ack does NOT wait on the server, unlike `[Opt in]`'s: the user asked +/// for no change, so gating dismissal on a round trip would only re-ask a +/// question they answered. +/// +/// The write is built here rather than through `set_coding_data_sharing`, +/// whose idempotent guard would skip it — the user is already opted out, +/// and recording that is the point. Its response re-anchors the mirror; +/// concurrent writes to this endpoint are still unordered. +pub(in crate::app::dispatch) fn dispatch_privacy_banner_opt_out(app: &mut AppView) -> Vec { + if app.privacy_banner_opt_in_inflight || !app.privacy_banner_should_show() { return vec![]; } let mut effects = ack_privacy_banner(app); - effects.extend(super::settings::ui::dispatch_open_settings( - app, - Some("coding_data_sharing"), - )); + effects.push(Effect::SetCodingDataSharing { + agent_id: coding_data_sharing_agent_id(app), + opted_in: false, + // Already opted out, so the revert is a no-op — and the generation + // guard drops it entirely if the user has opted in since. + rollback_to_opted_in: false, + seq: next_coding_data_write_seq(app), + }); effects } diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/task_result.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/task_result.rs index 8de391e..784c8cc 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/task_result.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/task_result.rs @@ -864,14 +864,17 @@ pub(super) fn dispatch_task_result(result: TaskResult, app: &mut AppView) -> Vec } vec![] } - TaskResult::CodingDataSharingUpdated { agent_id, opted_in } => { - handle_coding_data_sharing_updated(app, agent_id, opted_in) - } + TaskResult::CodingDataSharingUpdated { + agent_id, + opted_in, + seq, + } => handle_coding_data_sharing_updated(app, agent_id, opted_in, seq), TaskResult::CodingDataSharingFailed { agent_id, error, rollback_to_opted_in, - } => handle_coding_data_sharing_failed(app, agent_id, error, rollback_to_opted_in), + seq, + } => handle_coding_data_sharing_failed(app, agent_id, error, rollback_to_opted_in, seq), TaskResult::RenameSessionComplete { agent_id, title } => { if let Some(agent) = app.agents.get_mut(&agent_id) { let safe = crate::views::session_title::sanitize_display_text(&title); diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/tests/mod.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/tests/mod.rs index 736039b..d49ac34 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/tests/mod.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/tests/mod.rs @@ -159,7 +159,8 @@ fn test_app() -> AppView { privacy_notice_rollout: false, privacy_banner_reshow_days: None, privacy_banner_acked: None, - privacy_banner_accept_inflight: false, + privacy_banner_opt_in_inflight: false, + coding_data_write_seq: 0, show_tips: None, auto_update: None, ask_user_question_timeout_enabled: None, @@ -201,9 +202,10 @@ fn test_app() -> AppView { welcome_gate_url_rect: None, welcome_changelog_cta_rect: None, welcome_upgrade_cta_rect: None, - welcome_privacy_banner_accept_rect: None, - welcome_privacy_banner_customize_rect: None, - welcome_privacy_banner_legal_rect: None, + welcome_privacy_banner_opt_in_rect: None, + welcome_privacy_banner_opt_out_rect: None, + welcome_privacy_banner_terms_rect: None, + welcome_privacy_banner_policy_rect: None, welcome_toast: None, welcome_on_privacy_banner: false, welcome_on_upgrade_cta: false, diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/tests/settings.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/tests/settings.rs index 8089906..a53c461 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/tests/settings.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/tests/settings.rs @@ -660,9 +660,8 @@ fn dispatch_open_settings_opens_then_close_on_reentry() { ); } } -/// A focused open (privacy banner Customize) landing on an agent whose -/// settings modal is already open must reopen focused on the requested -/// row — not toggle the modal closed. +/// A focused open on an agent whose settings modal is already open must +/// reopen focused on the requested row — not toggle the modal closed. #[test] fn dispatch_open_settings_focus_reopens_when_already_open() { use crate::views::modal::ActiveModal; @@ -689,6 +688,62 @@ fn dispatch_open_settings_focus_reopens_when_already_open() { "focused re-entry must land on the requested row" ); } +/// Chooser when editable, browse row when locked. The team-admin arm is the +/// one a `team_name.is_some()` shortcut would break. +#[test] +fn dispatch_open_settings_focus_skips_the_chooser_only_when_locked() { + use crate::views::modal::ActiveModal; + use crate::views::settings_modal::SettingsModalMode; + let open_focused = |app: &mut AppView| -> SettingsModalMode { + let _ = dispatch( + Action::OpenSettingsFocus { + key: "coding_data_sharing", + }, + app, + ); + let agent = app.agents.get(&AgentId(0)).unwrap(); + let Some(ActiveModal::Settings { state }) = &agent.active_modal else { + panic!("settings modal must be open") + }; + assert_eq!( + state.focused_setting().map(|(k, _)| k), + Some("coding_data_sharing"), + "every landing focuses the row" + ); + state.mode() + }; + let mut app = test_app_with_agent(); + assert!( + matches!( + open_focused(&mut app), + SettingsModalMode::PickingEnum { .. } + ), + "an editable setting opens its chooser" + ); + let mut app = test_app_with_agent(); + app.is_zdr = true; + assert!( + matches!(open_focused(&mut app), SettingsModalMode::Browse), + "ZDR must stop at the row that says so" + ); + let mut app = test_app_with_agent(); + app.team_name = Some("acme".to_string()); + app.team_role = Some("member".to_string()); + assert!( + matches!(open_focused(&mut app), SettingsModalMode::Browse), + "a team-managed lock must stop at the row that says so" + ); + let mut app = test_app_with_agent(); + app.team_name = Some("acme".to_string()); + app.team_role = Some("admin".to_string()); + assert!( + matches!( + open_focused(&mut app), + SettingsModalMode::PickingEnum { .. } + ), + "a team admin is not locked" + ); +} /// `dispatch_open_reset_confirm` moves the Settings modal state /// into the new `ResetSettingsConfirm` variant, preserving it /// across the confirm dialog's lifecycle. The dispatch arm is diff --git a/crates/codegen/xai-grok-pager/src/app/dispatch/tests/status.rs b/crates/codegen/xai-grok-pager/src/app/dispatch/tests/status.rs index 1ca5cdb..fcd7460 100644 --- a/crates/codegen/xai-grok-pager/src/app/dispatch/tests/status.rs +++ b/crates/codegen/xai-grok-pager/src/app/dispatch/tests/status.rs @@ -60,94 +60,14 @@ fn send_while_idle_with_nonempty_shared_queue_routes_to_server() { assert_eq!(q.last().map(|e| e.text.as_str()), Some("c")); } -#[test] -fn show_privacy_info_zdr() { - let mut app = test_app_with_agent(); - app.is_zdr = true; - let effects = dispatch(Action::ShowPrivacyInfo, &mut app); - assert!(effects.is_empty()); - let text = last_system_text(&app, AgentId(0)); - assert!(text.contains("Zero Data Retention")); - assert!( - text.contains("Other settings (not changed by /privacy)"), - "must list other settings knobs: {text}", - ); - assert!( - text.contains("GROK_TELEMETRY_ENABLED") && text.contains("GROK_EXTERNAL_OTEL"), - "must list telemetry/OTEL config keys: {text}", - ); -} - -/// `/privacy` info-print uses the desktop-aligned "privacy mode" / -/// "share data" labels from the user's intentional rewrite. -#[test] -fn show_privacy_info_opted_out() { - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = true; - let effects = dispatch(Action::ShowPrivacyInfo, &mut app); - assert!(effects.is_empty()); - let text = last_system_text(&app, AgentId(0)); - assert!( - text.contains("Privacy: privacy mode"), - "info-print must use 'Privacy: privacy mode' (desktop-aligned label): {text}", - ); - assert!(text.contains("/privacy opt-in")); - assert!( - text.contains("Other settings (not changed by /privacy)") - && text.contains("GROK_TELEMETRY_ENABLED") - && text.contains("trace_upload") - && text.contains("GROK_EXTERNAL_OTEL"), - "must list config knobs not changed by /privacy: {text}", - ); -} - -#[test] -fn show_privacy_info_opted_in() { - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = false; - let effects = dispatch(Action::ShowPrivacyInfo, &mut app); - assert!(effects.is_empty()); - let text = last_system_text(&app, AgentId(0)); - assert!( - text.contains("Privacy: share data"), - "info-print must use 'Privacy: share data' (desktop-aligned label): {text}", - ); - assert!(text.contains("/privacy opt-out")); -} - -/// The info-print uses desktop-aligned labels ("privacy mode" / -/// "share data"). This test pins those labels to catch accidental -/// regressions to the registry's "Opt in" / "Opt out" display -/// strings. -#[test] -fn show_privacy_info_does_not_use_old_desktop_labels() { - // opted-out → "Privacy: privacy mode" - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = true; - let _ = dispatch(Action::ShowPrivacyInfo, &mut app); - let text = last_system_text(&app, AgentId(0)); - assert!( - text.contains("privacy mode"), - "[opted-out] info-print must contain 'privacy mode': {text:?}", - ); - - // opted-in → "Privacy: share data" - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = false; - let _ = dispatch(Action::ShowPrivacyInfo, &mut app); - let text = last_system_text(&app, AgentId(0)); - assert!( - text.contains("share data"), - "[opted-in] info-print must contain 'share data': {text:?}", - ); -} - // ── coding_data_sharing dispatch tests ─── // -// The dispatcher uses **optimistic + rollback + toast**, matching the -// `set_yolo_mode` pattern. These tests pin the contract: -// - Guards (ZDR, non-admin team) toast and short-circuit. -// - Idempotent dispatch toasts but emits no Effect. +// The dispatcher uses **optimistic + rollback**, matching the +// `set_yolo_mode` pattern minus its toasts — the surfaces that change this +// setting show the result themselves. These tests pin the contract: +// - Guards (ZDR, non-admin team) toast and short-circuit; they are the +// only paths that still speak up, because nothing else on screen would. +// - Idempotent dispatch emits no Effect and says nothing. // - Optimistic mutation flips `app.coding_data_retention_opt_out` // BEFORE the Effect is emitted. // - `Effect::SetCodingDataSharing` carries @@ -156,74 +76,26 @@ fn show_privacy_info_does_not_use_old_desktop_labels() { // mutation; `TaskResult::CodingDataSharingUpdated` re-anchors // to the server-confirmed value. -/// Idempotent re-dispatch when already opted-in toasts but emits -/// no Effect (avoids a wasted ACP round-trip). -/// -/// Toast uses the **display name** ("Opt in", not the -/// snake-case canonical "opt-in") AND the **destructive `⚠` -/// glyph** on the opt-in direction (privacy-degrading). +/// Idempotent re-dispatch skips the ACP round-trip. #[test] -fn set_coding_data_sharing_idempotent_opt_in() { - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = false; // currently opted-in - let effects = dispatch(Action::SetCodingDataSharing { opted_in: true }, &mut app); - assert!( - effects.is_empty(), - "idempotent re-dispatch must NOT emit Effect" - ); - let toast = read_toast(&app); - assert!( - toast.contains("Opt in"), - "toast must show display name 'Opt in' (PR 9 R1, General-3 Issue 6): {toast}", - ); - assert!( - !toast.contains("opt-in"), - "toast must NOT use snake-case canonical 'opt-in' — display name only: {toast}", - ); - assert!( - toast.contains('\u{26A0}'), - "idempotent opt-in toast uses ⚠ destructive-warning glyph (PR 9 R1, \ - General-3 Issue 5): {toast}", - ); - // State unchanged. - assert!( - !app.coding_data_retention_opt_out, - "idempotent path must not mutate state", - ); -} - -/// Idempotent re-dispatch when already opted-out toasts but emits -/// no Effect. -/// -/// Opt-out direction uses the **uniform `✓` glyph** -/// (restoring the safe default) and the display name "Opt out". -#[test] -fn set_coding_data_sharing_idempotent_opt_out() { - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = true; // currently opted-out - let effects = dispatch(Action::SetCodingDataSharing { opted_in: false }, &mut app); - assert!( - effects.is_empty(), - "idempotent re-dispatch must NOT emit Effect" - ); - let toast = read_toast(&app); - assert!( - toast.contains("Opt out"), - "toast must show display name 'Opt out': {toast}", - ); - assert!( - toast.contains('\u{2713}'), - "idempotent opt-out toast uses ✓ safe-default glyph: {toast}", - ); - assert!( - !toast.contains('\u{26A0}'), - "opt-out is the safe direction — must NOT use ⚠: {toast}", - ); - // State unchanged. - assert!( - app.coding_data_retention_opt_out, - "idempotent path must not mutate state", - ); +fn set_coding_data_sharing_idempotent_is_silent_and_effect_free() { + for opted_in in [true, false] { + let mut app = test_app_with_agent(); + app.coding_data_retention_opt_out = !opted_in; // already at the target + let effects = dispatch(Action::SetCodingDataSharing { opted_in }, &mut app); + assert!( + effects.is_empty(), + "idempotent re-dispatch must NOT emit Effect (opted_in={opted_in})" + ); + assert!( + app.agents[&AgentId(0)].toast.is_none(), + "idempotent re-dispatch must not toast (opted_in={opted_in})" + ); + assert_eq!( + app.coding_data_retention_opt_out, !opted_in, + "idempotent path must not mutate state (opted_in={opted_in})", + ); + } } /// ZDR teams are blocked from toggling. The blocked path @@ -314,7 +186,7 @@ fn set_coding_data_sharing_allowed_for_admin() { } /// Non-idempotent dispatch emits one Effect AND mutates state -/// optimistically AND toasts. +/// optimistically. #[test] fn set_coding_data_sharing_produces_effect_and_optimistic_mutation() { let mut app = test_app_with_agent(); @@ -326,6 +198,7 @@ fn set_coding_data_sharing_produces_effect_and_optimistic_mutation() { agent_id, opted_in, rollback_to_opted_in, + seq, } => { assert_eq!(*agent_id, AgentId(0)); assert!(!*opted_in); @@ -333,6 +206,10 @@ fn set_coding_data_sharing_produces_effect_and_optimistic_mutation() { *rollback_to_opted_in, "rollback_to_opted_in must be pre-toggle value (true == opted-in)", ); + assert_eq!( + *seq, app.coding_data_write_seq, + "the effect must carry the generation it was dispatched under", + ); } other => panic!("expected SetCodingDataSharing Effect, got {other:?}"), } @@ -341,38 +218,36 @@ fn set_coding_data_sharing_produces_effect_and_optimistic_mutation() { app.coding_data_retention_opt_out, "dispatch must optimistically mutate state", ); - // Toast on every dispatch (SHELL setter contract). - assert!(app.agents[&AgentId(0)].toast.is_some()); + assert!( + app.agents[&AgentId(0)].toast.is_none(), + "changing this setting must not toast — the settings row is the feedback", + ); } /// `TaskResult::CodingDataSharingUpdated` re-anchors state to the -/// server-confirmed value (defense-in-depth) and re-toasts. +/// server-confirmed value (defense-in-depth). #[test] -fn coding_data_sharing_updated_re_anchors_state_and_re_toasts() { +fn coding_data_sharing_updated_re_anchors_state() { let mut app = test_app_with_agent(); // Simulate post-optimistic state: opted-out. app.coding_data_retention_opt_out = true; let id = AgentId(0); // Server confirms opt-out (same as optimistic). + let seq = app.coding_data_write_seq; let effects = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingUpdated { agent_id: id, opted_in: false, + seq, }), &mut app, ); assert!(effects.is_empty(), "TaskResult arm must NOT emit Effect"); // State re-anchored (was already true, stays true). assert!(app.coding_data_retention_opt_out); - // Re-toast on confirmation uses display name + ✓. - let toast = read_toast(&app); assert!( - toast.contains("Opt out"), - "confirmation toast must use display name 'Opt out': {toast}", - ); - assert!( - toast.contains('\u{2713}'), - "opt-out confirmation toast uses ✓: {toast}", + app.agents[&AgentId(0)].toast.is_none(), + "server confirmation must not toast", ); } @@ -386,10 +261,12 @@ fn coding_data_sharing_updated_corrects_state_if_server_disagrees() { // overrides to "opt-in" (e.g. policy that prevents opt-out). app.coding_data_retention_opt_out = true; let id = AgentId(0); + let seq = app.coding_data_write_seq; let effects = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingUpdated { agent_id: id, opted_in: true, // server says opted-in + seq, }), &mut app, ); @@ -399,19 +276,6 @@ fn coding_data_sharing_updated_corrects_state_if_server_disagrees() { !app.coding_data_retention_opt_out, "server-confirmed opt-in must overwrite optimistic opt-out", ); - // Server-correction toast uses the destructive ⚠ - // pattern for the opt-in direction (the privacy-degrading - // override deserves the warning glyph even if the SERVER, not - // the user, made the call). - let toast = read_toast(&app); - assert!( - toast.contains("Opt in"), - "post-correction toast uses display name 'Opt in': {toast}", - ); - assert!( - toast.contains('\u{26A0}'), - "opt-in direction always uses ⚠ glyph, even on server-correction path: {toast}", - ); } /// `TaskResult::CodingDataSharingFailed` REVERTS the optimistic @@ -428,11 +292,13 @@ fn coding_data_sharing_failed_rolls_back_and_toasts_error() { // was opt-in (true), so `rollback_to_opted_in = true`. app.coding_data_retention_opt_out = true; let id = AgentId(0); + let seq = app.coding_data_write_seq; let effects = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: id, error: "server error".into(), rollback_to_opted_in: true, + seq, }), &mut app, ); @@ -462,11 +328,13 @@ fn coding_data_sharing_failed_rolls_back_to_opt_out() { // failed, pre-toggle was opt-out). app.coding_data_retention_opt_out = false; let id = AgentId(0); + let seq = app.coding_data_write_seq; let effects = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: id, error: "network timeout".into(), rollback_to_opted_in: false, + seq, }), &mut app, ); @@ -522,11 +390,13 @@ fn coding_data_sharing_failed_refreshes_open_modal_snapshot() { // Optimistic flip. let _ = dispatch(Action::SetCodingDataSharing { opted_in: false }, &mut app); // ACP failure. + let seq = app.coding_data_write_seq; let _ = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: AgentId(0), error: "x".into(), rollback_to_opted_in: true, + seq, }), &mut app, ); @@ -540,103 +410,18 @@ fn coding_data_sharing_failed_refreshes_open_modal_snapshot() { ); } -// ── coding_data_sharing toast tests ───────────── - -/// The opt-in transition -/// uses the **`⚠` destructive-warning glyph** + spelled-out -/// consequence text — mirroring `yolo_toast`'s -/// "Always-approve ON: all tool actions auto-run" pattern. The -/// consequence text is verbatim-pinned because the toast is the -/// only post-commit feedback for a privacy-degrading transition; -/// a future PR that softens the wording silently degrades the -/// safety affordance. #[test] -fn set_coding_data_sharing_opt_in_renders_destructive_warning_toast() { - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = true; // currently opted-out - let effects = dispatch(Action::SetCodingDataSharing { opted_in: true }, &mut app); - assert_eq!(effects.len(), 1, "non-idempotent opt-in must emit Effect"); - let toast = read_toast(&app); - assert!( - toast.contains('\u{26A0}'), - "opt-in toast MUST use ⚠ glyph (PR 9 R1, General-3 Issue 5 — \ - privacy-degrading transition deserves destructive-warning glyph): {toast}", - ); - assert!( - !toast.contains('\u{2713}'), - "opt-in toast MUST NOT use the uniform ✓ glyph — that's the \ - safe-default toast for opt-out: {toast}", - ); - assert!( - toast.contains("Opt in"), - "destructive toast still uses display name 'Opt in': {toast}", - ); - // Consequence text pinned: a future PR softening this loses - // the safety affordance. - assert!( - toast.contains("code samples"), - "destructive toast must spell out the consequence \ - (mention 'code samples'): {toast}", - ); - assert!( - toast.contains("training"), - "destructive toast must spell out the consequence \ - (mention 'training'): {toast}", - ); -} - -/// The opt-out transition uses the -/// uniform `✓` glyph (safe default), NOT the destructive `⚠`. -/// Mirrors `yolo_toast(false)` precedent — restoring the safe -/// default doesn't warrant the heavier visual. -#[test] -fn set_coding_data_sharing_opt_out_renders_safe_default_toast() { - let mut app = test_app_with_agent(); - app.coding_data_retention_opt_out = false; // currently opted-in - let _ = dispatch(Action::SetCodingDataSharing { opted_in: false }, &mut app); - let toast = read_toast(&app); - assert!( - toast.contains('\u{2713}'), - "opt-out toast uses ✓ safe-default glyph: {toast}", - ); - assert!( - !toast.contains('\u{26A0}'), - "opt-out toast MUST NOT use ⚠ — that's reserved for the privacy-degrading \ - direction (PR 9 R1): {toast}", - ); - assert!(toast.contains("Opt out")); -} - -/// The toast renders -/// the registered `EnumChoice.display` ("Opt in" / "Opt out"), -/// NOT the persisted canonical ("opt-in" / "opt-out"). Mirrors -/// the `set_theme_toast_format_uses_display_name` contract. -/// The display strings here are pinned by the -/// `coding_data_sharing_choices_use_canonical_strings` e2e test -/// (registry side) AND -/// `pr9_coding_data_sharing_choices_use_canonical_strings` (which -/// also pins the display labels via the same EnumChoice -/// entries). -#[test] -fn coding_data_sharing_toast_format_uses_display_name() { - let mut app = test_app_with_agent(); - // Opt-in direction. - app.coding_data_retention_opt_out = true; - let _ = dispatch(Action::SetCodingDataSharing { opted_in: true }, &mut app); - let opt_in_toast = read_toast(&app); - assert!( - opt_in_toast.contains("Opt in"), - "opt-in toast uses display 'Opt in', not canonical 'opt-in': {opt_in_toast}", - ); - // Clear and test opt-out direction. - app.agents.get_mut(&AgentId(0)).unwrap().toast = None; - app.coding_data_retention_opt_out = false; - let _ = dispatch(Action::SetCodingDataSharing { opted_in: false }, &mut app); - let opt_out_toast = read_toast(&app); - assert!( - opt_out_toast.contains("Opt out"), - "opt-out toast uses display 'Opt out', not canonical 'opt-out': {opt_out_toast}", - ); +fn set_coding_data_sharing_is_silent_in_both_directions() { + for opted_in in [true, false] { + let mut app = test_app_with_agent(); + app.coding_data_retention_opt_out = opted_in; // a real change either way + let _ = dispatch(Action::SetCodingDataSharing { opted_in }, &mut app); + assert!( + app.agents[&AgentId(0)].toast.is_none(), + "opted_in={opted_in} must not toast, got {:?}", + app.agents[&AgentId(0)].toast, + ); + } } /// The failure toast @@ -650,11 +435,13 @@ fn coding_data_sharing_failed_scrubs_long_error_messages() { let id = AgentId(0); // ~500-char error simulating a stack trace / HTML 502 page. let huge_error = "a".repeat(500); + let seq = app.coding_data_write_seq; let _ = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: id, error: huge_error.clone(), rollback_to_opted_in: false, + seq, }), &mut app, ); @@ -680,11 +467,13 @@ fn coding_data_sharing_failed_scrubs_control_chars_in_error() { let id = AgentId(0); // Short message with embedded newlines. let multiline = "line1\nline2\nline3".to_string(); + let seq = app.coding_data_write_seq; let _ = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: id, error: multiline.clone(), rollback_to_opted_in: false, + seq, }), &mut app, ); @@ -709,11 +498,13 @@ fn coding_data_sharing_failed_preserves_short_clean_error_message() { app.coding_data_retention_opt_out = true; let id = AgentId(0); let short_clean = "network timeout".to_string(); + let seq = app.coding_data_write_seq; let _ = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: id, error: short_clean.clone(), rollback_to_opted_in: false, + seq, }), &mut app, ); @@ -804,7 +595,7 @@ fn privacy_banner_ready_app() -> AppView { app.privacy_notice_rollout = true; app.privacy_banner_acked = None; app.privacy_banner_reshow_days = None; - app.privacy_banner_accept_inflight = false; + app.privacy_banner_opt_in_inflight = false; app.is_zdr = false; app.team_name = None; app.coding_data_retention_opt_out = true; @@ -841,28 +632,30 @@ fn privacy_banner_should_show_respects_gates() { assert!(!app.privacy_banner_should_show(), "rollout off"); } -/// Accept success: ACP confirmation acks the banner. +/// `[Opt in]` success: ACP confirmation acks the banner. #[test] -fn privacy_banner_accept_success_acks() { +fn privacy_banner_opt_in_success_acks() { let mut app = privacy_banner_ready_app(); - let effects = dispatch(Action::PrivacyBannerAccept, &mut app); + let effects = dispatch(Action::PrivacyBannerOptIn, &mut app); assert_eq!(effects.len(), 1); assert!(matches!( &effects[0], Effect::SetCodingDataSharing { opted_in: true, .. } )); - assert!(app.privacy_banner_accept_inflight); + assert!(app.privacy_banner_opt_in_inflight); assert!(!app.coding_data_retention_opt_out); assert!(app.privacy_banner_acked.is_none()); + let seq = app.coding_data_write_seq; let ack_effects = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingUpdated { agent_id: AgentId(0), opted_in: true, + seq, }), &mut app, ); - assert!(!app.privacy_banner_accept_inflight); + assert!(!app.privacy_banner_opt_in_inflight); assert!(app.privacy_banner_acked.is_some()); assert!( ack_effects @@ -872,24 +665,26 @@ fn privacy_banner_accept_success_acks() { ); } -/// Accept failure: no ack; welcome toast carries the error. +/// `[Opt in]` failure: no ack; welcome toast carries the error. #[test] -fn privacy_banner_accept_failure_no_ack_sets_welcome_toast() { +fn privacy_banner_opt_in_failure_no_ack_sets_welcome_toast() { let mut app = privacy_banner_ready_app(); - let effects = dispatch(Action::PrivacyBannerAccept, &mut app); + let effects = dispatch(Action::PrivacyBannerOptIn, &mut app); assert_eq!(effects.len(), 1); - assert!(app.privacy_banner_accept_inflight); + assert!(app.privacy_banner_opt_in_inflight); + let seq = app.coding_data_write_seq; let fail_effects = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: AgentId(0), error: "server error".into(), rollback_to_opted_in: false, + seq, }), &mut app, ); assert!(fail_effects.is_empty()); - assert!(!app.privacy_banner_accept_inflight); + assert!(!app.privacy_banner_opt_in_inflight); assert!(app.privacy_banner_acked.is_none()); assert!( app.coding_data_retention_opt_out, @@ -902,38 +697,160 @@ fn privacy_banner_accept_failure_no_ack_sets_welcome_toast() { .unwrap_or(""); assert!( toast.contains("coding data sharing"), - "welcome toast on Accept failure: {toast}" + "welcome toast on [Opt in] failure: {toast}" ); assert!(toast.contains("server error"), "error in toast: {toast}"); } -/// Customize while an Accept ACP call is inflight must be a no-op: an -/// eager ack would survive the Accept-failure rollback and hide the +/// `[Opt out]` while an `[Opt in]` ACP call is inflight must be a no-op: +/// an eager ack would survive the opt-in-failure rollback and hide the /// banner forever. #[test] -fn privacy_banner_customize_noop_while_accept_inflight() { +fn privacy_banner_opt_out_noop_while_opt_in_inflight() { let mut app = privacy_banner_ready_app(); - let _ = dispatch(Action::PrivacyBannerAccept, &mut app); - assert!(app.privacy_banner_accept_inflight); + let _ = dispatch(Action::PrivacyBannerOptIn, &mut app); + assert!(app.privacy_banner_opt_in_inflight); - let effects = dispatch(Action::PrivacyBannerCustomize, &mut app); + let effects = dispatch(Action::PrivacyBannerOptOut, &mut app); assert!( effects.is_empty(), - "customize during inflight accept must be a no-op: {effects:?}" + "[Opt out] during an inflight [Opt in] must be a no-op: {effects:?}" ); assert!(app.privacy_banner_acked.is_none(), "no ack while inflight"); + let seq = app.coding_data_write_seq; let _ = dispatch( Action::TaskComplete(TaskResult::CodingDataSharingFailed { agent_id: AgentId(0), error: "server error".into(), rollback_to_opted_in: false, + seq, }), &mut app, ); assert!( app.privacy_banner_should_show(), - "failed Accept must keep the banner even after a raced Customize" + "a failed [Opt in] must keep the banner even after a raced [Opt out]" + ); +} + +/// The ack must not hinge on the round trip, unlike `[Opt in]`'s. +#[test] +fn privacy_banner_opt_out_acks_now_and_records_decline() { + use crate::views::modal::ActiveModal; + let mut app = privacy_banner_ready_app(); + + let effects = dispatch(Action::PrivacyBannerOptOut, &mut app); + + assert!( + app.privacy_banner_acked.is_some(), + "the ack lands on click, not on an ACP reply" + ); + assert!( + !app.privacy_banner_should_show(), + "the banner is gone the moment it is dismissed" + ); + assert!( + effects + .iter() + .any(|e| matches!(e, Effect::PersistPrivacyBannerAcked { .. })), + "ack must persist: {effects:?}" + ); + assert!( + effects.iter().any(|e| matches!( + e, + Effect::SetCodingDataSharing { + opted_in: false, + rollback_to_opted_in: false, + .. + } + )), + "the decline rides the ordinary write, so its response re-anchors \ + the mirror like every other one: {effects:?}" + ); + assert!( + !app.privacy_banner_opt_in_inflight, + "a best-effort write must not arm the opt-in inflight guard, which \ + would block [Opt in] and confuse both ACP result handlers" + ); + assert!( + app.coding_data_retention_opt_out, + "declining leaves the user opted out" + ); + assert!( + app.agents + .values() + .all(|a| !matches!(a.active_modal, Some(ActiveModal::Settings { .. }))), + "[Opt out] answers the question; it must not detour into settings" + ); +} + +/// A superseded reply must not touch state. `[Opt out]` fires a write, the +/// user opts in from settings before it lands, and only then does the stale +/// decline answer: its `rollback_to_opted_in: false` was captured before the +/// opt-in existed, so applying it would flip the pager to opted-out while +/// the server holds opted-in — claiming data isn't retained when it is. +#[test] +fn superseded_coding_data_reply_cannot_clobber_a_newer_write() { + for stale_failed in [true, false] { + let mut app = privacy_banner_ready_app(); + + // Write 1: the banner decline. + let _ = dispatch(Action::PrivacyBannerOptOut, &mut app); + assert_eq!(app.coding_data_write_seq, 1); + + // Write 2: the user opts in from settings, and it confirms. + let _ = dispatch(Action::SetCodingDataSharing { opted_in: true }, &mut app); + assert_eq!(app.coding_data_write_seq, 2); + let _ = dispatch( + Action::TaskComplete(TaskResult::CodingDataSharingUpdated { + agent_id: AgentId(0), + opted_in: true, + seq: 2, + }), + &mut app, + ); + assert!(!app.coding_data_retention_opt_out, "opted in"); + + // Write 1 finally answers, either way it can. + let stale_reply = if stale_failed { + TaskResult::CodingDataSharingFailed { + agent_id: AgentId(0), + error: "network timeout".into(), + rollback_to_opted_in: false, + seq: 1, + } + } else { + TaskResult::CodingDataSharingUpdated { + agent_id: AgentId(0), + opted_in: false, + seq: 1, + } + }; + let effects = dispatch(Action::TaskComplete(stale_reply), &mut app); + + assert!(effects.is_empty(), "stale reply must emit nothing"); + assert!( + !app.coding_data_retention_opt_out, + "stale reply must not undo the newer opt-in (failed={stale_failed})" + ); + assert!( + app.agents[&AgentId(0)].toast.is_none(), + "stale reply must not toast — nothing the user is looking at failed" + ); + } +} + +/// A double-click (or a stale frame's hit rect) must not send a second +/// decline. +#[test] +fn privacy_banner_opt_out_is_idempotent() { + let mut app = privacy_banner_ready_app(); + let _ = dispatch(Action::PrivacyBannerOptOut, &mut app); + let again = dispatch(Action::PrivacyBannerOptOut, &mut app); + assert!( + again.is_empty(), + "second dismissal must be inert: {again:?}" ); } diff --git a/crates/codegen/xai-grok-pager/src/app/display_refresh_startup.rs b/crates/codegen/xai-grok-pager/src/app/display_refresh_startup.rs index 9e4de14..929fff5 100644 --- a/crates/codegen/xai-grok-pager/src/app/display_refresh_startup.rs +++ b/crates/codegen/xai-grok-pager/src/app/display_refresh_startup.rs @@ -125,6 +125,7 @@ fn spawn_terminal_and_display_refresh_telemetry(tel: StartupTel) { terminal.xtversion = %t.xtversion, terminal.term_version = %t.term_version, terminal.term_version_source = %t.term_version_source, + terminal.kitty_event_types_withheld = t.kitty_event_types_withheld, ) .entered(); tracing::info!("terminal environment detected"); diff --git a/crates/codegen/xai-grok-pager/src/app/effects/mod.rs b/crates/codegen/xai-grok-pager/src/app/effects/mod.rs index 5770147..631b25b 100644 --- a/crates/codegen/xai-grok-pager/src/app/effects/mod.rs +++ b/crates/codegen/xai-grok-pager/src/app/effects/mod.rs @@ -3305,7 +3305,12 @@ pub(crate) fn execute( } }); } - Effect::SetCodingDataSharing { agent_id, opted_in, rollback_to_opted_in } => { + Effect::SetCodingDataSharing { + agent_id, + opted_in, + rollback_to_opted_in, + seq, + } => { let tx = acp_tx.clone(); tasks .spawn(async move { @@ -3328,6 +3333,7 @@ pub(crate) fn execute( agent_id, error: format!("malformed response: {e}"), rollback_to_opted_in, + seq, }; } }; @@ -3343,6 +3349,7 @@ pub(crate) fn execute( agent_id, error: msg, rollback_to_opted_in, + seq, }; } let confirmed_opted_in = wrapper @@ -3353,6 +3360,7 @@ pub(crate) fn execute( TaskResult::CodingDataSharingUpdated { agent_id, opted_in: confirmed_opted_in, + seq, } } Err(e) => { @@ -3360,6 +3368,7 @@ pub(crate) fn execute( agent_id, error: format!("{e}"), rollback_to_opted_in, + seq, } } } diff --git a/crates/codegen/xai-grok-pager/src/app/event_loop.rs b/crates/codegen/xai-grok-pager/src/app/event_loop.rs index 4b5c7c3..0f44df3 100644 --- a/crates/codegen/xai-grok-pager/src/app/event_loop.rs +++ b/crates/codegen/xai-grok-pager/src/app/event_loop.rs @@ -1426,6 +1426,8 @@ pub(crate) async fn run( // Fire-and-forget XTVERSION query; must sit immediately before the input // reader thread is spawned so no earlier stdin consumer eats the reply. + // DA2 shares that constraint but runs earlier, in `init_terminal`, so its + // version is already resolved when the startup telemetry above is emitted. crate::terminal::xtversion::probe_at_startup(); // Read terminal events on a dedicated thread and forward them over an mpsc @@ -3165,13 +3167,13 @@ async fn drain_and_process( } // Voice capture chord (Ctrl+Space or F8), handled here before normal // routing so the release reaches us and the key never lands as text. - // Hold-to-talk under Kitty (press records, release stops), else tap - // toggle. A release is only ours when a hold session owns it, so a bare - // Space release (Ctrl lifted first) stops hold-to-talk without eating - // every Space release during normal typing. `[ui].voice_keybind_enabled` - // (read live, like `voice_capture_mode`) silences chord presses without - // touching `/voice` — see `voice_chord_claims_event` for the exact - // press/release/hold gating. + // Hold-to-talk where releases are reported (press records, release + // stops), else tap toggle. A release is only ours when a hold session + // owns it, so a bare Space release (Ctrl lifted first) stops + // hold-to-talk without eating every Space release during normal typing. + // `[ui].voice_keybind_enabled` (read live, like `voice_capture_mode`) + // silences chord presses without touching `/voice` — see + // `voice_chord_claims_event` for the exact press/release/hold gating. if let Event::Key(ke) = ev && app.voice_mode_enabled && xai_grok_voice::AUDIO_SUPPORTED @@ -3189,7 +3191,7 @@ async fn drain_and_process( ) == "hold"; let action = voice_chord_action( hold_mode, - crate::app::kitty_flags_pushed(), + crate::app::kitty_releases_reported(), ke.kind, app.voice_listening(), app.voice_hold_owned(), @@ -3405,19 +3407,18 @@ fn is_pasteable_key_event(ev: &Event) -> bool { /// Map a voice-chord key event to its action (pure, so it's unit-testable). /// -/// Hold mode on Kitty is press-to-record / release-to-stop, but only a -/// hold-*owned* session stops on release; a `/voice`/toggle session (not -/// hold-owned) has no release of its own, so a press toggles it off. Elsewhere -/// it's a tap toggle. +/// Hold mode is press-to-record / release-to-stop, but only a hold-*owned* +/// session stops on release; a `/voice`/toggle session (not hold-owned) has no +/// release of its own, so a press toggles it off. Elsewhere it's a tap toggle. fn voice_chord_action( hold_mode: bool, - kitty: bool, + releases_reported: bool, kind: KeyEventKind, listening: bool, hold_owned: bool, ) -> Option { use crate::app::actions::Action; - if hold_mode && kitty { + if hold_mode && releases_reported { match kind { KeyEventKind::Press if !listening => Some(Action::EnableVoiceMode), KeyEventKind::Press if !hold_owned => Some(Action::VoiceToggle), @@ -3749,8 +3750,8 @@ mod tests { #[test] fn voice_chord_action_cases() { use crate::app::actions::Action; - // (hold_mode, kitty, kind, listening, hold_owned) -> action tag, with the - // toggle-stop case being a past regression. + // (hold_mode, releases_reported, kind, listening, hold_owned) -> action + // tag, with the toggle-stop case being a past regression. let press = KeyEventKind::Press; let release = KeyEventKind::Release; let tag = |a: Option| match a { @@ -3761,22 +3762,24 @@ mod tests { _ => "other", }; let cases = [ - // hold+Kitty: press idle starts; release stops; press on a hold-owned - // session waits; press on a non-hold (/voice/toggle) session toggles off. + // hold + releases: press idle starts; release stops; press on a + // hold-owned session waits; press on a non-hold (/voice/toggle) + // session toggles off. ((true, true, press, false, false), "start"), ((true, true, release, true, true), "stop"), ((true, true, press, true, true), "none"), ((true, true, press, true, false), "toggle"), - // Non-hold (toggle mode or no Kitty releases): press toggles, release noops. + // Non-hold (toggle mode or no reported releases): press toggles, + // release noops. ((false, false, press, false, false), "toggle"), ((false, false, release, true, false), "none"), ((true, false, release, true, false), "none"), ]; - for ((hold, kitty, kind, listening, owned), want) in cases { + for ((hold, releases, kind, listening, owned), want) in cases { assert_eq!( - tag(voice_chord_action(hold, kitty, kind, listening, owned)), + tag(voice_chord_action(hold, releases, kind, listening, owned)), want, - "voice_chord_action({hold},{kitty},{kind:?},{listening},{owned})" + "voice_chord_action({hold},{releases},{kind:?},{listening},{owned})" ); } } diff --git a/crates/codegen/xai-grok-pager/src/app/mod.rs b/crates/codegen/xai-grok-pager/src/app/mod.rs index 9a5b75a..b471af2 100644 --- a/crates/codegen/xai-grok-pager/src/app/mod.rs +++ b/crates/codegen/xai-grok-pager/src/app/mod.rs @@ -47,7 +47,7 @@ pub(crate) mod screen_mode_relaunch; pub mod signal_handler; mod turn_completion; mod xt_filter; -pub(crate) use crate::terminal::kitty_flags_pushed; +pub(crate) use crate::terminal::{kitty_flags_pushed, kitty_releases_reported}; pub use cli::{ AgentArgs, AgentCmd, Command, HeadlessArgs, LeaderArgs, LeaderMgmtArgs, LeaderMgmtCommand, LeaderTargetArgs, OutputFormat, PagerArgs, ServeArgs, WrapArgs, @@ -70,8 +70,8 @@ use std::sync::atomic::{AtomicBool, Ordering}; use tokio_util::sync::CancellationToken; use xai_grok_shell::util::config; /// Tracks the extra Kitty keyboard layer pushed while the `/gboom` game is -/// open (see [`push_gboom_keyboard_flags`]). Kept separate from -/// `KITTY_FLAGS_PUSHED` so teardown pops both, in LIFO order. +/// open (see [`push_gboom_keyboard_flags`]). Kept separate from the base layer +/// (`terminal::kitty_keyboard`) so teardown pops both, in LIFO order. static GBOOM_KEYBOARD_PUSHED: AtomicBool = AtomicBool::new(false); /// While the `/gboom` game owns input, additionally request /// `REPORT_ALL_KEYS_AS_ESCAPE_CODES` so plain letter keys (WASD) emit @@ -1328,28 +1328,31 @@ fn init_terminal( Ok(true) => None, _ => Some("unsupported"), }); - let use_keyboard_enhancement = skip_reason.is_none(); - if use_keyboard_enhancement { - let flags = event::KeyboardEnhancementFlags::DISAMBIGUATE_ESCAPE_CODES - | event::KeyboardEnhancementFlags::REPORT_EVENT_TYPES; + crate::terminal::da2::probe_at_startup(); + let flags = crate::terminal::negotiated_kitty_flags( + skip_reason, + crate::terminal::da2::detected_packed(), + ); + if flags.is_empty() { + tracing::info!( + kitty.flags = "none", + kitty.skipped_reason = skip_reason.unwrap_or("unknown"), + "kitty keyboard protocol skipped" + ); + } else { xai_grok_shell::util::with_locked_stderr(|stderr| { let _ = execute!(stderr, event::PushKeyboardEnhancementFlags(flags)); }); tracing::info!( kitty.flags = ?flags, kitty.disambiguate = true, - kitty.report_event_types = true, + kitty.report_event_types = + flags.contains(event::KeyboardEnhancementFlags::REPORT_EVENT_TYPES), kitty.report_all_keys = false, "kitty keyboard protocol pushed" ); - } else { - tracing::info!( - kitty.flags = "none", - kitty.skipped_reason = skip_reason.unwrap_or("unknown"), - "kitty keyboard protocol skipped" - ); } - crate::terminal::set_kitty_flags_pushed(use_keyboard_enhancement); + crate::terminal::set_pushed_kitty_flags(flags); if mode.is_fullscreen() { let backend = CrosstermBackend::new( crate::render::draw::TermWriter::new(frame_tx, writer_sync) diff --git a/crates/codegen/xai-grok-pager/src/app/mouse.rs b/crates/codegen/xai-grok-pager/src/app/mouse.rs index 6f3bb73..32d12a1 100644 --- a/crates/codegen/xai-grok-pager/src/app/mouse.rs +++ b/crates/codegen/xai-grok-pager/src/app/mouse.rs @@ -138,28 +138,38 @@ impl AgentView { } if self .privacy_banner - .hit_accept + .hit_opt_in .contains(mouse.column, mouse.row) && !self.pos_occluded(mouse.column, mouse.row) { - return InputOutcome::Action(Action::PrivacyBannerAccept); + return InputOutcome::Action(Action::PrivacyBannerOptIn); } if self .privacy_banner - .hit_customize + .hit_opt_out .contains(mouse.column, mouse.row) && !self.pos_occluded(mouse.column, mouse.row) { - return InputOutcome::Action(Action::PrivacyBannerCustomize); + return InputOutcome::Action(Action::PrivacyBannerOptOut); } if self .privacy_banner - .hit_legal + .hit_terms .contains(mouse.column, mouse.row) && !self.pos_occluded(mouse.column, mouse.row) { return InputOutcome::Action(Action::OpenUrl( - crate::views::privacy_banner::PRIVACY_BANNER_LEGAL_URL.to_string(), + crate::views::privacy_banner::PRIVACY_BANNER_TERMS_URL.to_string(), + )); + } + if self + .privacy_banner + .hit_policy + .contains(mouse.column, mouse.row) + && !self.pos_occluded(mouse.column, mouse.row) + { + return InputOutcome::Action(Action::OpenUrl( + crate::views::privacy_banner::PRIVACY_BANNER_POLICY_URL.to_string(), )); } if self.hit_watching_cue.contains(mouse.column, mouse.row) @@ -1099,15 +1109,19 @@ impl AgentView { .update_hover(mouse.column, mouse.row); changed |= self .privacy_banner - .hit_accept + .hit_opt_in .update_hover(mouse.column, mouse.row); changed |= self .privacy_banner - .hit_customize + .hit_opt_out .update_hover(mouse.column, mouse.row); changed |= self .privacy_banner - .hit_legal + .hit_terms + .update_hover(mouse.column, mouse.row); + changed |= self + .privacy_banner + .hit_policy .update_hover(mouse.column, mouse.row); changed |= self .plugin_cta diff --git a/crates/codegen/xai-grok-pager/src/mcp_cmd.rs b/crates/codegen/xai-grok-pager/src/mcp_cmd.rs index a25fef4..13683bf 100644 --- a/crates/codegen/xai-grok-pager/src/mcp_cmd.rs +++ b/crates/codegen/xai-grok-pager/src/mcp_cmd.rs @@ -80,6 +80,16 @@ pub enum McpCommand { #[arg(short = 's', long, value_enum)] scope: Option, }, + /// Enable an MCP server + Enable { + /// Server name + name: String, + }, + /// Disable an MCP server + Disable { + /// Server name + name: String, + }, /// Diagnose MCP server configuration and connectivity Doctor { /// Emit machine-readable JSON output @@ -142,6 +152,8 @@ pub async fn run(mcp_args: McpArgs) -> Result<()> { McpCommand::List { json } => run_list(json), McpCommand::Add(args) => run_add(args).await, McpCommand::Remove { name, scope } => run_remove(&name, scope).await, + McpCommand::Enable { name } => run_set_enabled(&name, true).await, + McpCommand::Disable { name } => run_set_enabled(&name, false).await, McpCommand::Doctor { json, name } => run_doctor(json, name).await, } } @@ -151,6 +163,7 @@ fn run_list(json: bool) -> Result<()> { // a session started in this directory would load from config.toml files. let cwd = current_dir_or_exit(); let servers = xai_grok_shell::util::config::load_mcp_server_configs_with_project(&cwd); + let disabled = xai_grok_shell::util::config::disabled_mcp_server_names(&cwd); if json { let payload: serde_json::Value = servers @@ -160,6 +173,10 @@ fn run_list(json: bool) -> Result<()> { if let Some(obj) = entry.as_object_mut() { obj.insert("name".into(), serde_json::Value::String(name.clone())); obj.insert("scope".into(), serde_json::Value::String(scope.to_string())); + obj.insert( + "enabled".into(), + serde_json::Value::Bool(!disabled.contains(name)), + ); } entry }) @@ -179,7 +196,11 @@ fn run_list(json: bool) -> Result<()> { } McpServerTransportConfig::StreamableHttp { url, .. } => url.clone(), }; - let status = if config.enabled { "" } else { " (disabled)" }; + let status = if disabled.contains(name) { + " (disabled)" + } else { + "" + }; let scope_note = if *scope == "project" { " (project)" } else { @@ -524,6 +545,86 @@ fn surviving_definition( }) } +/// TOML / disabled list / compat JSON / legacy `grok_com_*` (not gateway). +fn mcp_server_is_known(name: &str, cwd: &Path) -> bool { + if name.starts_with("grok_com_") { + return true; + } + xai_grok_shell::util::config::cli_known_mcp_server_names(cwd).contains(name) +} + +fn available_mcp_server_names(cwd: &Path) -> Vec { + let mut names: Vec = xai_grok_shell::util::config::cli_known_mcp_server_names(cwd) + .into_iter() + .collect(); + names.sort(); + names +} + +async fn run_set_enabled(name: &str, enabled: bool) -> Result<()> { + // Do not use validate_server_name (add-only: [A-Za-z0-9_-]). Enable/disable + // also targets compat/plugin names that may contain dots or other keys. + if name.is_empty() { + bail!("Server name cannot be empty."); + } + if name.starts_with("managed_gateway:") || name.contains(':') { + eprintln!( + "Gateway connectors (e.g. managed_gateway:…) cannot be toggled via CLI; use Space in /mcps." + ); + std::process::exit(1); + } + let cwd = current_dir_or_exit(); + + if !mcp_server_is_known(name, &cwd) { + eprintln!("No MCP server named '{name}'."); + let available = available_mcp_server_names(&cwd); + if !available.is_empty() { + eprintln!("Available servers: {}", available.join(", ")); + } else { + eprintln!("No MCP servers configured. Run `grok mcp add --help` to get started."); + } + std::process::exit(1); + } + + let was_disabled = xai_grok_shell::util::config::disabled_mcp_server_names(&cwd).contains(name); + + let modified = + xai_grok_shell::util::config::save_mcp_server_enabled_in(name, enabled, &cwd).await?; + + let now_disabled = xai_grok_shell::util::config::disabled_mcp_server_names(&cwd).contains(name); + let now_enabled = !now_disabled; + + if enabled && now_disabled { + eprintln!( + "Warning: '{name}' is still disabled after enable (check project-scoped config)." + ); + std::process::exit(1); + } + if !enabled && now_enabled { + eprintln!("Warning: '{name}' is still enabled after disable."); + std::process::exit(1); + } + + if was_disabled == now_disabled { + let state = if now_enabled { "enabled" } else { "disabled" }; + println!("MCP server '{name}' is already {state}."); + } else if now_enabled { + println!("Enabled MCP server '{name}'."); + } else { + println!("Disabled MCP server '{name}'."); + } + + let user_config = xai_grok_shell::util::config::user_config_path(); + for path in &modified { + if path == &user_config { + println!("File modified: {}", display_user_grok_path("config.toml")); + } else { + println!("File modified: {}", path.display()); + } + } + Ok(()) +} + async fn run_remove(name: &str, requested_scope: Option) -> Result<()> { use xai_grok_shell::util::config::{ delete_mcp_server_config_at, mcp_server_defined_at, user_config_path, @@ -1047,6 +1148,38 @@ mod tests { } } + #[test] + fn enable_and_disable_parse_name() { + let args = PagerArgs::try_parse_from(["grok", "mcp", "enable", "user-grafana"]) + .expect("enable should parse"); + match args.command { + Some(Command::Mcp(McpArgs { + command: McpCommand::Enable { name }, + })) => assert_eq!(name, "user-grafana"), + other => panic!("expected mcp enable, got {other:?}"), + } + + let args = PagerArgs::try_parse_from(["grok", "mcp", "disable", "grok_com_slack"]) + .expect("disable should parse"); + match args.command { + Some(Command::Mcp(McpArgs { + command: McpCommand::Disable { name }, + })) => assert_eq!(name, "grok_com_slack"), + other => panic!("expected mcp disable, got {other:?}"), + } + } + + #[test] + fn enable_disable_require_name() { + let err = PagerArgs::try_parse_from(["grok", "mcp", "enable"]) + .expect_err("enable without name must fail"); + assert_eq!(err.kind(), clap::error::ErrorKind::MissingRequiredArgument); + + let err = PagerArgs::try_parse_from(["grok", "mcp", "disable"]) + .expect_err("disable without name must fail"); + assert_eq!(err.kind(), clap::error::ErrorKind::MissingRequiredArgument); + } + #[test] fn select_remove_site_covers_scope_presence_matrix() { let user = xai_grok_shell::util::config::user_config_path(); diff --git a/crates/codegen/xai-grok-pager/src/minimal/api.rs b/crates/codegen/xai-grok-pager/src/minimal/api.rs index 5717a8b..1782851 100644 --- a/crates/codegen/xai-grok-pager/src/minimal/api.rs +++ b/crates/codegen/xai-grok-pager/src/minimal/api.rs @@ -589,6 +589,13 @@ pub fn sync_pending_user_input_marks(v: &mut AgentView) { v.sync_pending_user_input_marks(); } +/// Scrollback entry id of the tool row for `tool_call_id`, while the tracker +/// still has that tool pending. `None` once it has been reaped, or if it never +/// reached scrollback. +pub fn pending_tool_entry_id(v: &AgentView, tool_call_id: &str) -> Option { + v.session.tracker.pending_tool_entry_id(tool_call_id) +} + /// [`AgentView::draw_active_modal`] — minimal reuses the full-TUI modal renderer. pub fn draw_active_modal( v: &mut AgentView, diff --git a/crates/codegen/xai-grok-pager/src/scrollback/blocks/thinking.rs b/crates/codegen/xai-grok-pager/src/scrollback/blocks/thinking.rs index b7de717..79ed0c3 100644 --- a/crates/codegen/xai-grok-pager/src/scrollback/blocks/thinking.rs +++ b/crates/codegen/xai-grok-pager/src/scrollback/blocks/thinking.rs @@ -1,7 +1,8 @@ //! ThinkingBlock - displays agent thinking/reasoning content with markdown support. -use ratatui::style::{Color, Stylize}; +use ratatui::style::{Color, Modifier, Style, Stylize}; use ratatui::text::{Line, Span, Text}; +use unicode_width::UnicodeWidthStr; use crate::render::color::blend_line_with_default; use crate::scrollback::block::BlockContent; @@ -13,6 +14,61 @@ use crate::theme::Theme; use super::markdown_content::MarkdownContent; use super::quote_bar::QuoteBarStrip; +/// TODO: hard-coded because `AppView::minimal_key_intercept` matches this chord +/// literally instead of going through the keybinding registry. Resolve the +/// label from the registry once it does, so a remap is advertised correctly. +const EXPAND_HINT: &str = "ctrl+e to expand"; + +const EXPAND_HINT_GAP: &str = " "; + +/// Append the dim `(ctrl+e to expand)` affordance to a collapsed header line. +/// +/// The `Collapsed` guard matters because `render_empty_placeholder` reuses the +/// collapsed renderer for an empty body in other modes, where the hint would be +/// a lie. Skipping the hint when it does not fit keeps the header out of +/// truncation and off a second row (K5). +fn append_expand_hint(line: Line<'static>, ctx: &BlockContext) -> Line<'static> { + if !ctx + .appearance + .scrollback + .blocks + .thinking + .collapsed_expand_hint + || ctx.mode != DisplayMode::Collapsed + { + return line; + } + let hint = format!("{EXPAND_HINT_GAP}({EXPAND_HINT})"); + let used: usize = line.spans.iter().map(|s| s.content.width()).sum(); + if used + hint.width() > ctx.content_width() { + return line; + } + let mut line = line; + line.spans.push(Span::styled(hint, Theme::current().dim())); + line +} + +/// The de-emphasis patch applied to every reasoning body span when +/// [`crate::appearance::ThinkingConfig::body_dim_italic`] is on. +/// +/// Attributes only, no foreground: the flag exists because minimal's +/// terminal-native palette makes color-based de-emphasis a no-op (design doc +/// §6.16), and SGR dim/italic survive `NO_COLOR` and either polarity. +/// +/// Legacy Windows ConHost has no italic SGR and renders the request as palette +/// noise. Terminals that merely *ignore* SGR 3 (tmux without `sitm`) are not +/// gated — there is no reliable probe, and they keep the other two cues. +fn body_emphasis_patch(ctx: &BlockContext) -> Option