From 948e42397fcb0f3355e07d633cafcfc843111f12 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 11 Jun 2026 02:26:45 +0000 Subject: [PATCH] =?UTF-8?q?fix(tui):=20#3029=20audit=20fix=20=E2=80=94=20i?= =?UTF-8?q?nterleave=20OSC=208=20open/close=20per=20region=20in=20diff=20o?= =?UTF-8?q?rder=20(OSC=208=20is=20last-writer-wins=20state;=20batched=20op?= =?UTF-8?q?ens=20linked=20the=20whole=20frame=20to=20the=20last=20target);?= =?UTF-8?q?=20drop=20cursor-move=20hack=20and=20dead=20pending=5Flinks;=20?= =?UTF-8?q?add=20byte-stream=20bracketing=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Claude https://claude.ai/code/session_018zaP8vUfTAsrE38L6h6fw5 --- crates/tui/src/tui/color_compat.rs | 201 +++++++++++++++++++++++------ crates/tui/src/tui/osc8.rs | 1 - 2 files changed, 165 insertions(+), 37 deletions(-) diff --git a/crates/tui/src/tui/color_compat.rs b/crates/tui/src/tui/color_compat.rs index 31bd24d5..0082830f 100644 --- a/crates/tui/src/tui/color_compat.rs +++ b/crates/tui/src/tui/color_compat.rs @@ -17,7 +17,6 @@ use ratatui::{ }; use crate::palette::{self, ColorDepth, PaletteMode, ThemeId, UiTheme}; -use crate::tui::osc8::LinkRegion; const RENDER_DEBUG_ENV: &str = "CODEWHALE_TUI_DEBUG"; const RENDER_DEBUG_SAMPLE_LIMIT: usize = 24; @@ -50,11 +49,6 @@ pub(crate) struct ColorCompatBackend { /// the live crossterm query. terminal_size: Option, render_debug: Option, - /// OSC 8 link regions to emit during the next `draw()` call (#3029). - /// Set by the render closure before `terminal.draw()`; cleared after each - /// draw so stale links don't persist across frames. - #[allow(dead_code)] // populated via set_pending_links from render closure - pending_links: Vec, } impl ColorCompatBackend { @@ -72,20 +66,9 @@ impl ColorCompatBackend { forced_size: None, terminal_size: None, render_debug: RenderDebugLog::from_env(), - pending_links: Vec::new(), } } - #[allow(dead_code)] // called from render closure (future integration) - pub(crate) fn set_pending_links(&mut self, links: Vec) { - self.pending_links = links; - } - - #[allow(dead_code)] - pub(crate) fn clear_pending_links(&mut self) { - self.pending_links.clear(); - } - pub(crate) fn force_size(&mut self, size: Size) { self.forced_size = Some(size); } @@ -147,26 +130,48 @@ impl Backend for ColorCompatBackend { render_debug.record(viewport, &adapted); } // #3029: Emit OSC 8 hyperlinks out-of-band through the backend's - // Write impl. ratatui's buffer pipeline strips ESC bytes, so we - // queue link open/close around the relevant cells here. - let frame_links = crate::tui::osc8::take_frame_links(); - let link_active = !frame_links.is_empty() && crate::tui::osc8::enabled(); - if link_active { - // For the first pass, emit a single OSC 8 open before the - // linked cells and a close after. Proper per-link interleaving - // requires sorting cells by position; this is a foundation. - for link in &frame_links { - let _ = crate::tui::osc8::write_osc8_open(self, &link.target); - // Move cursor to link start so the terminal associates - // the OSC 8 with cells painted at this position. - let _ = self.inner.set_cursor_position((link.col_start, link.row)); - } + // Write impl. ratatui's buffer pipeline strips ESC bytes, so the + // open/close sequences must be interleaved with the cell stream + // here. OSC 8 is stateful and last-writer-wins: every cell painted + // between an open and the next close links to that open's target, + // so each region's cells must be bracketed by their OWN open/close + // pair — never batched. + let mut frame_links = crate::tui::osc8::take_frame_links(); + if frame_links.is_empty() || !crate::tui::osc8::enabled() { + self.inner + .draw(adapted.iter().map(|(x, y, cell)| (*x, *y, cell)))?; + return Ok(()); } - self.inner - .draw(adapted.iter().map(|(x, y, cell)| (*x, *y, cell)))?; - if link_active { - for _link in &frame_links { - let _ = crate::tui::osc8::write_osc8_close(self); + // Deterministic region lookup when regions are adjacent/overlapping: + // the first (top-left-most) region wins. + frame_links.sort_unstable_by_key(|link| (link.row, link.col_start)); + let region_for = |x: u16, y: u16| -> Option { + frame_links + .iter() + .position(|link| y == link.row && x >= link.col_start && x <= link.col_end) + }; + + // Walk the diff in its original order and split it into runs at + // region boundaries, so the visible byte stream stays identical to + // a no-link render apart from the inserted OSC 8 sequences. + let mut idx = 0; + while idx < adapted.len() { + let current_region = region_for(adapted[idx].0, adapted[idx].1); + let run_start = idx; + while idx < adapted.len() + && region_for(adapted[idx].0, adapted[idx].1) == current_region + { + idx += 1; + } + let run = &adapted[run_start..idx]; + if let Some(region_idx) = current_region { + crate::tui::osc8::write_osc8_open(self, &frame_links[region_idx].target)?; + self.inner + .draw(run.iter().map(|(x, y, cell)| (*x, *y, cell)))?; + crate::tui::osc8::write_osc8_close(self)?; + } else { + self.inner + .draw(run.iter().map(|(x, y, cell)| (*x, *y, cell)))?; } } Ok(()) @@ -579,4 +584,128 @@ mod tests { backend.force_size(Size::new(80, 25)); assert_eq!(backend.size().unwrap(), Size::new(80, 25)); } + + // ── #3029: OSC 8 emission through the backend byte stream ────────────── + + fn row_cells(symbols: &str) -> Vec<(u16, u16, Cell)> { + symbols + .chars() + .enumerate() + .map(|(i, ch)| { + let mut cell = Cell::default(); + cell.set_symbol(&ch.to_string()); + (u16::try_from(i).unwrap(), 0u16, cell) + }) + .collect() + } + + #[test] + fn osc8_open_close_bracket_only_their_region_cells() { + use crate::tui::osc8::LinkRegion; + + // Baseline: identical cells, no link regions. + let baseline_writer = SharedWriter::default(); + let baseline_capture = baseline_writer.0.clone(); + let mut baseline = + ColorCompatBackend::new(baseline_writer, ColorDepth::TrueColor, PaletteMode::Dark); + let cells = row_cells("ABCDE"); + baseline + .draw(cells.iter().map(|(x, y, cell)| (*x, *y, cell))) + .unwrap(); + let baseline_out = String::from_utf8_lossy(&baseline_capture.borrow()).to_string(); + + // Linked render: columns 2..=3 ("CD") carry one link region. + crate::tui::osc8::set_frame_links(vec![LinkRegion { + row: 0, + col_start: 2, + col_end: 3, + target: "https://example.test/1".to_string(), + }]); + let writer = SharedWriter::default(); + let capture = writer.0.clone(); + let mut backend = ColorCompatBackend::new(writer, ColorDepth::TrueColor, PaletteMode::Dark); + let cells = row_cells("ABCDE"); + backend + .draw(cells.iter().map(|(x, y, cell)| (*x, *y, cell))) + .unwrap(); + let out = String::from_utf8_lossy(&capture.borrow()).to_string(); + + let open = "\x1b]8;;https://example.test/1\x1b\\"; + let close = "\x1b]8;;\x1b\\"; + assert_eq!(out.matches(open).count(), 1, "exactly one open: {out:?}"); + assert_eq!(out.matches(close).count(), 1, "exactly one close: {out:?}"); + + // The open must precede the first linked glyph and the close must sit + // between the last linked glyph and the first glyph after the region. + let open_at = out.find(open).expect("open present"); + let close_at = out.find(close).expect("close present"); + let c_at = out.find('C').expect("glyph C"); + let d_at = out.find('D').expect("glyph D"); + let e_at = out.find('E').expect("glyph E"); + assert!(open_at < c_at, "open before linked cells: {out:?}"); + assert!(d_at < close_at, "close after linked cells: {out:?}"); + assert!( + close_at < e_at, + "cells after the region must not inherit the link: {out:?}" + ); + + // Visible glyph stream is unchanged by link insertion. + let mut baseline_visible = String::new(); + crate::tui::osc8::strip_ansi_into(&baseline_out, &mut baseline_visible); + let mut linked_visible = String::new(); + crate::tui::osc8::strip_ansi_into(&out, &mut linked_visible); + assert_eq!( + baseline_visible, linked_visible, + "link emission must not move or alter visible cells" + ); + } + + #[test] + fn osc8_two_regions_link_to_their_own_targets() { + use crate::tui::osc8::LinkRegion; + + crate::tui::osc8::set_frame_links(vec![ + LinkRegion { + row: 0, + col_start: 0, + col_end: 1, + target: "https://example.test/first".to_string(), + }, + LinkRegion { + row: 0, + col_start: 3, + col_end: 4, + target: "https://example.test/second".to_string(), + }, + ]); + let writer = SharedWriter::default(); + let capture = writer.0.clone(); + let mut backend = ColorCompatBackend::new(writer, ColorDepth::TrueColor, PaletteMode::Dark); + let cells = row_cells("ABZCD"); + backend + .draw(cells.iter().map(|(x, y, cell)| (*x, *y, cell))) + .unwrap(); + let out = String::from_utf8_lossy(&capture.borrow()).to_string(); + + let first = "\x1b]8;;https://example.test/first\x1b\\"; + let second = "\x1b]8;;https://example.test/second\x1b\\"; + let close = "\x1b]8;;\x1b\\"; + assert_eq!(out.matches(first).count(), 1, "{out:?}"); + assert_eq!(out.matches(second).count(), 1, "{out:?}"); + assert_eq!(out.matches(close).count(), 2, "{out:?}"); + + // Pre-#3029-audit bug: both opens were emitted before any cell, so + // the whole frame linked to the LAST region's target. Each region's + // open must close before the next region's open begins. + let first_at = out.find(first).expect("first open"); + let first_close_at = out[first_at..].find(close).expect("first close") + first_at; + let second_at = out.find(second).expect("second open"); + assert!( + first_close_at < second_at, + "region one must close before region two opens: {out:?}" + ); + // The unlinked middle glyph sits between the two link spans. + let z_at = out.find('Z').expect("unlinked glyph"); + assert!(first_close_at < z_at && z_at < second_at, "{out:?}"); + } } diff --git a/crates/tui/src/tui/osc8.rs b/crates/tui/src/tui/osc8.rs index 933708e5..b13e9834 100644 --- a/crates/tui/src/tui/osc8.rs +++ b/crates/tui/src/tui/osc8.rs @@ -28,7 +28,6 @@ const OSC8_CLOSE: &str = "\x1b]8;;\x1b\\"; pub struct LinkRegion { pub row: u16, pub col_start: u16, - #[allow(dead_code)] // used by future buffer-scan link detection pub col_end: u16, pub target: String, }