mirror of
https://github.com/herdrdev/herdr.git
synced 2026-09-22 00:01:06 +00:00
fix: honor droid scrollback clear requests in panes (#4432)
Remove the droid-specific filter that discarded ED3 before PTY output reached the terminal parser. Keeping stale scrollback beneath replayed primary-screen frames duplicated the welcome header and old transcript. Pass the original bytes to the parser and trackers. Add process-aware regression coverage across PTY chunk boundaries and preserve ED2/ED3 screen-history separation and alternate-screen isolation. Co-authored-by: ain3sh <ainesh.chatterjee@gmail.com> Co-authored-by: factory-droid[bot] <138933559+factory-droid[bot]@users.noreply.github.com> Co-authored-by: JJ Liebig <jonathan.liebig@gmail.com>
This commit is contained in:
co-authored by
ain3sh
factory-droid[bot]
JJ Liebig
parent
3602c757e0
commit
309749ad65
-138
@@ -1,4 +1,3 @@
|
||||
use std::borrow::Cow;
|
||||
use std::path::PathBuf;
|
||||
|
||||
use tracing::info;
|
||||
@@ -695,68 +694,6 @@ pub(super) fn current_transient_default_color_owner(shell_pid: u32) -> Option<u3
|
||||
(!foreground_job_is_shell(&job, shell_pid)).then_some(job.process_group_id)
|
||||
}
|
||||
|
||||
fn foreground_job_uses_droid_scrollback_compat(job: &crate::platform::ForegroundJob) -> bool {
|
||||
job.processes.iter().any(|process| {
|
||||
process.name.eq_ignore_ascii_case("droid")
|
||||
|| process
|
||||
.argv0
|
||||
.as_deref()
|
||||
.is_some_and(|argv0| argv0.eq_ignore_ascii_case("droid"))
|
||||
|| process.cmdline.as_deref().is_some_and(|cmdline| {
|
||||
cmdline.eq_ignore_ascii_case("droid")
|
||||
|| cmdline.starts_with("droid ")
|
||||
|| cmdline.to_ascii_lowercase().contains("/droid")
|
||||
})
|
||||
})
|
||||
}
|
||||
|
||||
pub(super) fn contains_scrollback_clear_sequence(bytes: &[u8]) -> bool {
|
||||
bytes.windows(4).any(|window| window == b"\x1b[3J")
|
||||
|| bytes.windows(5).any(|window| window == b"\x1b[?3J")
|
||||
}
|
||||
|
||||
fn strip_scrollback_clear_sequences<'a>(bytes: &'a [u8]) -> Cow<'a, [u8]> {
|
||||
if !contains_scrollback_clear_sequence(bytes) {
|
||||
return Cow::Borrowed(bytes);
|
||||
}
|
||||
|
||||
let mut filtered = Vec::with_capacity(bytes.len());
|
||||
let mut index = 0;
|
||||
while index < bytes.len() {
|
||||
let remaining = &bytes[index..];
|
||||
if remaining.starts_with(b"\x1b[3J") {
|
||||
index += 4;
|
||||
continue;
|
||||
}
|
||||
if remaining.starts_with(b"\x1b[?3J") {
|
||||
index += 5;
|
||||
continue;
|
||||
}
|
||||
filtered.push(bytes[index]);
|
||||
index += 1;
|
||||
}
|
||||
|
||||
Cow::Owned(filtered)
|
||||
}
|
||||
|
||||
pub(super) fn maybe_filter_primary_screen_scrollback_clear<'a>(
|
||||
bytes: &'a [u8],
|
||||
alternate_screen: bool,
|
||||
foreground_job: Option<&crate::platform::ForegroundJob>,
|
||||
) -> Cow<'a, [u8]> {
|
||||
// Droid redraws its primary-screen TUI with CSI 3 J, which erases pane
|
||||
// scrollback inside herdr. Keep the hack scoped to Droid on the primary
|
||||
// screen so normal terminal clear-history behavior still works elsewhere.
|
||||
if alternate_screen
|
||||
|| !contains_scrollback_clear_sequence(bytes)
|
||||
|| !foreground_job.is_some_and(foreground_job_uses_droid_scrollback_compat)
|
||||
{
|
||||
return Cow::Borrowed(bytes);
|
||||
}
|
||||
|
||||
strip_scrollback_clear_sequences(bytes)
|
||||
}
|
||||
|
||||
#[cfg(target_os = "macos")]
|
||||
pub(super) fn should_restore_host_terminal_theme(
|
||||
owner_pgid: u32,
|
||||
@@ -1350,81 +1287,6 @@ mod tests {
|
||||
);
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn droid_scrollback_compat_matches_process_name_and_cmdline() {
|
||||
let name_only = crate::platform::ForegroundJob {
|
||||
process_group_id: 42,
|
||||
processes: vec![crate::platform::ForegroundProcess {
|
||||
pid: 42,
|
||||
name: "droid".to_string(),
|
||||
argv0: None,
|
||||
argv: Some(vec![
|
||||
"/opt/factory/droid".to_string(),
|
||||
"--resume".to_string(),
|
||||
]),
|
||||
cmdline: Some("/opt/factory/droid --resume".to_string()),
|
||||
}],
|
||||
};
|
||||
assert!(foreground_job_uses_droid_scrollback_compat(&name_only));
|
||||
|
||||
let cmdline_only = crate::platform::ForegroundJob {
|
||||
process_group_id: 42,
|
||||
processes: vec![crate::platform::ForegroundProcess {
|
||||
pid: 42,
|
||||
name: "bun".to_string(),
|
||||
argv0: Some("bun".to_string()),
|
||||
argv: Some(vec![
|
||||
"bun".to_string(),
|
||||
"/home/can/.local/bin/droid".to_string(),
|
||||
"--resume".to_string(),
|
||||
]),
|
||||
cmdline: Some("/home/can/.local/bin/droid --resume".to_string()),
|
||||
}],
|
||||
};
|
||||
assert!(foreground_job_uses_droid_scrollback_compat(&cmdline_only));
|
||||
|
||||
let shell = shell_job(7);
|
||||
assert!(!foreground_job_uses_droid_scrollback_compat(&shell));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn strip_scrollback_clear_sequences_removes_ed3_only() {
|
||||
let filtered = strip_scrollback_clear_sequences(b"a\x1b[3Jb\x1b[?3Jc\x1b[2Jd");
|
||||
assert_eq!(filtered.as_ref(), b"abc\x1b[2Jd");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn primary_screen_droid_compat_ignores_scrollback_clear_only_for_droid() {
|
||||
let droid_job = crate::platform::ForegroundJob {
|
||||
process_group_id: 42,
|
||||
processes: vec![crate::platform::ForegroundProcess {
|
||||
pid: 42,
|
||||
name: "droid".to_string(),
|
||||
argv0: Some("droid".to_string()),
|
||||
argv: Some(vec!["droid".to_string()]),
|
||||
cmdline: Some("droid".to_string()),
|
||||
}],
|
||||
};
|
||||
|
||||
let filtered = maybe_filter_primary_screen_scrollback_clear(
|
||||
b"\x1b[3J\x1b[2J",
|
||||
false,
|
||||
Some(&droid_job),
|
||||
);
|
||||
assert_eq!(filtered.as_ref(), b"\x1b[2J");
|
||||
|
||||
let shell = maybe_filter_primary_screen_scrollback_clear(
|
||||
b"\x1b[3J\x1b[2J",
|
||||
false,
|
||||
Some(&shell_job(7)),
|
||||
);
|
||||
assert_eq!(shell.as_ref(), b"\x1b[3J\x1b[2J");
|
||||
|
||||
let alternate =
|
||||
maybe_filter_primary_screen_scrollback_clear(b"\x1b[3J\x1b[2J", true, Some(&droid_job));
|
||||
assert_eq!(alternate.as_ref(), b"\x1b[3J\x1b[2J");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn host_theme_restore_waits_for_shell_and_non_alternate_screen() {
|
||||
assert!(!should_restore_host_terminal_theme(
|
||||
|
||||
+8
-35
@@ -1,4 +1,3 @@
|
||||
use std::borrow::Cow;
|
||||
use std::collections::hash_map::DefaultHasher;
|
||||
use std::hash::{Hash, Hasher};
|
||||
use std::sync::{Arc, Mutex};
|
||||
@@ -33,8 +32,7 @@ use super::{
|
||||
},
|
||||
kitty_keyboard::KittyKeyboardTracker,
|
||||
osc::{
|
||||
contains_scrollback_clear_sequence, current_transient_default_color_owner,
|
||||
maybe_filter_primary_screen_scrollback_clear, parse_reported_cwd,
|
||||
current_transient_default_color_owner, parse_reported_cwd,
|
||||
restore_host_terminal_theme_if_needed, write_host_terminal_theme_selective,
|
||||
AgentOscStateTracker, DefaultColorEvent, DefaultColorEventTracker, DefaultColorOscTracker,
|
||||
DefaultColorQuery, DefaultColorTrackedEvent, OscDebugTracker,
|
||||
@@ -1381,43 +1379,18 @@ impl GhosttyPaneTerminal {
|
||||
}
|
||||
let terminal_title_changed = core.agent_osc_state.observe(bytes);
|
||||
|
||||
let alternate_screen = core
|
||||
.terminal
|
||||
.active_screen()
|
||||
.map(|screen| screen == crate::ghostty::ActiveScreen::Alternate)
|
||||
.unwrap_or(false);
|
||||
let filtered_bytes = if shell_pid > 0 {
|
||||
let foreground_job = (!alternate_screen && contains_scrollback_clear_sequence(bytes))
|
||||
.then(|| crate::detect::foreground_job(shell_pid))
|
||||
.flatten();
|
||||
maybe_filter_primary_screen_scrollback_clear(
|
||||
bytes,
|
||||
alternate_screen,
|
||||
foreground_job.as_ref(),
|
||||
)
|
||||
} else {
|
||||
Cow::Borrowed(bytes)
|
||||
};
|
||||
if filtered_bytes.len() != bytes.len() {
|
||||
debug!(
|
||||
pane = pane_id.raw(),
|
||||
shell_pid, "ignored scrollback clear sequence for droid compatibility"
|
||||
);
|
||||
}
|
||||
|
||||
core.kitty_keyboard.observe(filtered_bytes.as_ref());
|
||||
core.kitty_keyboard.observe(bytes);
|
||||
let mut terminal_responses = Vec::new();
|
||||
core.default_color_event_tracker
|
||||
.observe(filtered_bytes.as_ref());
|
||||
core.c1_xtgettcap_tracker.observe(filtered_bytes.as_ref());
|
||||
core.default_color_event_tracker.observe(bytes);
|
||||
core.c1_xtgettcap_tracker.observe(bytes);
|
||||
let c1_xtgettcap_responses = core.c1_xtgettcap_tracker.drain_pending();
|
||||
core.decscusr_tracker.observe(filtered_bytes.as_ref());
|
||||
core.decscusr_tracker.observe(bytes);
|
||||
let in_progress_default_color_event = core.default_color_event_tracker.in_progress_event();
|
||||
let default_color_events = core.default_color_event_tracker.drain_pending();
|
||||
let write_started = crate::render_prof::timer();
|
||||
self.write_pty_bytes_with_ordered_responses(
|
||||
&mut core,
|
||||
filtered_bytes.as_ref(),
|
||||
bytes,
|
||||
default_color_events,
|
||||
in_progress_default_color_event,
|
||||
c1_xtgettcap_responses,
|
||||
@@ -1435,8 +1408,8 @@ impl GhosttyPaneTerminal {
|
||||
windows_recent_fallback::update_after_write(&mut core);
|
||||
crate::render_prof::duration_since("pty.ghostty_write", write_started);
|
||||
|
||||
let has_kitty_graphics_sequence = crate::kitty_graphics::is_enabled()
|
||||
&& contains_kitty_graphics_sequence(filtered_bytes.as_ref());
|
||||
let has_kitty_graphics_sequence =
|
||||
crate::kitty_graphics::is_enabled() && contains_kitty_graphics_sequence(bytes);
|
||||
if has_kitty_graphics_sequence {
|
||||
debug!(pane = pane_id.raw(), "processed kitty graphics sequence");
|
||||
}
|
||||
|
||||
@@ -46,9 +46,13 @@ impl Harness {
|
||||
}
|
||||
|
||||
fn write(&mut self, bytes: &[u8]) {
|
||||
self.write_from_process(0, bytes);
|
||||
}
|
||||
|
||||
fn write_from_process(&mut self, shell_pid: u32, bytes: &[u8]) {
|
||||
let result = self
|
||||
.pane
|
||||
.process_pty_bytes(PaneId::from_raw(1), 0, bytes, &self.tx);
|
||||
.process_pty_bytes(PaneId::from_raw(1), shell_pid, bytes, &self.tx);
|
||||
for reply in result.terminal_responses {
|
||||
self.effects.replies.extend_from_slice(&reply);
|
||||
}
|
||||
@@ -111,6 +115,128 @@ impl Harness {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(target_os = "linux")]
|
||||
#[test]
|
||||
fn primary_screen_replay_honors_ed3_for_droid_at_chunk_boundaries() {
|
||||
// A zero PID would bypass the former process-specific filter entirely.
|
||||
// Use the real foreground-job lookup, with cleanup even on assertion failure.
|
||||
struct ChildGuard(Box<dyn portable_pty::Child + Send + Sync>);
|
||||
impl Drop for ChildGuard {
|
||||
fn drop(&mut self) {
|
||||
let _ = self.0.kill();
|
||||
let _ = self.0.wait();
|
||||
}
|
||||
}
|
||||
let pair = portable_pty::native_pty_system()
|
||||
.openpty(portable_pty::PtySize {
|
||||
rows: 24,
|
||||
cols: 80,
|
||||
pixel_width: 0,
|
||||
pixel_height: 0,
|
||||
})
|
||||
.unwrap();
|
||||
let mut command = portable_pty::CommandBuilder::new("bash");
|
||||
command.args(["-c", "exec -a droid sleep 999"]);
|
||||
let child = ChildGuard(pair.slave.spawn_command(command).unwrap());
|
||||
let pid = child.0.process_id().unwrap();
|
||||
let deadline = Instant::now() + Duration::from_secs(5);
|
||||
loop {
|
||||
let ready = crate::detect::foreground_job(pid).is_some_and(|job| {
|
||||
job.processes
|
||||
.iter()
|
||||
.any(|process| process.cmdline.as_deref() == Some("droid 999"))
|
||||
});
|
||||
if ready {
|
||||
break;
|
||||
}
|
||||
assert!(Instant::now() < deadline, "Droid foreground job not ready");
|
||||
std::thread::sleep(Duration::from_millis(10));
|
||||
}
|
||||
|
||||
for clear in ["\x1b[3J", "\x1b[?3J"] {
|
||||
let prefix = format!("\x1b[?2026h\x1b[2J{clear}\x1b[H");
|
||||
let frame = |label: &str| {
|
||||
let mut bytes = format!("{prefix}welcome\r\n");
|
||||
for row in 0..55 {
|
||||
bytes.push_str(&format!("{label}-{row:02}\r\n"));
|
||||
}
|
||||
bytes.push_str("\x1b[?2026l");
|
||||
bytes
|
||||
};
|
||||
let old = frame("old");
|
||||
let new = frame("new");
|
||||
// Whole writes catch the old filter; bytewise writes and every split
|
||||
// within the erase prefix prove that PTY chunking cannot change ED3.
|
||||
for chunk_size in [usize::MAX, 1, 7] {
|
||||
for split in 0..=prefix.len() {
|
||||
let mut harness = Harness::new(80, 24);
|
||||
for bytes in old.as_bytes().chunks(chunk_size) {
|
||||
harness.write_from_process(pid, bytes);
|
||||
}
|
||||
assert!(harness
|
||||
.pane
|
||||
.recent_text_snapshot(256)
|
||||
.text
|
||||
.contains("old-00"));
|
||||
harness.write_from_process(pid, &new.as_bytes()[..split]);
|
||||
for bytes in new.as_bytes()[split..].chunks(chunk_size) {
|
||||
harness.write_from_process(pid, bytes);
|
||||
}
|
||||
let recent = harness.pane.recent_text_snapshot(256).text;
|
||||
assert!(recent.contains("new-54"), "redraw must complete");
|
||||
assert_eq!(
|
||||
recent.matches("welcome").count(),
|
||||
1,
|
||||
"{clear:?}, split {split}, chunk {chunk_size}: {recent}"
|
||||
);
|
||||
assert!(!recent.contains("old-"), "{recent}");
|
||||
assert!(harness.pane.visible_text().contains("new-54"));
|
||||
harness.pane.scroll_up(256);
|
||||
let top = harness.pane.visible_text();
|
||||
assert!(top.contains("welcome") && top.contains("new-00"), "{top}");
|
||||
assert!(!top.contains("old-"), "{top}");
|
||||
}
|
||||
}
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn erase_display_preserves_screen_and_history_boundaries() {
|
||||
for clear in [b"\x1b[3J".as_slice(), b"\x1b[?3J"] {
|
||||
let mut harness = Harness::new(80, 24);
|
||||
for row in 0..55 {
|
||||
harness.write(format!("history-{row:02}\r\n").as_bytes());
|
||||
}
|
||||
assert!(harness
|
||||
.pane
|
||||
.recent_text_snapshot(256)
|
||||
.text
|
||||
.contains("history-00"));
|
||||
let primary = harness.pane.recent_text_snapshot(256);
|
||||
let visible = harness.pane.visible_text();
|
||||
|
||||
harness.write(b"\x1b[?1049h\x1b[2J\x1b[Halternate");
|
||||
harness.write(clear);
|
||||
assert!(harness.pane.visible_text().contains("alternate"));
|
||||
harness.write(b"\x1b[?1049l");
|
||||
assert_eq!(harness.pane.recent_text_snapshot(256), primary);
|
||||
assert_eq!(harness.pane.visible_text(), visible);
|
||||
|
||||
// ED2 clears only the display, not prior shell output in scrollback.
|
||||
harness.write(b"\x1b[2J\x1b[Hprompt");
|
||||
assert_eq!(harness.pane.visible_text().trim(), "prompt");
|
||||
assert!(harness
|
||||
.pane
|
||||
.recent_text_snapshot(256)
|
||||
.text
|
||||
.contains("history-00"));
|
||||
harness.write(clear);
|
||||
// ED3 clears history without erasing the current display.
|
||||
assert_eq!(harness.pane.visible_text().trim(), "prompt");
|
||||
assert_eq!(harness.pane.recent_text_snapshot(256).text.trim(), "prompt");
|
||||
}
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn short_streams_are_invariant_at_every_byte_boundary() {
|
||||
let fixtures: &[&[u8]] = &[
|
||||
|
||||
Reference in New Issue
Block a user