From 492f2e64c75a2a0f70b02dbd8fa10e6a49520be8 Mon Sep 17 00:00:00 2001 From: Tarun Khatri Date: Thu, 30 Apr 2026 15:16:51 +0300 Subject: [PATCH 1/3] Live read-side sync between Warp and OS shell history files (#3422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit When the user opts into the new `terminal.live_sync_os_shell_history` setting, Warp watches the active shell's history file (`~/.zsh_history`, `~/.bash_history`, fish, PSReadLine) for changes made by other terminals and merges new commands into Warp's autocomplete in real time, without needing a session restart. This implements the read-side half of GH-3422 (vscode/external terminal -> warp). The write-side half (warp -> histfile) is intentionally deferred to a follow-up PR — writing to user-owned files in $HOME has materially more edge cases (HISTCONTROL/HISTIGNORE/HISTSIZE semantics, advisory locking against the live shell) and benefits from being its own focused review. Implementation: * New singleton `ShellHistoryWatcher` (`app/src/terminal/ shell_history_watcher.rs`) wraps a dedicated `BulkFilesystemWatcher` with a refcounted `register_histfile` / `unregister_histfile` API. The existing `HomeDirectoryWatcher` only watches $HOME non-recursively, which covers bash and zsh but not fish or PSReadLine, so we register the actual histfile paths instead. * New settings group `ShellHistorySyncSettings` with one boolean `terminal.live_sync_os_shell_history` (default `false`). * `History::init_session_with` calls a new helper `maybe_register_live_sync` which, when the setting is on, expands the active session's `shell_type.history_files()` candidates and registers the existing paths with the watcher. * `History::set_up_external_history_sync` (called once at app startup from `lib.rs`) subscribes to `ShellHistoryWatcherEvent`. On each event, `handle_shell_history_watcher_event` re-reads the affected files via `async_fs`, parses with the existing per-shell `ShellType::parse_history`, and dispatches to `apply_external_history_lines`. * `apply_external_history_lines` is the merge: dedupe via `dedupe_from_last`, append entries not already in `history_file_commands[host]`, shift `session_start_indices` and `session_skip_indices` upward by N for every session on that host (the per-host history list is the concatenation `history_file_commands ++ session_commands`, so inserting at the boundary shifts every index >= old_boundary), and emit a new `HistoryEvent::ExternalHistoryUpdated { host, num_appended }`. * The four call sites that pattern-matched on `HistoryEvent::Initialized` are updated to handle the new variant. Testing: * Two new unit tests in `app/src/terminal/history_tests.rs` cover the merge logic — `external_history_lines_append_new_tail_only` checks that re-applying a histfile re-read appends only the genuinely-new lines and is idempotent on no-op re-applies, and `external_history_lines_emit_external_history_updated_event` confirms the new event variant fires with the right host and count. * `cargo check -p warp` clean locally. Tracks #3422. --- app/src/lib.rs | 10 + app/src/search/command_search/view.rs | 6 + app/src/settings/init.rs | 5 +- app/src/settings/mod.rs | 2 + app/src/settings/shell_history_sync.rs | 18 ++ app/src/terminal/history.rs | 243 +++++++++++++++++++++- app/src/terminal/history_tests.rs | 172 ++++++++++++++- app/src/terminal/mod.rs | 1 + app/src/terminal/shell_history_watcher.rs | 123 +++++++++++ app/src/terminal/view.rs | 4 + app/src/terminal/view_test.rs | 1 + app/src/test_util/settings.rs | 1 + 12 files changed, 581 insertions(+), 5 deletions(-) create mode 100644 app/src/settings/shell_history_sync.rs create mode 100644 app/src/terminal/shell_history_watcher.rs diff --git a/app/src/lib.rs b/app/src/lib.rs index d8e5e64f9..c0e04c6b7 100644 --- a/app/src/lib.rs +++ b/app/src/lib.rs @@ -1441,6 +1441,7 @@ fn initialize_app( } else { log::info!("Home directory not found; skipping HomeDirectoryWatcher registration"); } + ctx.add_singleton_model(crate::terminal::shell_history_watcher::ShellHistoryWatcher::new); } #[cfg(feature = "local_fs")] @@ -1494,6 +1495,15 @@ fn initialize_app( ctx.add_singleton_model(move |_| History::new(command_history)); + // GH-3422: subscribe History to ShellHistoryWatcher so live updates from + // other terminals (when the user has opted in via + // `terminal.live_sync_os_shell_history`) flow into Warp's autocomplete. + // Idempotent — safe to call once at app startup. + #[cfg(not(target_family = "wasm"))] + crate::terminal::history::History::handle(ctx).update(ctx, |history, ctx| { + history.set_up_external_history_sync(ctx); + }); + ctx.add_singleton_model(CustomSecretRegexUpdater::new); // Register the `TelemetryCollection` singleton model. diff --git a/app/src/search/command_search/view.rs b/app/src/search/command_search/view.rs index ce567f8ec..669411e6a 100644 --- a/app/src/search/command_search/view.rs +++ b/app/src/search/command_search/view.rs @@ -339,6 +339,12 @@ impl CommandSearchView { ctx.notify(); } } + HistoryEvent::ExternalHistoryUpdated { .. } => { + // Live OS-shell-history sync (GH-3422). The mixer's + // history source re-queries on its own debounce, so + // we just trigger a notify to refresh visible state. + ctx.notify(); + } } }); } diff --git a/app/src/settings/init.rs b/app/src/settings/init.rs index f14ec26ad..361ae94fe 100644 --- a/app/src/settings/init.rs +++ b/app/src/settings/init.rs @@ -38,8 +38,8 @@ use super::{ AliasExpansionSettings, AppEditorSettings, BlockVisibilitySettings, ChangelogSettings, CodeSettings, DebugSettings, EmacsBindingsSettings, FontSettings, FontSettingsChangedEvent, GPUSettings, InputBoxType, InputModeSettings, InputSettings, PaneSettings, - SameLinePromptBlockSettings, ScrollSettings, SelectionSettings, SshSettings, ThemeSettings, - VimBannerSettings, WarpDrivePrivacySettings, + SameLinePromptBlockSettings, ScrollSettings, SelectionSettings, ShellHistorySyncSettings, + SshSettings, ThemeSettings, VimBannerSettings, WarpDrivePrivacySettings, }; pub struct UserDefaultsOnStartup { @@ -91,6 +91,7 @@ pub fn register_all_settings(ctx: &mut AppContext) { AltScreenReporting::register(ctx); UndoCloseSettings::register(ctx); SshSettings::register(ctx); + ShellHistorySyncSettings::register(ctx); VimBannerSettings::register(ctx); SharedSessionSettings::register(ctx); WarpDriveSettings::register(ctx); diff --git a/app/src/settings/mod.rs b/app/src/settings/mod.rs index 7c99c7998..8487b33ce 100644 --- a/app/src/settings/mod.rs +++ b/app/src/settings/mod.rs @@ -29,6 +29,7 @@ mod privacy; mod same_line_prompt_block; mod scroll; mod select; +mod shell_history_sync; mod ssh; mod theme; mod vim_banner; @@ -61,6 +62,7 @@ pub use privacy::*; pub use same_line_prompt_block::*; pub use scroll::*; pub use select::*; +pub use shell_history_sync::*; pub use ssh::*; pub use theme::*; pub use vim_banner::*; diff --git a/app/src/settings/shell_history_sync.rs b/app/src/settings/shell_history_sync.rs new file mode 100644 index 000000000..5ab6d525a --- /dev/null +++ b/app/src/settings/shell_history_sync.rs @@ -0,0 +1,18 @@ +use settings::{ + macros::define_settings_group, RespectUserSyncSetting, SupportedPlatforms, SyncToCloud, +}; + +define_settings_group!(ShellHistorySyncSettings, settings: [ + live_sync_os_shell_history: LiveSyncOsShellHistoryEnabled { + type: bool, + default: false, + supported_platforms: SupportedPlatforms::ALL, + sync_to_cloud: SyncToCloud::Globally(RespectUserSyncSetting::Yes), + private: false, + toml_path: "terminal.live_sync_os_shell_history", + description: "When enabled, Warp watches the active shell's history file (~/.zsh_history, \ + ~/.bash_history, fish, PSReadLine) for changes made by other terminals and \ + merges new commands into Warp's autocomplete in real time. Off by default. \ + Tracks GH-3422.", + }, +]); diff --git a/app/src/terminal/history.rs b/app/src/terminal/history.rs index f570fba9c..dda75d58f 100644 --- a/app/src/terminal/history.rs +++ b/app/src/terminal/history.rs @@ -3,15 +3,18 @@ use futures::Future; use serde::{Deserialize, Serialize}; use std::{ collections::{HashMap, HashSet}, + path::PathBuf, sync::Arc, }; +use settings::Setting as _; use warp_core::command::ExitCode; use warpui::{AppContext, Entity, ModelContext, SingletonEntity}; use super::{ model::block::{AgentInteractionMetadata, Block, SerializedAIMetadata, SerializedBlock}, shell::ShellType, + shell_history_watcher::{ShellHistoryWatcher, ShellHistoryWatcherEvent}, }; use crate::{ cloud_object::{ @@ -19,6 +22,7 @@ use crate::{ Space, }, server::ids::{ClientId, HashableId as _, SyncId}, + settings::ShellHistorySyncSettings, terminal::model::session::{Session, SessionId}, util::dedupe_from_last, workflows::{ @@ -162,6 +166,14 @@ enum ReadHistoryFileState { pub enum HistoryEvent { /// History has been initialized for the session with the contained ID. Initialized(SessionId), + /// External history file (e.g. `~/.zsh_history`) was modified by another + /// terminal and `num_appended` new entries were merged into + /// `history_file_commands` for `host`. Listeners that cache history-derived + /// state (autocomplete index, suggestion bar) should re-query. + /// + /// Only emitted when the user has opted into + /// `terminal.live_sync_os_shell_history` (GH-3422). + ExternalHistoryUpdated { host: ShellHost, num_appended: usize }, } /// This holds the aggregated data from the "commands" table in sqlite. We aggregate as a means of @@ -191,8 +203,12 @@ pub struct History { /// execution metadata from the most recent run. persisted_commands_summary: HashMap>, - /// Entries from the history file for the host. Immutable once loaded and - /// shared between sessions. + /// Entries from the history file for the host. Loaded once at session-init + /// and shared between sessions. When the user enables + /// `terminal.live_sync_os_shell_history` (GH-3422) this map is also + /// append-only updated by [`Self::apply_external_history_lines`] whenever + /// the underlying histfile is modified by another terminal — see + /// [`Self::set_up_external_history_sync`]. history_file_commands: HashMap>>, /// Global history entries across all sessions for each host. Only grows. Deduping @@ -211,6 +227,18 @@ pub struct History { read_history_file_state: HashMap, session_id_to_shell_host: HashMap, + + /// For live OS-shell-history sync (GH-3422). Map from histfile path to the + /// set of hosts whose `history_file_commands` should be re-merged when + /// that path changes on disk. Populated by [`Self::maybe_register_live_sync`] + /// at session init (only when the live-sync setting is on); consulted by + /// [`Self::handle_shell_history_watcher_event`] when watcher events arrive. + live_sync_paths: HashMap>, + + /// Set to `true` once [`Self::set_up_external_history_sync`] has installed + /// the watcher subscription. The subscription is global and idempotent so + /// we want to install it exactly once. + external_sync_subscribed: bool, } #[derive(Clone, Debug)] @@ -568,6 +596,12 @@ impl History { self.session_id_to_shell_host .insert(session_id, host.clone()); + // GH-3422: when the user has opted into live shell-history sync, register + // this host's histfile path(s) with `ShellHistoryWatcher` so subsequent + // changes by other terminals are merged into `history_file_commands`. + // No-op when the setting is off. + self.maybe_register_live_sync(&host, ctx); + match self.read_history_file_state.get_mut(&host) { None => { let mut session_ids = HashSet::new(); @@ -964,6 +998,211 @@ impl History { } } } + + // --------------------------------------------------------------------- + // Live OS-shell-history sync (GH-3422). + // + // When the `terminal.live_sync_os_shell_history` setting is on, the + // `History` model subscribes to [`ShellHistoryWatcher`] events. When + // another terminal appends to the user's `~/.zsh_history` (or other + // shell histfile), the watcher fires, we re-read the file, parse it + // with the existing per-shell parser, and append the new commands to + // `history_file_commands` so they show up in Warp's autocomplete + // immediately. No write-back to disk happens in this code path — + // see GH-3422 follow-up. + // --------------------------------------------------------------------- + + /// Subscribe to [`ShellHistoryWatcher`] events. Idempotent. Should be + /// called once at app startup (from `lib.rs`) after `History` is + /// registered as a singleton. + pub fn set_up_external_history_sync(&mut self, ctx: &mut ModelContext) { + if self.external_sync_subscribed { + return; + } + self.external_sync_subscribed = true; + let watcher_handle = ShellHistoryWatcher::handle(ctx); + ctx.subscribe_to_model(&watcher_handle, |me, event, ctx| { + me.handle_shell_history_watcher_event(event, ctx); + }); + } + + /// Handler for [`ShellHistoryWatcherEvent::HistfilesChanged`]. For each + /// changed path that we registered in [`Self::maybe_register_live_sync`], + /// kick off an async re-read of the file and dispatch the parsed lines + /// to [`Self::apply_external_history_lines`]. + fn handle_shell_history_watcher_event( + &mut self, + event: &ShellHistoryWatcherEvent, + ctx: &mut ModelContext, + ) { + let ShellHistoryWatcherEvent::HistfilesChanged(fs_event) = event; + for path in fs_event.added_or_updated_iter() { + // Snapshot the host set under this path so we can drop the + // borrow on `self` before spawning. + let Some(hosts) = self.live_sync_paths.get(path).cloned() else { + continue; + }; + for host in hosts { + let path_for_read = path.clone(); + let shell_type = host.shell_type; + let host_for_apply = host.clone(); + ctx.spawn( + async move { + async_fs::read(&path_for_read) + .await + .ok() + .map(|bytes| shell_type.parse_history(&bytes)) + }, + move |me, lines_opt, ctx| { + if let Some(lines) = lines_opt { + me.apply_external_history_lines(host_for_apply, lines, ctx); + } + }, + ); + } + } + } + + /// Merge `new_lines` (the freshly-re-read histfile contents for `host`) + /// into `history_file_commands[host]`. Appends any commands not already + /// present, shifts session-index bookkeeping for sessions on the same + /// host, and emits [`HistoryEvent::ExternalHistoryUpdated`]. + fn apply_external_history_lines( + &mut self, + host: ShellHost, + new_lines: Vec, + ctx: &mut ModelContext, + ) { + let new_deduped = dedupe_from_last(new_lines); + + let existing_commands: HashSet = self + .history_file_commands + .get(&host) + .map(|v| v.iter().map(|e| e.command.clone()).collect()) + .unwrap_or_default(); + + let to_append: Vec = new_deduped + .into_iter() + .filter(|cmd| !existing_commands.contains(cmd)) + .collect(); + + if to_append.is_empty() { + return; + } + let n = to_append.len(); + + let new_entries: Vec> = to_append + .into_iter() + .map(|command| { + self.persisted_commands_summary + .get(&host) + .and_then(|summaries| summaries.get(&command)) + .map(|summary| summary.most_recent_entry.clone()) + .unwrap_or_else(|| HistoryEntry::command_only(command)) + }) + .map(Arc::new) + .collect(); + + // Capture old boundary BEFORE extending so the index shift below is correct. + let old_history_file_len = self + .history_file_commands + .get(&host) + .map(|v| v.len()) + .unwrap_or(0); + + self.history_file_commands + .entry(host.clone()) + .or_default() + .extend(new_entries); + + // The render-space history list for a host is + // history_file_commands[host] ++ session_commands[host] + // (see the doc comment on `session_skip_indices`). We just inserted + // `n` entries at position `old_history_file_len`, which shifts every + // index currently >= that boundary up by `n`. + let on_host_session_ids: Vec = self + .session_id_to_shell_host + .iter() + .filter(|(_, h)| **h == host) + .map(|(id, _)| *id) + .collect(); + + for session_id in &on_host_session_ids { + if let Some(start) = self.session_start_indices.get_mut(session_id) { + if *start >= old_history_file_len { + *start += n; + } + } + if let Some(skips) = self.session_skip_indices.get_mut(session_id) { + *skips = skips + .iter() + .map(|&i| if i >= old_history_file_len { i + n } else { i }) + .collect(); + } + } + + ctx.emit(HistoryEvent::ExternalHistoryUpdated { + host, + num_appended: n, + }); + } + + /// Helper called from [`Self::init_session_with`] to register the + /// active session's histfile path(s) with [`ShellHistoryWatcher`] when + /// the live-sync setting is on. No-op when off. + /// + /// Idempotent: registering the same `(path, host)` pair twice is safe — + /// the underlying watcher refcounts paths and the `live_sync_paths` + /// map is keyed by `HashSet`. + fn maybe_register_live_sync(&mut self, host: &ShellHost, ctx: &mut ModelContext) { + let enabled = *ShellHistorySyncSettings::as_ref(ctx) + .live_sync_os_shell_history + .value(); + if !enabled { + return; + } + + let Some(home) = dirs::home_dir() else { + log::warn!( + "live_sync_os_shell_history is on but no home directory could be \ + resolved; skipping live history watch registration" + ); + return; + }; + + let candidate_paths: Vec = host + .shell_type + .history_files() + .into_iter() + .filter_map(|p| { + // `history_files()` returns `~/...`-prefixed strings; expand them. + let stripped = p.strip_prefix("~/").or_else(|| p.strip_prefix("~"))?; + Some(home.join(stripped)) + }) + .collect(); + + let watcher_handle = ShellHistoryWatcher::handle(ctx); + for path in candidate_paths { + // Only register paths that actually exist on disk. Watching a + // non-existent file would either fail or rely on the watcher's + // parent-directory fallback semantics (varies by OS), and the + // initial read at session-init already produces an empty list + // for missing histfiles. + if !path.exists() { + continue; + } + self.live_sync_paths + .entry(path.clone()) + .or_default() + .insert(host.clone()); + // Always call `register_histfile` — the watcher itself refcounts, + // so registering the same path for two sessions is safe and the + // first call is the one that actually drives a syscall. + watcher_handle.update(ctx, |watcher, ctx| { + watcher.register_histfile(&path, ctx); + }); + } + } } #[cfg(test)] diff --git a/app/src/terminal/history_tests.rs b/app/src/terminal/history_tests.rs index e65b694f3..e637a756d 100644 --- a/app/src/terminal/history_tests.rs +++ b/app/src/terminal/history_tests.rs @@ -45,7 +45,9 @@ impl History { let history_handle_clone = history_handle.clone(); history_handle.update(app, move |_, ctx| { ctx.subscribe_to_model(&history_handle_clone, move |_, event, _| { - let HistoryEvent::Initialized(event_id) = event; + let HistoryEvent::Initialized(event_id) = event else { + return; + }; if session_id == *event_id { let _ = tx.try_send(()); } @@ -1023,3 +1025,171 @@ fn is_appendable_vs_is_queryable() { }); }); } + +// ----------------------------------------------------------------------------- +// GH-3422: live OS-shell-history sync (read-side, vscode -> warp). +// ----------------------------------------------------------------------------- + +/// Asserts that calling [`History::apply_external_history_lines`] with a +/// superset of the existing histfile commands appends the new tail entries to +/// `history_file_commands` and that subsequent `commands()` queries see them. +#[test] +fn external_history_lines_append_new_tail_only() { + VirtualFS::test("external_history_lines_append_new_tail", |dirs, mut sandbox| { + App::test((), |mut app| async move { + // Initial histfile content. + sandbox.with_files(vec![Stub::FileWithContentToBeTrimmed( + ".bash_history", + r#" + a + b + c + "#, + )]); + + let mut history_handle = app.add_model(|_| History::default()); + let file = Some(dirs.tests().join(".bash_history").display().to_string()); + let session = Arc::new(Session::new( + SessionInfo::new_for_test() + .with_histfile(file) + .with_shell_type(ShellType::Bash), + Arc::new(TestCommandExecutor::default()), + )); + + // Initialize with just the original three entries. + let session_clone = session.clone(); + initialize_history_for_testing( + &mut history_handle, + session.clone(), + async move { session_clone.read_history(false).await }, + vec![], + &mut app, + ) + .await; + + let host = ShellHost::from_session(session.as_ref()); + + // Sanity check: we start with exactly a, b, c. + history_handle.read(&app, |history, _| { + let cmds = history.commands(session.id()).unwrap_or_default(); + let texts: Vec<&str> = cmds.iter().map(|e| e.command.as_str()).collect(); + assert_eq!(texts, vec!["a", "b", "c"]); + }); + + // Simulate another terminal appending `d` and `e`. We pass the + // *full re-read* (not just the delta) because that's what the real + // watcher path produces — it always re-reads the whole histfile. + history_handle.update(&mut app, |history, ctx| { + history.apply_external_history_lines( + host.clone(), + vec![ + "a".to_owned(), + "b".to_owned(), + "c".to_owned(), + "d".to_owned(), + "e".to_owned(), + ], + ctx, + ); + }); + + history_handle.read(&app, |history, _| { + let cmds = history.commands(session.id()).unwrap_or_default(); + let texts: Vec<&str> = cmds.iter().map(|e| e.command.as_str()).collect(); + assert_eq!( + texts, + vec!["a", "b", "c", "d", "e"], + "expected `d` and `e` to be appended after the existing tail" + ); + }); + + // Calling again with the same content should be a no-op (no + // duplicates, no spurious new entries). + history_handle.update(&mut app, |history, ctx| { + history.apply_external_history_lines( + host.clone(), + vec![ + "a".to_owned(), + "b".to_owned(), + "c".to_owned(), + "d".to_owned(), + "e".to_owned(), + ], + ctx, + ); + }); + + history_handle.read(&app, |history, _| { + let cmds = history.commands(session.id()).unwrap_or_default(); + let texts: Vec<&str> = cmds.iter().map(|e| e.command.as_str()).collect(); + assert_eq!( + texts, + vec!["a", "b", "c", "d", "e"], + "second invocation with no new lines must not duplicate entries" + ); + }); + }); + }); +} + +/// Asserts that the new [`HistoryEvent::ExternalHistoryUpdated`] event fires +/// (with the right `host` and `num_appended` count) when external lines are +/// merged in. Listeners (autocomplete, suggestion bar) rely on this signal. +#[test] +fn external_history_lines_emit_external_history_updated_event() { + VirtualFS::test("external_history_event", |dirs, mut sandbox| { + App::test((), |mut app| async move { + sandbox.with_files(vec![Stub::FileWithContentToBeTrimmed( + ".bash_history", + r#" + a + "#, + )]); + + let mut history_handle = app.add_model(|_| History::default()); + let file = Some(dirs.tests().join(".bash_history").display().to_string()); + let session = Arc::new(Session::new( + SessionInfo::new_for_test() + .with_histfile(file) + .with_shell_type(ShellType::Bash), + Arc::new(TestCommandExecutor::default()), + )); + + let session_clone = session.clone(); + initialize_history_for_testing( + &mut history_handle, + session.clone(), + async move { session_clone.read_history(false).await }, + vec![], + &mut app, + ) + .await; + + let host = ShellHost::from_session(session.as_ref()); + + // Subscribe to capture the ExternalHistoryUpdated event. + let (tx, rx) = async_channel::unbounded::<(ShellHost, usize)>(); + let history_handle_clone = history_handle.clone(); + history_handle.update(&mut app, move |_, ctx| { + ctx.subscribe_to_model(&history_handle_clone, move |_, event, _| { + if let HistoryEvent::ExternalHistoryUpdated { host, num_appended } = event { + let _ = tx.try_send((host.clone(), *num_appended)); + } + }); + }); + + // Append two new lines. + history_handle.update(&mut app, |history, ctx| { + history.apply_external_history_lines( + host.clone(), + vec!["a".to_owned(), "b".to_owned(), "c".to_owned()], + ctx, + ); + }); + + let (event_host, num_appended) = rx.recv().await.expect("event was emitted"); + assert_eq!(event_host, host); + assert_eq!(num_appended, 2, "two new entries (`b`, `c`) were appended"); + }); + }); +} diff --git a/app/src/terminal/mod.rs b/app/src/terminal/mod.rs index d43ecdbc0..bd1ecb3b2 100644 --- a/app/src/terminal/mod.rs +++ b/app/src/terminal/mod.rs @@ -72,6 +72,7 @@ pub mod session_settings; pub mod settings; mod share_block_modal; pub mod shared_session; +pub mod shell_history_watcher; mod shell_launch_state; pub mod universal_developer_input; diff --git a/app/src/terminal/shell_history_watcher.rs b/app/src/terminal/shell_history_watcher.rs new file mode 100644 index 000000000..951323564 --- /dev/null +++ b/app/src/terminal/shell_history_watcher.rs @@ -0,0 +1,123 @@ +//! Singleton that watches the OS-side shell history files (`~/.zsh_history`, +//! `~/.bash_history`, fish, PSReadLine) for changes made by *other* terminals +//! and forwards modify events to subscribers. +//! +//! The existing [`watcher::HomeDirectoryWatcher`] only watches `$HOME` +//! non-recursively, which covers bash and zsh but not fish +//! (`~/.local/share/fish/fish_history`) or PSReadLine +//! (`~/.local/share/powershell/PSReadLine/...`). Rather than special-case those +//! paths inside the home watcher, this singleton wraps a dedicated +//! [`BulkFilesystemWatcher`] and exposes a simple `register_histfile` / +//! `unregister_histfile` API that the [`super::history::History`] model calls +//! per-session. +//! +//! See GH-3422. + +use std::collections::HashMap; +use std::path::{Path, PathBuf}; +use std::time::Duration; + +use notify_debouncer_full::notify::{RecursiveMode, WatchFilter}; +use warpui::{Entity, ModelContext, ModelHandle, SingletonEntity}; +use watcher::{BulkFilesystemWatcher, BulkFilesystemWatcherEvent}; + +/// Debounce duration for histfile watch events. Histfiles can be appended to +/// many times in quick succession (long pipelines, scripts) — debouncing keeps +/// the merge work cheap without losing observability. +const SHELL_HISTORY_WATCHER_DEBOUNCE_MS: u64 = 500; + +/// Event emitted when one of the registered histfiles changes on disk. +/// +/// Carries the underlying [`BulkFilesystemWatcherEvent`] so subscribers can +/// inspect exactly which paths were added / modified / deleted. +#[derive(Clone, Debug)] +pub enum ShellHistoryWatcherEvent { + /// One or more registered histfile paths changed. + HistfilesChanged(BulkFilesystemWatcherEvent), +} + +/// Singleton that registers individual shell history files with an underlying +/// [`BulkFilesystemWatcher`] and re-emits filesystem events under a typed +/// [`ShellHistoryWatcherEvent`]. +pub struct ShellHistoryWatcher { + watcher: ModelHandle, + /// Reference count per registered path: same histfile may be opened by + /// multiple sessions, and we only `unregister_path` once the last one + /// drops. + refcounts: HashMap, +} + +impl ShellHistoryWatcher { + pub fn new(ctx: &mut ModelContext) -> Self { + let watcher = ctx.add_model(|ctx| { + BulkFilesystemWatcher::new( + Duration::from_millis(SHELL_HISTORY_WATCHER_DEBOUNCE_MS), + ctx, + ) + }); + ctx.subscribe_to_model(&watcher, Self::handle_fs_event); + + Self { + watcher, + refcounts: HashMap::new(), + } + } + + /// Test-only constructor that uses the stub filesystem watcher (no + /// background thread). + pub fn new_for_test(ctx: &mut ModelContext) -> Self { + let watcher = ctx.add_model(|_| BulkFilesystemWatcher::new_for_test()); + Self { + watcher, + refcounts: HashMap::new(), + } + } + + /// Begin watching `path` for filesystem changes. Idempotent — calling it + /// twice for the same path bumps an internal refcount; the path is only + /// passed to the underlying watcher on the first call. + pub fn register_histfile(&mut self, path: &Path, ctx: &mut ModelContext) { + let entry = self.refcounts.entry(path.to_path_buf()).or_insert(0); + *entry += 1; + if *entry == 1 { + let path = path.to_path_buf(); + self.watcher.update(ctx, |watcher, _ctx| { + std::mem::drop(watcher.register_path( + &path, + WatchFilter::accept_all(), + RecursiveMode::NonRecursive, + )); + }); + } + } + + /// Decrement the refcount for `path`. When it hits zero the path is + /// passed to the underlying watcher's `unregister_path`. + pub fn unregister_histfile(&mut self, path: &Path, ctx: &mut ModelContext) { + let Some(count) = self.refcounts.get_mut(path) else { + return; + }; + *count = count.saturating_sub(1); + if *count == 0 { + self.refcounts.remove(path); + let path = path.to_path_buf(); + self.watcher.update(ctx, |watcher, _ctx| { + std::mem::drop(watcher.unregister_path(&path)); + }); + } + } + + fn handle_fs_event( + &mut self, + event: &BulkFilesystemWatcherEvent, + ctx: &mut ModelContext, + ) { + ctx.emit(ShellHistoryWatcherEvent::HistfilesChanged(event.clone())); + } +} + +impl Entity for ShellHistoryWatcher { + type Event = ShellHistoryWatcherEvent; +} + +impl SingletonEntity for ShellHistoryWatcher {} diff --git a/app/src/terminal/view.rs b/app/src/terminal/view.rs index 606f20fb8..3d13cc02a 100644 --- a/app/src/terminal/view.rs +++ b/app/src/terminal/view.rs @@ -12228,6 +12228,10 @@ impl TerminalView { me.pending_onboarding_agentic_suggestions_block = false; } } + HistoryEvent::ExternalHistoryUpdated { .. } => { + // Live shell-history sync (GH-3422) — no UI work needed in + // this view; autocomplete consumers re-query on their own. + } }); } diff --git a/app/src/terminal/view_test.rs b/app/src/terminal/view_test.rs index 73047e3d6..e7feb3d4e 100644 --- a/app/src/terminal/view_test.rs +++ b/app/src/terminal/view_test.rs @@ -3450,6 +3450,7 @@ fn test_first_onboarding_block_exists() { HistoryEvent::Initialized(_) => { assert!(me.onboarding_agentic_suggestions_block.is_some()); } + HistoryEvent::ExternalHistoryUpdated { .. } => {} }); }); }) diff --git a/app/src/test_util/settings.rs b/app/src/test_util/settings.rs index 813aec0cc..fc17dbd83 100644 --- a/app/src/test_util/settings.rs +++ b/app/src/test_util/settings.rs @@ -83,6 +83,7 @@ pub fn initialize_settings_for_tests_with_mode( SameLinePromptBlockSettings::register(app); ScrollSettings::register(app); SelectionSettings::register(app); + ShellHistorySyncSettings::register(app); app.update(|ctx| { WarpifySettings::register(ctx); }); From 0a819413655fcc2173a1f0d17926db5f5a5be343 Mon Sep 17 00:00:00 2001 From: Tarun Khatri Date: Thu, 30 Apr 2026 22:14:17 +0300 Subject: [PATCH 2/3] Address Oz review on PR #9584 (#3422) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four follow-up fixes for the read-side shell-history sync, in response to the Oz review on PR #9584: * **wasm**: `shell_history_watcher` and the History-side integration pulled in non-wasm-only `BulkFilesystemWatcher`, breaking wasm builds. The module declaration in `terminal/mod.rs` and the methods that touch it (`set_up_external_history_sync`, `handle_shell_history_watcher_event`, `maybe_register_live_sync`) are now `#[cfg(not(target_family = "wasm"))]`-gated, with no-op wasm stubs for the two pub methods so callers don't need their own cfg branches. * **remote sessions**: live-sync registered the *local* home-dir histfile for any session, including `WarpifiedRemote` sessions — which would mistakenly merge local command history into the remote session's autocomplete. `maybe_register_live_sync` now early-returns unless `session.session_type()` is `SessionType::Local`. * **custom HISTFILE**: the watched paths only consulted `shell_type.history_files()` defaults, so users with `HISTFILE=/some/custom/path` got the initial read but no live updates. Path resolution now prefers `session.histfile()` when set and falls back to the shell-default candidates. * **recency on re-run**: `apply_external_history_lines` filtered new lines by set membership against the current cache, so re-running an existing command in another terminal didn't move it to the most-recent position despite the deduped re-read. Switch from "append-not-already-present" to "replace-with-the-new-deduped-list": cache becomes whatever `dedupe_from_last` produces for the freshly re-read file, with session-index bookkeeping shifted by the size delta and skip-indices that pointed into the now-replaced history_file portion dropped (those entries are gone). Adds a cheap identical-content short-circuit to skip the rebuild + event emit when the watcher fires for a touch that didn't actually change deduped contents. `cargo check -p warp` clean locally. Tracks #3422. --- app/src/terminal/history.rs | 179 ++++++++++++++++++++++++------------ app/src/terminal/mod.rs | 1 + 2 files changed, 121 insertions(+), 59 deletions(-) diff --git a/app/src/terminal/history.rs b/app/src/terminal/history.rs index dda75d58f..2b4240eab 100644 --- a/app/src/terminal/history.rs +++ b/app/src/terminal/history.rs @@ -11,10 +11,11 @@ use settings::Setting as _; use warp_core::command::ExitCode; use warpui::{AppContext, Entity, ModelContext, SingletonEntity}; +#[cfg(not(target_family = "wasm"))] +use super::shell_history_watcher::{ShellHistoryWatcher, ShellHistoryWatcherEvent}; use super::{ model::block::{AgentInteractionMetadata, Block, SerializedAIMetadata, SerializedBlock}, shell::ShellType, - shell_history_watcher::{ShellHistoryWatcher, ShellHistoryWatcherEvent}, }; use crate::{ cloud_object::{ @@ -23,7 +24,7 @@ use crate::{ }, server::ids::{ClientId, HashableId as _, SyncId}, settings::ShellHistorySyncSettings, - terminal::model::session::{Session, SessionId}, + terminal::model::session::{Session, SessionId, SessionType}, util::dedupe_from_last, workflows::{ local_workflows::LocalWorkflows, workflow::Workflow, WorkflowId, WorkflowSource, @@ -596,11 +597,12 @@ impl History { self.session_id_to_shell_host .insert(session_id, host.clone()); - // GH-3422: when the user has opted into live shell-history sync, register - // this host's histfile path(s) with `ShellHistoryWatcher` so subsequent - // changes by other terminals are merged into `history_file_commands`. - // No-op when the setting is off. - self.maybe_register_live_sync(&host, ctx); + // GH-3422: when the user has opted into live shell-history sync and the + // session is local, register this session's histfile path(s) with + // `ShellHistoryWatcher` so subsequent changes by other terminals are + // merged into `history_file_commands`. No-op when the setting is off, + // when the session is remote (we only watch local files), or on wasm. + self.maybe_register_live_sync(&session, &host, ctx); match self.read_history_file_state.get_mut(&host) { None => { @@ -1015,6 +1017,7 @@ impl History { /// Subscribe to [`ShellHistoryWatcher`] events. Idempotent. Should be /// called once at app startup (from `lib.rs`) after `History` is /// registered as a singleton. + #[cfg(not(target_family = "wasm"))] pub fn set_up_external_history_sync(&mut self, ctx: &mut ModelContext) { if self.external_sync_subscribed { return; @@ -1026,10 +1029,16 @@ impl History { }); } + /// Wasm stub — no filesystem watcher available, so live shell-history sync + /// is a no-op. The setting itself is still registered, just inert. + #[cfg(target_family = "wasm")] + pub fn set_up_external_history_sync(&mut self, _ctx: &mut ModelContext) {} + /// Handler for [`ShellHistoryWatcherEvent::HistfilesChanged`]. For each /// changed path that we registered in [`Self::maybe_register_live_sync`], /// kick off an async re-read of the file and dispatch the parsed lines /// to [`Self::apply_external_history_lines`]. + #[cfg(not(target_family = "wasm"))] fn handle_shell_history_watcher_event( &mut self, event: &ShellHistoryWatcherEvent, @@ -1063,10 +1072,14 @@ impl History { } } - /// Merge `new_lines` (the freshly-re-read histfile contents for `host`) - /// into `history_file_commands[host]`. Appends any commands not already - /// present, shifts session-index bookkeeping for sessions on the same - /// host, and emits [`HistoryEvent::ExternalHistoryUpdated`]. + /// Merge a freshly-re-read histfile (`new_lines`) into + /// `history_file_commands[host]`. **Replaces** the cached entries with the + /// re-deduped list rather than appending, so a re-run of an existing + /// command in another terminal correctly moves it to the most-recent + /// position (`dedupe_from_last` keeps the last occurrence; the order of the + /// resulting list is the user's most-recent recency view of the file). + /// Shifts session-index bookkeeping by the size delta and emits + /// [`HistoryEvent::ExternalHistoryUpdated`]. fn apply_external_history_lines( &mut self, host: ShellHost, @@ -1075,23 +1088,22 @@ impl History { ) { let new_deduped = dedupe_from_last(new_lines); - let existing_commands: HashSet = self - .history_file_commands - .get(&host) - .map(|v| v.iter().map(|e| e.command.clone()).collect()) - .unwrap_or_default(); - - let to_append: Vec = new_deduped - .into_iter() - .filter(|cmd| !existing_commands.contains(cmd)) - .collect(); - - if to_append.is_empty() { - return; + // Cheap no-op short-circuit: if the new list is identical to the cache, + // skip the rebuild and the event entirely. Common when the watcher + // fires for a write that didn't actually change deduped contents + // (e.g. another process touched mtime). + if let Some(current) = self.history_file_commands.get(&host) { + if current.len() == new_deduped.len() + && current + .iter() + .zip(new_deduped.iter()) + .all(|(entry, command)| entry.command == *command) + { + return; + } } - let n = to_append.len(); - let new_entries: Vec> = to_append + let new_entries: Vec> = new_deduped .into_iter() .map(|command| { self.persisted_commands_summary @@ -1103,23 +1115,27 @@ impl History { .map(Arc::new) .collect(); - // Capture old boundary BEFORE extending so the index shift below is correct. let old_history_file_len = self .history_file_commands .get(&host) .map(|v| v.len()) .unwrap_or(0); + let new_history_file_len = new_entries.len(); self.history_file_commands - .entry(host.clone()) - .or_default() - .extend(new_entries); + .insert(host.clone(), new_entries); // The render-space history list for a host is // history_file_commands[host] ++ session_commands[host] - // (see the doc comment on `session_skip_indices`). We just inserted - // `n` entries at position `old_history_file_len`, which shifts every - // index currently >= that boundary up by `n`. + // The boundary moved from `old_history_file_len` to + // `new_history_file_len`. For sessions on this host, indices that + // pointed into the session_commands portion (i.e. were + // `>= old_history_file_len`) need to shift by the same delta. + // Indices that pointed into the history_file portion are now stale + // because the file portion was replaced — drop them from + // `session_skip_indices` and clamp `session_start_indices` to the new + // boundary at minimum. + let delta: isize = new_history_file_len as isize - old_history_file_len as isize; let on_host_session_ids: Vec = self .session_id_to_shell_host .iter() @@ -1129,32 +1145,62 @@ impl History { for session_id in &on_host_session_ids { if let Some(start) = self.session_start_indices.get_mut(session_id) { - if *start >= old_history_file_len { - *start += n; - } + let shifted = (*start as isize + delta).max(new_history_file_len as isize); + *start = shifted.max(0) as usize; } if let Some(skips) = self.session_skip_indices.get_mut(session_id) { *skips = skips .iter() - .map(|&i| if i >= old_history_file_len { i + n } else { i }) + .filter_map(|&i| { + if i >= old_history_file_len { + // index pointed into session_commands; shift by delta + let shifted = i as isize + delta; + (shifted >= 0).then_some(shifted as usize) + } else { + // index pointed into the now-replaced history_file + // portion; drop it (the underlying entries are gone) + None + } + }) .collect(); } } + let num_appended = new_history_file_len.saturating_sub(old_history_file_len); ctx.emit(HistoryEvent::ExternalHistoryUpdated { host, - num_appended: n, + num_appended, }); } - /// Helper called from [`Self::init_session_with`] to register the - /// active session's histfile path(s) with [`ShellHistoryWatcher`] when - /// the live-sync setting is on. No-op when off. + /// Helper called from [`Self::init_session_with`] to register the active + /// session's histfile path with [`ShellHistoryWatcher`] when the live-sync + /// setting is on. No-op when: + /// * the setting is off, or + /// * the session is remote (we deliberately don't watch the *local* + /// home-dir histfile for a remote session — that would merge local + /// command history into the remote session's autocomplete), or + /// * we're targeting wasm (no filesystem watcher available). + /// + /// Honors the session's resolved `HISTFILE` (`session.info.histfile`) + /// so users with custom paths get live updates too; falls back to the + /// shell's default histfile candidates. /// /// Idempotent: registering the same `(path, host)` pair twice is safe — /// the underlying watcher refcounts paths and the `live_sync_paths` /// map is keyed by `HashSet`. - fn maybe_register_live_sync(&mut self, host: &ShellHost, ctx: &mut ModelContext) { + #[cfg(not(target_family = "wasm"))] + fn maybe_register_live_sync( + &mut self, + session: &Arc, + host: &ShellHost, + ctx: &mut ModelContext, + ) { + // Live-sync watches a *local* file — meaningless for a remote session. + if !matches!(session.session_type(), SessionType::Local) { + return; + } + let enabled = *ShellHistorySyncSettings::as_ref(ctx) .live_sync_os_shell_history .value(); @@ -1162,25 +1208,30 @@ impl History { return; } - let Some(home) = dirs::home_dir() else { - log::warn!( - "live_sync_os_shell_history is on but no home directory could be \ - resolved; skipping live history watch registration" - ); - return; + // Prefer the session's resolved `HISTFILE` (handles `HISTFILE=...` + // overrides in the user's rc files). Fall back to the shell's default + // candidates expanded against the home directory. + let candidate_paths: Vec = if let Some(custom) = session.histfile().as_deref() { + vec![PathBuf::from(custom)] + } else { + let Some(home) = dirs::home_dir() else { + log::warn!( + "live_sync_os_shell_history is on but no home directory could be \ + resolved; skipping live history watch registration" + ); + return; + }; + host.shell_type + .history_files() + .into_iter() + .filter_map(|p| { + // `history_files()` returns `~/...`-prefixed strings; expand them. + let stripped = p.strip_prefix("~/").or_else(|| p.strip_prefix("~"))?; + Some(home.join(stripped)) + }) + .collect() }; - let candidate_paths: Vec = host - .shell_type - .history_files() - .into_iter() - .filter_map(|p| { - // `history_files()` returns `~/...`-prefixed strings; expand them. - let stripped = p.strip_prefix("~/").or_else(|| p.strip_prefix("~"))?; - Some(home.join(stripped)) - }) - .collect(); - let watcher_handle = ShellHistoryWatcher::handle(ctx); for path in candidate_paths { // Only register paths that actually exist on disk. Watching a @@ -1203,6 +1254,16 @@ impl History { }); } } + + /// Wasm stub for [`Self::maybe_register_live_sync`]. + #[cfg(target_family = "wasm")] + fn maybe_register_live_sync( + &mut self, + _session: &Arc, + _host: &ShellHost, + _ctx: &mut ModelContext, + ) { + } } #[cfg(test)] diff --git a/app/src/terminal/mod.rs b/app/src/terminal/mod.rs index bd1ecb3b2..cb882a110 100644 --- a/app/src/terminal/mod.rs +++ b/app/src/terminal/mod.rs @@ -72,6 +72,7 @@ pub mod session_settings; pub mod settings; mod share_block_modal; pub mod shared_session; +#[cfg(not(target_family = "wasm"))] pub mod shell_history_watcher; mod shell_launch_state; pub mod universal_developer_input; From 1e16bf17dc689a975c86eb40781b72923c9f6caf Mon Sep 17 00:00:00 2001 From: Tarun Khatri Date: Thu, 30 Apr 2026 23:59:17 +0300 Subject: [PATCH 3/3] Address third-pass Oz review on PR #9584 (#3422) Four follow-ups for the live read-side history sync, in response to the third Oz review pass: 1. Race: register watcher AFTER initial load completes The first version called maybe_register_live_sync() before kicking off the async initial-load future, so a watcher event arriving between registration and load completion could trigger a re-read whose result would then be overwritten by the still-pending initial load. Move the registration into the post-load callback (None branch of init_session_with) so it only fires once load_history_file_commands has populated history_file_commands. The Done branch (initial load already happened on a prior session) registers inline as before; InProgress now no-ops because the in-flight sessions --- app/src/terminal/history.rs | 200 ++++++++++++++++++++++++------ app/src/terminal/history_tests.rs | 3 + 2 files changed, 167 insertions(+), 36 deletions(-) diff --git a/app/src/terminal/history.rs b/app/src/terminal/history.rs index 2b4240eab..7f913a98a 100644 --- a/app/src/terminal/history.rs +++ b/app/src/terminal/history.rs @@ -232,10 +232,31 @@ pub struct History { /// For live OS-shell-history sync (GH-3422). Map from histfile path to the /// set of hosts whose `history_file_commands` should be re-merged when /// that path changes on disk. Populated by [`Self::maybe_register_live_sync`] - /// at session init (only when the live-sync setting is on); consulted by - /// [`Self::handle_shell_history_watcher_event`] when watcher events arrive. + /// after the initial histfile load completes (only when the live-sync + /// setting is on); consulted by [`Self::handle_shell_history_watcher_event`] + /// when watcher events arrive. live_sync_paths: HashMap>, + /// One [`Arc`] per live-sync-enabled host, kept around so the + /// watcher's async re-read can route through the same `Session::read_history` + /// path the initial load uses — including the Windows PowerShell / + /// Kaspersky workaround in `read_powershell_history_contents`. Without + /// this, the live re-read would `async_fs::read` directly and bypass that + /// fallback. Any session for the host is fine; we just need *some* + /// `Session` whose `info` resolves the histfile and Kaspersky flag the + /// same way the initial read did. + live_sync_sessions: HashMap>, + + /// Per-host generation counter incremented on every authoritative refresh + /// of `history_file_commands[host]` (initial load **and** + /// [`Self::apply_external_history_lines`]). The watcher's async re-read + /// snapshots the generation when it kicks off and only applies its result + /// if the generation still matches at completion time. Without this, two + /// watcher events firing close together can race: the second + /// (most-recent) read can complete first, then the first (older) read + /// completes and rolls history back. + live_sync_generation: HashMap, + /// Set to `true` once [`Self::set_up_external_history_sync`] has installed /// the watcher subscription. The subscription is global and idempotent so /// we want to install it exactly once. @@ -597,13 +618,6 @@ impl History { self.session_id_to_shell_host .insert(session_id, host.clone()); - // GH-3422: when the user has opted into live shell-history sync and the - // session is local, register this session's histfile path(s) with - // `ShellHistoryWatcher` so subsequent changes by other terminals are - // merged into `history_file_commands`. No-op when the setting is off, - // when the session is remote (we only watch local files), or on wasm. - self.maybe_register_live_sync(&session, &host, ctx); - match self.read_history_file_state.get_mut(&host) { None => { let mut session_ids = HashSet::new(); @@ -616,6 +630,8 @@ impl History { }, ); let host_clone = host.clone(); + let host_for_live_sync = host.clone(); + let session_for_live_sync = session.clone(); ctx.spawn( read_history_file_future, move |me, history_file_commands, ctx| { @@ -645,6 +661,18 @@ impl History { host, ctx, ); + // GH-3422: register the histfile with `ShellHistoryWatcher` + // *after* the initial load is committed. Registering before + // would race: a watcher event arriving between registration + // and load-completion would kick off an async re-read whose + // result could be overwritten by the still-pending initial + // load. No-op when the setting is off, the session is + // remote, or we're on wasm. + me.maybe_register_live_sync( + &session_for_live_sync, + &host_for_live_sync, + ctx, + ); }, ); } @@ -664,14 +692,22 @@ impl History { let mut session_ids = HashSet::new(); session_ids.insert(session_id); + let host_for_live_sync = host.clone(); self.initialize_session_start_and_skip_indices( session_ids, host, session_start_index, ctx, - ) + ); + // Initial load already happened on a prior session; safe to + // register live-sync now (idempotent for hosts where another + // session already registered this path). + self.maybe_register_live_sync(&session, &host_for_live_sync, ctx); } Some(ReadHistoryFileState::InProgress { session_ids, .. }) => { + // Another session for this host is mid-load. The deferred + // registration in that session's spawn closure covers this + // host already — no need to register again here. session_ids.insert(session_id); } } @@ -714,6 +750,11 @@ impl History { .insert(host.clone(), session_commands_to_append); } + // Bump the live-sync generation so any in-flight async re-read started + // *before* this load completed sees a stale generation and drops its + // result instead of overwriting the freshly-loaded state. (GH-3422) + *self.live_sync_generation.entry(host.clone()).or_insert(0) += 1; + self.initialize_session_start_and_skip_indices(session_ids, host, start_index, ctx); } @@ -1036,14 +1077,19 @@ impl History { /// Handler for [`ShellHistoryWatcherEvent::HistfilesChanged`]. For each /// changed path that we registered in [`Self::maybe_register_live_sync`], - /// kick off an async re-read of the file and dispatch the parsed lines - /// to [`Self::apply_external_history_lines`]. + /// kick off an async re-read **through the same `Session::read_history` + /// path the initial load uses** (so the Windows PowerShell / Kaspersky + /// workaround in `read_powershell_history_contents` is honored), snapshot + /// the host's live-sync generation, and dispatch the parsed lines to + /// [`Self::apply_external_history_lines`] only if the generation still + /// matches at completion time. #[cfg(not(target_family = "wasm"))] fn handle_shell_history_watcher_event( &mut self, event: &ShellHistoryWatcherEvent, ctx: &mut ModelContext, ) { + let is_kaspersky_running = Self::is_kaspersky_running(ctx); let ShellHistoryWatcherEvent::HistfilesChanged(fs_event) = event; for path in fs_event.added_or_updated_iter() { // Snapshot the host set under this path so we can drop the @@ -1052,20 +1098,29 @@ impl History { continue; }; for host in hosts { - let path_for_read = path.clone(); - let shell_type = host.shell_type; + // Need a `Session` for this host to route through + // `Session::read_history` (Kaspersky/PowerShell-aware on + // Windows). Stored in `live_sync_sessions` at registration. + let Some(session) = self.live_sync_sessions.get(&host).cloned() else { + log::debug!( + "GH-3422: watcher fired for {host:?} but no live_sync_sessions entry; \ + skipping (host probably already torn down)" + ); + continue; + }; + // Snapshot the generation so a stale completion can't roll + // back a newer apply that landed first. + let snapshot_gen = self.live_sync_generation.get(&host).copied().unwrap_or(0); let host_for_apply = host.clone(); ctx.spawn( - async move { - async_fs::read(&path_for_read) - .await - .ok() - .map(|bytes| shell_type.parse_history(&bytes)) - }, - move |me, lines_opt, ctx| { - if let Some(lines) = lines_opt { - me.apply_external_history_lines(host_for_apply, lines, ctx); - } + async move { session.read_history(is_kaspersky_running).await }, + move |me, lines, ctx| { + me.apply_external_history_lines( + host_for_apply, + lines, + Some(snapshot_gen), + ctx, + ); }, ); } @@ -1080,12 +1135,38 @@ impl History { /// resulting list is the user's most-recent recency view of the file). /// Shifts session-index bookkeeping by the size delta and emits /// [`HistoryEvent::ExternalHistoryUpdated`]. + /// + /// **Generation check**: when `snapshot_gen` is `Some(n)`, n is the + /// live-sync generation at the time the async re-read kicked off. If + /// the host's current generation has moved on (because the initial + /// load completed in the meantime, or another live-sync update landed + /// first), we drop this stale read instead of overwriting newer state. + /// `None` skips the check (used by direct test callers). + /// + /// **session_commands dedupe**: when the same command appears in both + /// the freshly re-read histfile and `session_commands[host]` (the + /// commands this Warp session has executed), we drop the duplicate from + /// `session_commands` so the user sees it only once in autocomplete. fn apply_external_history_lines( &mut self, host: ShellHost, new_lines: Vec, + snapshot_gen: Option, ctx: &mut ModelContext, ) { + // Stale-read guard: if the generation has moved on since this async + // re-read started, drop the result. Newer state has already landed. + if let Some(snap) = snapshot_gen { + let current_gen = self.live_sync_generation.get(&host).copied().unwrap_or(0); + if current_gen != snap { + log::debug!( + "GH-3422: dropping stale live-sync read for {host:?} \ + (snapshot_gen={snap}, current_gen={current_gen})" + ); + return; + } + } + let new_deduped = dedupe_from_last(new_lines); // Cheap no-op short-circuit: if the new list is identical to the cache, @@ -1122,20 +1203,59 @@ impl History { .unwrap_or(0); let new_history_file_len = new_entries.len(); + // Build a set of commands now in the (replaced) history_file portion + // so we can drop matching entries from session_commands below — the + // user otherwise sees them twice in autocomplete (once from the + // re-read histfile, once from this Warp session's own log). + let new_history_command_set: HashSet = + new_entries.iter().map(|e| e.command.clone()).collect(); + self.history_file_commands .insert(host.clone(), new_entries); + // Snapshot pre-dedupe session_commands length so we can compute how + // many entries get removed below. + let old_session_commands_len = self + .session_commands + .get(&host) + .map(|v| v.len()) + .unwrap_or(0); + + // Drop session_commands entries whose command string appears in the + // newly-replaced history_file_commands. This keeps each command + // visible exactly once in the rendered autocomplete list. + if let Some(session_cmds) = self.session_commands.get_mut(&host) { + session_cmds.retain(|entry| !new_history_command_set.contains(&entry.command)); + } + let new_session_commands_len = self + .session_commands + .get(&host) + .map(|v| v.len()) + .unwrap_or(0); + let session_commands_dropped = old_session_commands_len - new_session_commands_len; + + // Bump the live-sync generation so any later in-flight read started + // before this apply ran sees a stale generation and drops its result. + *self.live_sync_generation.entry(host.clone()).or_insert(0) += 1; + // The render-space history list for a host is // history_file_commands[host] ++ session_commands[host] - // The boundary moved from `old_history_file_len` to - // `new_history_file_len`. For sessions on this host, indices that - // pointed into the session_commands portion (i.e. were - // `>= old_history_file_len`) need to shift by the same delta. - // Indices that pointed into the history_file portion are now stale - // because the file portion was replaced — drop them from - // `session_skip_indices` and clamp `session_start_indices` to the new - // boundary at minimum. - let delta: isize = new_history_file_len as isize - old_history_file_len as isize; + // + // The history_file boundary moved from `old_history_file_len` to + // `new_history_file_len` (delta_history). The session_commands + // portion shrank by `session_commands_dropped` (delta_session is + // negative). For sessions on this host: + // * `session_start_indices` is positioned just past the snapshot + // of history+session at session-start time. After this apply, + // it must shift by the net delta so it still points to "where + // this session's commands begin". + // * `session_skip_indices` are absolute indices into the concat + // list. Indices in the (now-replaced) history_file portion are + // gone; indices in the session_commands portion shift by the + // net delta and need to drop entries that were among the + // deduped-out commands. + let delta_history: isize = new_history_file_len as isize - old_history_file_len as isize; + let net_delta: isize = delta_history - session_commands_dropped as isize; let on_host_session_ids: Vec = self .session_id_to_shell_host .iter() @@ -1145,7 +1265,7 @@ impl History { for session_id in &on_host_session_ids { if let Some(start) = self.session_start_indices.get_mut(session_id) { - let shifted = (*start as isize + delta).max(new_history_file_len as isize); + let shifted = (*start as isize + net_delta).max(new_history_file_len as isize); *start = shifted.max(0) as usize; } if let Some(skips) = self.session_skip_indices.get_mut(session_id) { @@ -1153,8 +1273,8 @@ impl History { .iter() .filter_map(|&i| { if i >= old_history_file_len { - // index pointed into session_commands; shift by delta - let shifted = i as isize + delta; + // index pointed into session_commands; shift by net delta + let shifted = i as isize + net_delta; (shifted >= 0).then_some(shifted as usize) } else { // index pointed into the now-replaced history_file @@ -1232,6 +1352,14 @@ impl History { .collect() }; + // Stash an `Arc` for this host so the watcher's async + // re-read can route through `Session::read_history` (Kaspersky/PowerShell- + // aware on Windows). One session per host is enough — they're + // equivalent for histfile-read purposes. + self.live_sync_sessions + .entry(host.clone()) + .or_insert_with(|| session.clone()); + let watcher_handle = ShellHistoryWatcher::handle(ctx); for path in candidate_paths { // Only register paths that actually exist on disk. Watching a diff --git a/app/src/terminal/history_tests.rs b/app/src/terminal/history_tests.rs index e637a756d..ec95be630 100644 --- a/app/src/terminal/history_tests.rs +++ b/app/src/terminal/history_tests.rs @@ -1089,6 +1089,7 @@ fn external_history_lines_append_new_tail_only() { "d".to_owned(), "e".to_owned(), ], + None, ctx, ); }); @@ -1115,6 +1116,7 @@ fn external_history_lines_append_new_tail_only() { "d".to_owned(), "e".to_owned(), ], + None, ctx, ); }); @@ -1183,6 +1185,7 @@ fn external_history_lines_emit_external_history_updated_event() { history.apply_external_history_lines( host.clone(), vec!["a".to_owned(), "b".to_owned(), "c".to_owned()], + None, ctx, ); });