From 392fc8a07455a01a92559eff19c4d1bcef09019d Mon Sep 17 00:00:00 2001 From: akbash-bot <300245827+akbash-bot@users.noreply.github.com> Date: Fri, 7 Aug 2026 20:40:43 +0000 Subject: [PATCH] fix: forward pane terminal bells refs #2453 --- docs/next/CHANGELOG.md | 4 ++ docs/next/api/herdr-api.schema.json | 2 +- src/app/actions.rs | 3 +- src/app/api.rs | 13 +++++ src/client/mod.rs | 7 +++ src/events.rs | 3 ++ src/ghostty/mod.rs | 20 ++++++++ src/main.rs | 1 + src/pane.rs | 56 +++++++++++++++++++++ src/pane/terminal.rs | 23 ++++++++- src/protocol/wire.rs | 17 ++++++- src/server/headless.rs | 75 +++++++++++++++++++++++++++++ src/terminal_effects.rs | 27 +++++++++++ tests/api_ping.rs | 2 +- tests/cli/sessions.rs | 12 ++--- tests/support/mod.rs | 2 +- 16 files changed, 255 insertions(+), 12 deletions(-) create mode 100644 src/terminal_effects.rs diff --git a/docs/next/CHANGELOG.md b/docs/next/CHANGELOG.md index ce029cca43..080f0710a9 100644 --- a/docs/next/CHANGELOG.md +++ b/docs/next/CHANGELOG.md @@ -8,10 +8,14 @@ - Settings and `ui.status_indicators = "symbols"` can now use distinct static shapes for blocked, working, done, idle, and unknown agent states. (#2260) - The plugin marketplace now discovers valid manifests at repository roots and subdirectories, groups multiple plugins under each repository, and publishes their versions and exact default-branch commits. +### Changed +- Bumped the client/server protocol version to 20 for pane terminal bell forwarding. + ### Fixed - `herdr config check` now reports unknown built-in theme names instead of silently accepting them. (#2452) - macOS `herdr --remote` clients now keep the accepted bridge socket blocking, preventing an immediate disconnect after the protocol handshake. (#2478, thanks @mathijshenquet) - Prefix keybindings now preserve Shift in WezTerm Kitty keyboard mode, so commands such as config reload no longer trigger their unshifted action. (#2435) +- BEL characters emitted by pane programs now reach the outer terminal so its audible and visual bell settings can react. (#2453) - Stable direct installs, self-updates, and remote helper downloads now require and verify the SHA-256 digest published for each GitHub release asset. - Configs containing the retired Herdr-written `ui.agent_panel_scope` setting no longer report it as an unknown key after upgrades. (#2292) - Claude Code confirmation prompts using `Enter to confirm · Esc to cancel` now report `blocked` instead of `idle`. (#2268) diff --git a/docs/next/api/herdr-api.schema.json b/docs/next/api/herdr-api.schema.json index 1a91870bab..4183c2ba0d 100644 --- a/docs/next/api/herdr-api.schema.json +++ b/docs/next/api/herdr-api.schema.json @@ -1,6 +1,6 @@ { "$schema": "https://json-schema.org/draft/2020-12/schema", - "protocol": 19, + "protocol": 20, "schema_version": 1, "schemas": { "error_response": { diff --git a/src/app/actions.rs b/src/app/actions.rs index 51de049c9b..40707cee26 100644 --- a/src/app/actions.rs +++ b/src/app/actions.rs @@ -2902,9 +2902,10 @@ impl AppState { .collect() } } - // Both intercepted before this dispatch — in App::handle_internal_event (monolithic) + // Intercepted before this dispatch — in App::handle_internal_event (monolithic) // or via HeadlessServer forwarding to the foreground client (server); never touch // AppState. Kept for AppEvent exhaustiveness. + AppEvent::TerminalBell { .. } => Vec::new(), AppEvent::ClipboardWrite { .. } => Vec::new(), AppEvent::PrefixInputSource { .. } => Vec::new(), AppEvent::TerminalCwdReported { pane_id, cwd } => { diff --git a/src/app/api.rs b/src/app/api.rs index 5f81a5cb22..c2130f3602 100644 --- a/src/app/api.rs +++ b/src/app/api.rs @@ -64,6 +64,10 @@ impl App { results, cache_updates, } => self.handle_git_status_refreshed(results, cache_updates), + ev @ AppEvent::TerminalBell { .. } => { + self.handle_internal_event(ev); + false + } ev => { self.handle_internal_event(ev); true @@ -97,6 +101,15 @@ impl App { } pub(crate) fn handle_internal_event(&mut self, ev: AppEvent) { + if let AppEvent::TerminalBell { count, .. } = ev { + if let Err(err) = + crate::terminal_effects::write_terminal_bells(&mut std::io::stdout(), count) + { + tracing::warn!(err = %err, "failed to emit terminal bell"); + } + return; + } + if let AppEvent::ClipboardWrite { content } = ev { #[cfg(not(test))] crate::selection::write_osc52_bytes(&content); diff --git a/src/client/mod.rs b/src/client/mod.rs index b153dd93ce..7932cb79bf 100644 --- a/src/client/mod.rs +++ b/src/client/mod.rs @@ -1571,6 +1571,13 @@ async fn run_client_loop( let _ = stdout.flush(); } } + ServerMessage::TerminalBell { count } => { + if let Err(err) = + crate::terminal_effects::write_terminal_bells(&mut io::stdout(), count) + { + warn!(err = %err, "failed to emit terminal bell"); + } + } ServerMessage::ServerShutdown { reason } => { return Err(ClientError::ServerShutdown { reason }); } diff --git a/src/events.rs b/src/events.rs index a3869ef741..7510ae3c61 100644 --- a/src/events.rs +++ b/src/events.rs @@ -124,6 +124,9 @@ pub enum AppEvent { updated: Vec, status: crate::detect::manifest_update::ManifestUpdateStatus, }, + /// A pane child emitted one or more executable BEL characters. + /// The host-facing process forwards them to its outer terminal. + TerminalBell { pane_id: PaneId, count: u16 }, /// A pane child emitted a valid OSC 52 clipboard write. The main loop /// re-emits it through herdr's own clipboard writer. ClipboardWrite { content: Vec }, diff --git a/src/ghostty/mod.rs b/src/ghostty/mod.rs index 04ad87af2d..93efbd76d7 100644 --- a/src/ghostty/mod.rs +++ b/src/ghostty/mod.rs @@ -470,12 +470,22 @@ const MAX_CLIPBOARD_BYTES: usize = 192 * 1024; #[derive(Default)] struct TerminalCallbackState { write_pty: Option>, + bell_count: u16, pwd_changes: Vec>, clipboard_writes: Vec>, size_report: ffi::GhosttySizeReportSize, color_scheme: Option, } +unsafe extern "C" fn bell_trampoline(_terminal: ffi::GhosttyTerminal, userdata: *mut c_void) { + if userdata.is_null() { + return; + } + // SAFETY: userdata is the TerminalCallbackState installed with this terminal. + let state = unsafe { &mut *userdata.cast::() }; + state.bell_count = state.bell_count.saturating_add(1); +} + unsafe extern "C" fn color_scheme_trampoline( _terminal: ffi::GhosttyTerminal, userdata: *mut c_void, @@ -820,6 +830,12 @@ impl Terminal { (size_trampoline as *const ()).cast(), ) .into_result()?; + ffi::ghostty_terminal_set( + terminal.raw, + ffi::GhosttyTerminalOption_GHOSTTY_TERMINAL_OPT_BELL, + (bell_trampoline as *const ()).cast(), + ) + .into_result()?; ffi::ghostty_terminal_set( terminal.raw, ffi::GhosttyTerminalOption_GHOSTTY_TERMINAL_OPT_PWD_CHANGED, @@ -979,6 +995,10 @@ impl Terminal { mem::replace(&mut self.callback_state.color_scheme, color_scheme) } + pub fn take_bell_count(&mut self) -> u16 { + mem::take(&mut self.callback_state.bell_count) + } + pub fn take_pwd_changes(&mut self) -> Vec> { mem::take(&mut self.callback_state.pwd_changes) } diff --git a/src/main.rs b/src/main.rs index 85ff264b7a..7f79730d54 100644 --- a/src/main.rs +++ b/src/main.rs @@ -93,6 +93,7 @@ mod server; mod session; mod sound; mod terminal; +mod terminal_effects; mod terminal_modes; mod terminal_notify; mod terminal_theme; diff --git a/src/pane.rs b/src/pane.rs index 7669fc13cd..ab984946e3 100644 --- a/src/pane.rs +++ b/src/pane.rs @@ -1499,6 +1499,20 @@ fn usable_reported_cwd(cwd: std::path::PathBuf) -> Option { (cwd.is_absolute() && cwd.is_dir()).then_some(cwd) } +fn publish_terminal_bells(pane_id: PaneId, count: u16, events: &mpsc::Sender) { + if count == 0 { + return; + } + if let Err(err) = events.try_send(AppEvent::TerminalBell { pane_id, count }) { + warn!( + pane = pane_id.raw(), + count, + err = %err, + "failed to queue terminal bell" + ); + } +} + fn publish_reported_cwd( pane_id: PaneId, cwd: std::path::PathBuf, @@ -1868,6 +1882,7 @@ impl PaneRuntime { let shell_pid = child_pid.load(Ordering::Acquire); let result = terminal.process_pty_bytes(pane_id, shell_pid, bytes, &response_writer); + publish_terminal_bells(pane_id, result.terminal_bells, &read_events); observe_detection_content_change(bytes, &detection_content_seq); if result.request_render && render_dirty.request_pty(pane_id) { render_notify.notify_one(); @@ -2028,6 +2043,7 @@ impl PaneRuntime { let shell_pid = child_pid.load(Ordering::Acquire); let result = terminal.process_pty_bytes(pane_id, shell_pid, bytes, &response_writer); + publish_terminal_bells(pane_id, result.terminal_bells, &events); if agent_detection == AgentDetection::Enabled { observe_detection_content_change(bytes, &detection_content_seq); } @@ -4214,6 +4230,46 @@ mod tests { .expect("re-entering active authority should notify detection reset"); } + #[cfg(unix)] + #[tokio::test] + async fn spawned_pty_reader_aggregates_terminal_bells() { + let (events, mut event_rx) = mpsc::channel(8); + let pane_id = PaneId::from_raw(42); + let runtime = PaneRuntime::spawn_shell_command( + pane_id, + 24, + 80, + std::env::temp_dir(), + "printf '\\a\\a'; sleep 0.05", + &PaneLaunchEnv::default(), + AgentDetection::Disabled, + 0, + crate::terminal_theme::TerminalTheme::default(), + None, + events, + Arc::new(Notify::new()), + Arc::new(RenderSignal::new()), + ) + .unwrap(); + + let bell = tokio::time::timeout(std::time::Duration::from_secs(2), async { + loop { + if let Some(AppEvent::TerminalBell { + pane_id: delivered_pane, + count, + }) = event_rx.recv().await + { + break (delivered_pane, count); + } + } + }) + .await + .expect("PTY reader should publish terminal bells"); + + assert_eq!(bell, (pane_id, 2)); + runtime.shutdown(); + } + #[tokio::test] async fn state_changed_event_waits_for_queue_space_instead_of_dropping() { let (tx, mut rx) = mpsc::channel(1); diff --git a/src/pane/terminal.rs b/src/pane/terminal.rs index 67b571537b..8de84af39e 100644 --- a/src/pane/terminal.rs +++ b/src/pane/terminal.rs @@ -145,6 +145,7 @@ impl InputState { pub(crate) struct ProcessBytesResult { pub request_render: bool, pub render_delay: Option, + pub terminal_bells: u16, pub clipboard_writes: Vec>, pub reported_cwd: Option, pub terminal_responses: Vec, @@ -1201,6 +1202,7 @@ impl GhosttyPaneTerminal { return ProcessBytesResult { request_render: false, render_delay: None, + terminal_bells: 0, clipboard_writes: Vec::new(), reported_cwd: None, terminal_responses: Vec::new(), @@ -1209,7 +1211,8 @@ impl GhosttyPaneTerminal { let _ = core.terminal.take_pwd_changes(); // Restored history may have exercised terminal callbacks before this live PTY write. - // Those writes must not be delivered as live pane output. + // Those effects must not be delivered as live pane output. + let _ = core.terminal.take_bell_count(); let _ = core.terminal.take_clipboard_writes(); let default_color_observation = core.default_color_tracker.observe(bytes); if shell_pid > 0 && default_color_observation { @@ -1276,6 +1279,7 @@ impl GhosttyPaneTerminal { xtgettcap_responses, &mut terminal_responses, ); + let terminal_bells = core.terminal.take_bell_count(); let clipboard_writes = core.terminal.take_clipboard_writes(); let reported_cwd = core .terminal @@ -1332,6 +1336,7 @@ impl GhosttyPaneTerminal { ProcessBytesResult { request_render, render_delay, + terminal_bells, clipboard_writes, reported_cwd, terminal_responses, @@ -3734,6 +3739,21 @@ mod tests { ); } + #[test] + fn process_pty_bytes_surfaces_live_bells_only() { + let (tx, _rx) = mpsc::channel(4); + let terminal = crate::ghostty::Terminal::new(80, 24, 100).unwrap(); + let pane = GhosttyPaneTerminal::new(terminal, tx.clone()).unwrap(); + let pane_id = PaneId::from_raw(1); + + pane.seed_history_ansi("stale\x07"); + let result = pane.process_pty_bytes(pane_id, 0, b"\x07\x1b]0;title\x07\x07", &tx); + + assert_eq!(result.terminal_bells, 2); + let drained = pane.process_pty_bytes(pane_id, 0, b"live output", &tx); + assert_eq!(drained.terminal_bells, 0); + } + #[test] fn process_pty_bytes_surfaces_clipboard_writes_without_other_results() { let (tx, _rx) = mpsc::channel(4); @@ -3749,6 +3769,7 @@ mod tests { assert!(result.request_render); assert_eq!(result.render_delay, None); + assert_eq!(result.terminal_bells, 0); assert_eq!(result.clipboard_writes, vec![b"clipboard".to_vec()]); assert_eq!(result.reported_cwd, None); assert!(result.terminal_responses.is_empty()); diff --git a/src/protocol/wire.rs b/src/protocol/wire.rs index 655f47cb44..4af2c79492 100644 --- a/src/protocol/wire.rs +++ b/src/protocol/wire.rs @@ -13,7 +13,7 @@ use serde::{Deserialize, Serialize}; // --------------------------------------------------------------------------- /// Current protocol version. Bumped when wire format changes incompatibly. -pub const PROTOCOL_VERSION: u32 = 19; +pub const PROTOCOL_VERSION: u32 = 20; /// Maximum allowed frame payload size (2 MB). Frames larger than this are /// rejected to prevent denial-of-service via oversized length prefixes. @@ -711,6 +711,12 @@ pub enum ServerMessage { /// Whether the ASCII input source should be active. active: bool, }, + + /// Ring the foreground client's outer terminal for pane-originated BEL characters. + TerminalBell { + /// Number of BEL characters parsed from one PTY read. + count: u16, + }, } // --------------------------------------------------------------------------- @@ -1555,6 +1561,15 @@ mod tests { } } + #[test] + fn server_terminal_bell_roundtrip() { + let msg = ServerMessage::TerminalBell { count: 3 }; + let encoded = bincode::serde::encode_to_vec(&msg, bincode::config::standard()).unwrap(); + let (decoded, _): (ServerMessage, _) = + bincode::serde::decode_from_slice(&encoded, bincode::config::standard()).unwrap(); + assert_eq!(msg, decoded); + } + // ---- Framing ---- #[test] diff --git a/src/server/headless.rs b/src/server/headless.rs index 859e02af2a..5a90177b82 100644 --- a/src/server/headless.rs +++ b/src/server/headless.rs @@ -2098,6 +2098,15 @@ impl HeadlessServer { /// Returns true if the event changed visual state (requiring a re-render). fn handle_internal_event_with_forwarding(&mut self, ev: AppEvent) -> bool { match &ev { + AppEvent::TerminalBell { pane_id, count } => { + if !self.send_to_foreground_client(ServerMessage::TerminalBell { count: *count }) { + debug!( + pane = pane_id.raw(), + count, "dropped terminal bell without a foreground client" + ); + } + false + } AppEvent::ClipboardWrite { content } => { // Clipboard writes are client-local side effects. Forward them only to // the foreground client instead of broadcasting to every attached client. @@ -9583,6 +9592,72 @@ next_tab = "" assert!(!server.app.state.request_client_config_reload); } + #[test] + fn terminal_bell_targets_foreground_client_only() { + let mut server = test_headless_server(); + let (background_tx, background_control_rx, _background_rx) = test_client_writer(); + let (foreground_tx, foreground_control_rx, _foreground_rx) = test_client_writer(); + + server.clients.insert( + 1, + ClientConnection::new( + (120, 40), + crate::kitty_graphics::HostCellSize::default(), + crate::terminal_theme::TerminalTheme::default(), + None, + 1, + RenderEncoding::SemanticFrame, + Some(background_tx), + ), + ); + server.clients.insert( + 2, + ClientConnection::new( + (80, 24), + crate::kitty_graphics::HostCellSize::default(), + crate::terminal_theme::TerminalTheme::default(), + None, + 2, + RenderEncoding::SemanticFrame, + Some(foreground_tx), + ), + ); + server.foreground_client_id = Some(2); + + let changed = server.handle_internal_event_with_forwarding(AppEvent::TerminalBell { + pane_id: crate::layout::PaneId::from_raw(1), + count: 3, + }); + + assert!(!changed); + match read_server_message( + foreground_control_rx + .recv_timeout(Duration::from_millis(100)) + .expect("foreground terminal bell message"), + ) { + ServerMessage::TerminalBell { count } => assert_eq!(count, 3), + other => panic!("expected terminal bell message, got {other:?}"), + } + assert!( + background_control_rx + .recv_timeout(Duration::from_millis(50)) + .is_err(), + "background client should not receive terminal bells" + ); + + server.foreground_client_id = None; + server.handle_internal_event_with_forwarding(AppEvent::TerminalBell { + pane_id: crate::layout::PaneId::from_raw(1), + count: 1, + }); + assert!( + foreground_control_rx + .recv_timeout(Duration::from_millis(50)) + .is_err(), + "bells without a foreground client must not be retained" + ); + } + #[test] fn clipboard_write_targets_foreground_client_only() { let mut server = test_headless_server(); diff --git a/src/terminal_effects.rs b/src/terminal_effects.rs new file mode 100644 index 0000000000..88fcf55f8c --- /dev/null +++ b/src/terminal_effects.rs @@ -0,0 +1,27 @@ +use std::io::{self, Write}; + +const BELL_CHUNK: [u8; 64] = [b'\x07'; 64]; + +pub(crate) fn write_terminal_bells(writer: &mut W, count: u16) -> io::Result<()> { + let full_chunks = usize::from(count) / BELL_CHUNK.len(); + let remainder = usize::from(count) % BELL_CHUNK.len(); + for _ in 0..full_chunks { + writer.write_all(&BELL_CHUNK)?; + } + writer.write_all(&BELL_CHUNK[..remainder])?; + writer.flush() +} + +#[cfg(test)] +mod tests { + use super::*; + + #[test] + fn writes_exact_terminal_bell_count() { + let mut output = Vec::new(); + + write_terminal_bells(&mut output, 130).unwrap(); + + assert_eq!(output, vec![b'\x07'; 130]); + } +} diff --git a/tests/api_ping.rs b/tests/api_ping.rs index 1033628a92..f4f92da681 100644 --- a/tests/api_ping.rs +++ b/tests/api_ping.rs @@ -304,7 +304,7 @@ fn ping_over_socket_returns_version() { assert_eq!(value["result"]["version"], env!("CARGO_PKG_VERSION")); // Intentionally hardcoded so wire protocol bumps require updating this test. // Changing this value means old clients/servers are no longer compatible. - assert_eq!(value["result"]["protocol"], 19); + assert_eq!(value["result"]["protocol"], 20); cleanup_spawned_herdr(child, base); } diff --git a/tests/cli/sessions.rs b/tests/cli/sessions.rs index 78b046809a..d86de44a65 100644 --- a/tests/cli/sessions.rs +++ b/tests/cli/sessions.rs @@ -389,7 +389,7 @@ fn status_commands_report_client_and_server_versions() { "stdout: {full_stdout}" ); assert!( - full_stdout.contains(" protocol: 19"), + full_stdout.contains(" protocol: 20"), "stdout: {full_stdout}" ); assert!(full_stdout.contains("server:\n"), "stdout: {full_stdout}"); @@ -422,7 +422,7 @@ fn status_commands_report_client_and_server_versions() { "stdout: {server_stdout}" ); assert!( - server_stdout.contains("protocol: 19"), + server_stdout.contains("protocol: 20"), "stdout: {server_stdout}" ); @@ -434,7 +434,7 @@ fn status_commands_report_client_and_server_versions() { "stdout: {client_stdout}" ); assert!( - client_stdout.contains("protocol: 19"), + client_stdout.contains("protocol: 20"), "stdout: {client_stdout}" ); assert!( @@ -444,7 +444,7 @@ fn status_commands_report_client_and_server_versions() { let full_json = run_cli_json(&socket_path, &["status", "--json"]); assert_eq!(full_json["client"]["version"], env!("CARGO_PKG_VERSION")); - assert_eq!(full_json["client"]["protocol"], 19); + assert_eq!(full_json["client"]["protocol"], 20); assert_eq!(full_json["server"]["status"], "running"); assert_eq!(full_json["server"]["running"], true); assert_eq!(full_json["server"]["compatible"], true); @@ -458,12 +458,12 @@ fn status_commands_report_client_and_server_versions() { let server_json = run_cli_json(&socket_path, &["status", "server", "--json"]); assert_eq!(server_json["status"], "running"); assert_eq!(server_json["version"], env!("CARGO_PKG_VERSION")); - assert_eq!(server_json["protocol"], 19); + assert_eq!(server_json["protocol"], 20); assert_eq!(server_json["compatible"], true); let client_json = run_cli_json(&socket_path, &["status", "client", "--json"]); assert_eq!(client_json["version"], env!("CARGO_PKG_VERSION")); - assert_eq!(client_json["protocol"], 19); + assert_eq!(client_json["protocol"], 20); assert!(client_json["binary"] .as_str() .is_some_and(|path| !path.is_empty())); diff --git a/tests/support/mod.rs b/tests/support/mod.rs index b10e2a93ae..f0ed100bcc 100644 --- a/tests/support/mod.rs +++ b/tests/support/mod.rs @@ -15,7 +15,7 @@ static INIT: Once = Once::new(); static CLEANUP_GUARD: OnceLock = OnceLock::new(); const WATCHDOG_SCAN_INTERVAL: Duration = Duration::from_secs(1); const RUNTIME_OWNER_MARKER: &str = ".herdr-test-owner-pid"; -pub const CURRENT_PROTOCOL: u32 = 19; +pub const CURRENT_PROTOCOL: u32 = 20; pub fn register_spawned_herdr_pid(pid: Option) { let Some(pid) = pid else {