From c7139c2128feb8f730ea5ff97c9334a5de05b77c Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Tue, 4 Aug 2026 23:18:05 -0500 Subject: [PATCH 1/7] fix(iced): anchor sidebar-grip presses at the last event-carried move MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit iced's ButtonPressed carries no position, and iced_winit drains an event batch with a single batch-newest cursor — so a seam press made during pointer motion was hit-tested at a position the pointer only reached after the press, missing the ±3px grip zone (issue #295). The grip now records the last CursorMoved's own position (events process in receipt order, so at press time it holds where the pointer was when the press fired) and gates presses on that anchor, falling back to the batch cursor before any observed motion. Hardened per review: the anchor drops on CursorLeft, and only ever replaces an available batch cursor, so an unavailable-cursor press stays a no-op. The ReorderStrip half of #295 is deliberately NOT fixed: both strips sit under iced scrollables, which pass children a translated cursor but the raw untranslated event, so an event-position anchor is wrong by the scroll offset — split to #300 with the analysis. The real-input harness dwell that existed solely as the workaround (iced_clipboard_check.py, commit 2671ba1) is removed; the cage-tier run now proves the fix. Grip unit tests drive the real update() path (the precomputed in_zone test shortcut is gone) — negative controls confirmed each new test fails with its guard reverted. Cursor review: strip coordinate-space + anchor-staleness findings → strip reverted, grip hardened (this shape). Simplify pass: no changes. Closes #295 (plan 014 C1) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- crates/roost-iced/src/sidebar_resize.rs | 188 ++++++++++++++++++---- tools/input/linux/iced_clipboard_check.py | 9 -- 2 files changed, 156 insertions(+), 41 deletions(-) diff --git a/crates/roost-iced/src/sidebar_resize.rs b/crates/roost-iced/src/sidebar_resize.rs index de385e13..e819b232 100644 --- a/crates/roost-iced/src/sidebar_resize.rs +++ b/crates/roost-iced/src/sidebar_resize.rs @@ -41,6 +41,7 @@ struct Drag { #[derive(Debug, Default)] struct State { drag: Option, + last_cursor: Option, } #[derive(Clone, Copy, Debug, PartialEq)] @@ -73,11 +74,14 @@ fn cursor_in_zone(bounds: Rectangle, seam_x: f32, position: Point) -> bool { && (position.x - seam_x).abs() <= GRIP_HALF_WIDTH } +fn over_seam_at(layout: Layout<'_>, position: Point) -> bool { + seam_x(layout).is_some_and(|seam| cursor_in_zone(layout.bounds(), seam, position)) +} + fn over_seam(layout: Layout<'_>, cursor: mouse::Cursor) -> bool { cursor .position() - .zip(seam_x(layout)) - .is_some_and(|(position, seam)| cursor_in_zone(layout.bounds(), seam, position)) + .is_some_and(|position| over_seam_at(layout, position)) } fn dragged_width(drag: Drag, x: f32) -> f32 { @@ -87,10 +91,31 @@ fn dragged_width(drag: Drag, x: f32) -> f32 { fn owns_event( state: &mut State, event: &Event, - cursor: Option, - in_zone: bool, + layout: Layout<'_>, + cursor: mouse::Cursor, current_width: f32, ) -> Ownership { + // Recorded before every early return: `ButtonPressed` carries no position + // of its own, and iced hit-tests it against the newest cursor of the batch + // it was drained with. A frame behind, that is wherever the pointer + // travelled *after* the button went down, so the last move the grip + // actually saw is the honest press anchor (issue #295). A pointer that + // left the window invalidates it — the next entry can land anywhere, and + // no move need be observed before the press. + match event { + Event::Mouse(mouse::Event::CursorMoved { position, .. }) => { + state.last_cursor = Some(*position); + } + Event::Mouse(mouse::Event::CursorLeft) => state.last_cursor = None, + _ => {} + } + // Anchoring only ever *replaces* an available batch cursor. With no cursor + // at all the grip has always been a no-op, and a stale anchor must not + // start arming presses it used to ignore. + let anchored = cursor + .position() + .map(|batch| state.last_cursor.unwrap_or(batch)); + match event { Event::Mouse(mouse::Event::ButtonPressed(mouse::Button::Left)) => { // A second press during a live drag belongs to the grip too: it @@ -99,7 +124,7 @@ fn owns_event( if state.drag.is_some() { return Ownership::Own(None); } - let Some(position) = cursor.filter(|_| in_zone) else { + let Some(position) = anchored.filter(|position| over_seam_at(layout, *position)) else { return Ownership::Delegate; }; state.drag = Some(Drag { @@ -204,8 +229,8 @@ impl Widget for SidebarResizeGrip<'_> { match owns_event( tree.state.downcast_mut::(), event, - cursor.position(), - over_seam(layout, cursor), + layout, + cursor, self.current_width, ) { Ownership::Delegate => {} @@ -323,29 +348,34 @@ mod tests { }) } + fn left() -> Event { + Event::Mouse(mouse::Event::CursorLeft) + } + fn dragging() -> State { State { drag: Some(Drag { start_x: 220.0, start_width: 220.0, }), + ..State::default() } } + /// Drives the real ownership path: the seam zone is computed from + /// `sidebar_width`'s layout, never handed in precomputed. fn owns( state: &mut State, event: &Event, + sidebar_width: f32, cursor_x: Option, - in_zone: bool, current_width: f32, ) -> Ownership { - owns_event( - state, - event, - cursor_x.map(|x| Point::new(x, 20.0)), - in_zone, - current_width, - ) + let node = split(sidebar_width); + let cursor = cursor_x.map_or(mouse::Cursor::Unavailable, |x| { + mouse::Cursor::Available(Point::new(x, 20.0)) + }); + owns_event(state, event, Layout::new(&node), cursor, current_width) } #[test] @@ -390,7 +420,7 @@ mod tests { fn seam_press_is_owned_and_a_press_elsewhere_is_delegated() { let mut state = State::default(); assert_eq!( - owns(&mut state, &press(), Some(221.0), true, 220.0), + owns(&mut state, &press(), 220.0, Some(221.0), 220.0), Ownership::Own(None) ); assert_eq!( @@ -403,7 +433,7 @@ mod tests { let mut elsewhere = State::default(); assert_eq!( - owns(&mut elsewhere, &press(), Some(600.0), false, 220.0), + owns(&mut elsewhere, &press(), 220.0, Some(600.0), 220.0), Ownership::Delegate ); assert_eq!(elsewhere.drag, None); @@ -413,31 +443,31 @@ mod tests { fn motion_and_release_are_owned_only_while_a_drag_is_live() { let mut idle = State::default(); assert_eq!( - owns(&mut idle, &moved(600.0), Some(600.0), false, 220.0), + owns(&mut idle, &moved(600.0), 220.0, Some(600.0), 220.0), Ownership::Delegate ); assert_eq!( - owns(&mut idle, &release(), Some(600.0), false, 220.0), + owns(&mut idle, &release(), 220.0, Some(600.0), 220.0), Ownership::Delegate ); let mut live = dragging(); assert_eq!( - owns(&mut live, &moved(300.0), Some(300.0), false, 220.0), + owns(&mut live, &moved(300.0), 220.0, Some(300.0), 220.0), Ownership::Own(Some(GripEvent::Dragged { width: 300.0 })) ); // The pointer left the window: the drag still owns the motion. assert_eq!( - owns(&mut live, &moved(280.0), None, false, 220.0), + owns(&mut live, &moved(280.0), 220.0, None, 220.0), Ownership::Own(Some(GripEvent::Dragged { width: 280.0 })) ); assert_eq!( - owns(&mut live, &release(), None, false, 220.0), + owns(&mut live, &release(), 220.0, None, 220.0), Ownership::Own(Some(GripEvent::Ended)) ); assert_eq!(live.drag, None); assert_eq!( - owns(&mut live, &release(), None, false, 220.0), + owns(&mut live, &release(), 220.0, None, 220.0), Ownership::Delegate ); } @@ -446,7 +476,7 @@ mod tests { fn a_move_that_recomputes_the_current_width_owns_without_publishing() { let mut live = dragging(); assert_eq!( - owns(&mut live, &moved(220.0), Some(220.0), false, 220.0), + owns(&mut live, &moved(220.0), 220.0, Some(220.0), 220.0), Ownership::Own(None) ); // Past the clamp bound the width stops moving, so neither does the app. @@ -455,9 +485,10 @@ mod tests { start_x: 220.0, start_width: MIN_WIDTH, }), + ..State::default() }; assert_eq!( - owns(&mut clamped, &moved(100.0), Some(100.0), false, MIN_WIDTH), + owns(&mut clamped, &moved(100.0), 220.0, Some(100.0), MIN_WIDTH), Ownership::Own(None) ); } @@ -467,12 +498,12 @@ mod tests { let mut live = dragging(); let unfocused = Event::Window(window::Event::Unfocused); assert_eq!( - owns(&mut live, &unfocused, None, false, 220.0), + owns(&mut live, &unfocused, 220.0, None, 220.0), Ownership::PublishAndDelegate(GripEvent::Ended) ); assert_eq!(live.drag, None); assert_eq!( - owns(&mut live, &unfocused, None, false, 220.0), + owns(&mut live, &unfocused, 220.0, None, 220.0), Ownership::Delegate ); } @@ -483,10 +514,10 @@ mod tests { start_x: 220.0, start_width: 220.0, }; - for (cursor_x, in_zone) in [(Some(221.0), true), (Some(600.0), false), (None, false)] { + for cursor_x in [Some(221.0), Some(600.0), None] { let mut live = dragging(); assert_eq!( - owns(&mut live, &press(), cursor_x, in_zone, 300.0), + owns(&mut live, &press(), 220.0, cursor_x, 300.0), Ownership::Own(None), "press at {cursor_x:?} during a drag must never reach the content" ); @@ -499,7 +530,7 @@ mod tests { let mut live = dragging(); let right_press = Event::Mouse(mouse::Event::ButtonPressed(mouse::Button::Right)); assert_eq!( - owns(&mut live, &right_press, Some(220.0), true, 220.0), + owns(&mut live, &right_press, 220.0, Some(220.0), 220.0), Ownership::Delegate ); assert!(live.drag.is_some()); @@ -511,12 +542,105 @@ mod tests { // disagree mid-frame; the press must anchor on the reported width. let mut state = State::default(); assert!(matches!( - owns(&mut state, &press(), Some(300.0), true, 300.0), + owns(&mut state, &press(), 300.0, Some(300.0), 300.0), Ownership::Own { .. } )); assert_eq!( - owns(&mut state, &moved(340.0), Some(340.0), false, 300.0), + owns(&mut state, &moved(340.0), 300.0, Some(340.0), 300.0), Ownership::Own(Some(GripEvent::Dragged { width: 340.0 })) ); } + + #[test] + fn a_press_is_hit_tested_where_the_last_move_landed_not_at_the_batch_cursor() { + // The batch reports 600 for both events — a frame-behind UI drains + // the press together with the move that followed it. + let mut state = State::default(); + assert_eq!( + owns(&mut state, &moved(221.0), 220.0, Some(600.0), 220.0), + Ownership::Delegate + ); + assert_eq!( + owns(&mut state, &press(), 220.0, Some(600.0), 220.0), + Ownership::Own(None) + ); + assert_eq!( + state.drag, + Some(Drag { + start_x: 221.0, + start_width: 220.0, + }) + ); + + // The inverse: a press whose honest position left the zone must not + // be rescued by a batch cursor that drifted back onto the seam. + let mut away = State::default(); + assert_eq!( + owns(&mut away, &moved(600.0), 220.0, Some(221.0), 220.0), + Ownership::Delegate + ); + assert_eq!( + owns(&mut away, &press(), 220.0, Some(221.0), 220.0), + Ownership::Delegate + ); + assert_eq!(away.drag, None); + } + + #[test] + fn a_press_before_any_move_falls_back_to_the_batch_cursor() { + let mut state = State::default(); + assert_eq!( + owns(&mut state, &press(), 220.0, Some(222.0), 220.0), + Ownership::Own(None) + ); + assert_eq!( + state.drag, + Some(Drag { + start_x: 222.0, + start_width: 220.0, + }) + ); + + let mut outside = State::default(); + assert_eq!( + owns(&mut outside, &press(), 220.0, Some(600.0), 220.0), + Ownership::Delegate + ); + assert_eq!(outside.drag, None); + } + + #[test] + fn a_pointer_that_left_the_window_drops_the_anchor() { + // Re-entering somewhere else and pressing before any move is observed + // must not be hit-tested where the pointer used to be. + let mut state = State::default(); + assert_eq!( + owns(&mut state, &moved(221.0), 220.0, Some(221.0), 220.0), + Ownership::Delegate + ); + assert_eq!( + owns(&mut state, &left(), 220.0, None, 220.0), + Ownership::Delegate + ); + assert_eq!(state.last_cursor, None); + assert_eq!( + owns(&mut state, &press(), 220.0, Some(600.0), 220.0), + Ownership::Delegate + ); + assert_eq!(state.drag, None); + } + + #[test] + fn a_press_with_no_batch_cursor_stays_a_no_op_however_fresh_the_anchor() { + let mut state = State::default(); + assert_eq!( + owns(&mut state, &moved(221.0), 220.0, Some(221.0), 220.0), + Ownership::Delegate + ); + assert_eq!( + owns(&mut state, &press(), 220.0, None, 220.0), + Ownership::Delegate + ); + assert_eq!(state.drag, None); + } } diff --git a/tools/input/linux/iced_clipboard_check.py b/tools/input/linux/iced_clipboard_check.py index dc38eaf5..648397eb 100755 --- a/tools/input/linux/iced_clipboard_check.py +++ b/tools/input/linux/iced_clipboard_check.py @@ -1605,15 +1605,6 @@ def width_within(expected: float) -> bool: ["mousemove", "--window", launch.window, str(x0), str(y)] ) launch.terminal_pointer(["mousedown", "1"]) - # `mouse::Event::ButtonPressed` carries no position, so iced hit-tests - # a press against the newest cursor position in the event batch it is - # drained with — not the position the button actually went down at. - # A UI running a frame behind (routine on a loaded CI box) drains the - # press together with the first drag move and evaluates it 8px away, - # outside the grip's 6px zone, so no drag ever starts and the wait - # below times out with the sidebar untouched. Let the press drain on - # its own before the pointer moves again. - time.sleep(0.5 * SCALE) # Separate XTEST submissions per sample, like the tab/project drags # above: a single batched xdotool motion can be coalesced past the # grip's move handling. From 2ae8a155eff6203d80e627182c5e5d918f9f93e8 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Tue, 4 Aug 2026 23:31:45 -0500 Subject: [PATCH 2/7] fix(gtk): widen the sidebar side of the paned grab zone asymmetrically MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit PR #251's replacement grab zone was symmetric ±2px — tight enough to stop the paned stealing column-0 terminal selections, but too narrow to grab reliably ("resizing seems to only work sometimes"). The two goals only conflict on the terminal side, so the zone is now asymmetric: 4px into the sidebar, 2px past the separator's terminal edge (unchanged, selection still wins). 4px, not the 6-10px the issue floated: the claim fires on press (capture phase), and the sidebar ScrolledWindow's overlay scrollbar sits flush against the seam with a hovering interactive column of ~14-16px (verified against the compiled Adwaita stylesheet) — wider bands steal proportionally more of it. 4px doubles the grabbable sidebar-side width while leaving ~12px of scrollbar reachable. The harness layout cannot scroll the project list, so scrollbar overlap is a production-only tradeoff, recorded in plan 014. The cage-tier check gains a sidebar-side range probe (sep-6..sep-3, mirroring the terminal side's drift-buffer scan): at least one offset must resize, then the seam is restored from a press guaranteed inside the new band (review finding: undoing from the original press offset missed the band after drift-shifted hits). Cursor review: probe-undo drift bug fixed; stale fn-doc comment updated; bounds math confirmed clean. Closes #252 (plan 014 C2) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- crates/roost-linux/src/app.rs | 18 ++++++++--- tools/input/linux/real_input_check.py | 44 +++++++++++++++++++++++---- 2 files changed, 51 insertions(+), 11 deletions(-) diff --git a/crates/roost-linux/src/app.rs b/crates/roost-linux/src/app.rs index 9018bc27..38ac7d65 100644 --- a/crates/roost-linux/src/app.rs +++ b/crates/roost-linux/src/app.rs @@ -5037,7 +5037,10 @@ impl App { } /// Replace `GtkPaned`'s built-in resize gestures with one whose hit - /// zone is the separator itself (±2px), no more. + /// zone is the separator itself, plus a small asymmetric grab margin: + /// 4px into the sidebar side (no selectable text competes there), 2px + /// into the terminal side (kept tight so column-0 text selection wins + /// — see #251). /// /// GTK's internal capture-phase drag gesture claims any press within /// `HANDLE_EXTRA_SIZE` (6px) of the separator's styled box — with the @@ -5079,13 +5082,18 @@ impl App { let start_pos = start_pos.clone(); move |g, x, _y| { let pos = paned.position(); - // Separator box: [position .. end-child x]. ±2px slop. + // Separator box: [position .. end-child x]. The grab zone is + // ASYMMETRIC: 4px into the sidebar side, 2px into the terminal + // side. Sidebar side is widened because nothing there competes + // for the gesture (no selectable text); the terminal side stays + // tight so a drag starting at column 0 still resolves to text + // selection, not a resize — the deliberate outcome of #251. let sep_end = paned .end_child() .and_then(|c| c.compute_bounds(&paned)) .map(|b| b.x() as i32) .unwrap_or(pos + 6); - if (x as i32) < pos - 2 || (x as i32) > sep_end + 2 { + if (x as i32) < pos - 4 || (x as i32) > sep_end + 2 { g.set_state(gtk4::EventSequenceState::Denied); return; } @@ -5101,8 +5109,8 @@ impl App { } }); // `drag-end` also fires for denied sequences (a press outside the - // ±2px grab zone above claims Denied, not Claimed, but GestureDrag - // still emits begin/end around it) — persisting the paned's + // asymmetric grab zone above claims Denied, not Claimed, but + // GestureDrag still emits begin/end around it) — persisting the paned's // unrelated position on those is benign only because the engine // setter no-ops when the width is unchanged; don't drop the guard // below thinking this makes it redundant. diff --git a/tools/input/linux/real_input_check.py b/tools/input/linux/real_input_check.py index 260a643c..69e733c3 100644 --- a/tools/input/linux/real_input_check.py +++ b/tools/input/linux/real_input_check.py @@ -505,7 +505,9 @@ def _check_left_edge_drag_selects(r, env_x, wait_tab_attached) -> None: the separator's styled box (~22px into the terminal), so selecting a line from its start silently resized the sidebar instead; `App::tighten_paned_grab_zone` replaces it with a separator-only hit - zone. Also proves the separator itself still resizes.""" + zone. Also proves the separator itself still resizes, from BOTH the + terminal side (tight, 2px) and the sidebar side (widened, 4px — #252, + a press a few px shy of the seam used to miss it).""" pid = r.create_project(name="edge-drag", cwd="/tmp") tab = r.open_tab(pid, cwd="/tmp") wait_tab_attached(r, tab) @@ -535,6 +537,7 @@ def _check_left_edge_drag_selects(r, env_x, wait_tab_attached) -> None: # The separator must still resize: probe the few px around the seam # (its exact screen x shifts with the CSD margin). + resized_at = None for probe in range(sb + 1, sb + 10): _drag(env_x, probe, y0, probe + 60, y0) now = int(r.window_metrics()["sidebar_width"]) @@ -545,11 +548,40 @@ def _check_left_edge_drag_selects(r, env_x, wait_tab_attached) -> None: restored = int(r.window_metrics()["sidebar_width"]) assert restored == sb, \ f"separator undo failed: sidebar width {restored}, expected {sb}" - print(f" left-edge drag selection OK " - f"(selected {len(text)} chars; separator resizes at " - f"x=+{probe - sb})") - return - raise AssertionError("separator drag never resized the sidebar") + resized_at = probe - sb + break + if resized_at is None: + raise AssertionError("separator drag never resized the sidebar") + + # #252: the grab zone is asymmetric — widened 4px into the sidebar side + # (no selectable text competes there), so a press a couple px shy of the + # seam still grabs instead of missing. Probe sep-6..sep-3, mirroring the + # terminal-side range probe above (a single offset would flake the same + # way against CSD margin drift). + sidebar_resized_at = None + for probe in range(sb - 6, sb - 2): + _drag(env_x, probe, y0, probe - 40, y0) + now = int(r.window_metrics()["sidebar_width"]) + if now != sb: + # Undo from a seam-relative point guaranteed inside the 4px band + # of the NEW seam (`now`), not `probe`'s original offset from the + # OLD seam (`sb`): a probe that only hit because of the overscan + # (sep-6/sep-5, outside the 4px band under aligned coords) would + # put `probe + (now - sb)` outside the band relative to `now` + # too, and the undo press itself would get Denied. + undo_x0 = now - 2 + _drag(env_x, undo_x0, y0, undo_x0 + (sb - now), y0) + restored = int(r.window_metrics()["sidebar_width"]) + assert restored == sb, \ + f"sidebar-side separator undo failed: sidebar width {restored}, expected {sb}" + sidebar_resized_at = sb - probe + break + assert sidebar_resized_at is not None, \ + "sidebar-side grab zone (sep-6..sep-3) never resized the sidebar (#252)" + + print(f" left-edge drag selection OK " + f"(selected {len(text)} chars; separator resizes at " + f"x=+{resized_at}, sidebar-side grab at x=-{sidebar_resized_at})") def _check_sidebar_reorder(r, rc, env_x, tmp: Path, roost) -> None: From ca187a97944b6a150ec5e54cd511e36614d30845 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Tue, 4 Aug 2026 23:41:30 -0500 Subject: [PATCH 3/7] feat(mac): report terminal_top + terminal_font_family in window_metrics MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Swift app now answers the two optional app.window_metrics fields both Rust UIs already report (#278 added them; the Mac omission was the parity gap). A UiBridge terminalMetrics() accessor mirrors sidebarMetrics(), delegating to a pure static function so the geometry contract is unit-testable headless: terminal_top is the active terminal view's offset from the content view's TOP (bounds.height - converted maxY — AppKit content views are unflipped, so origin.y alone would be the bottom offset), and the family is the NSFont actually in use (familyName over fontName), matching the resolved-family semantics on iced/GTK. Both fields omit cleanly when no terminal is mounted, per the optional wire contract. XCTest coverage pins the flip derivation against constructed layouts (including a regression guard proving origin.y-based math would fail) plus nil-tolerance; test_sidebar_resize.py now asserts the family is a non-empty string on every target and terminal_top is positive when present (GTK legitimately omits it). ipc.md updated. Cursor review: no findings (convert() flip handling verified empirically, accessor matches the codebase's active-tab pattern, nil-omission confirmed on the wire). Closes #287 (plan 014 C3) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- docs/reference/ipc.md | 19 +- mac/Sources/Roost/App.swift | 52 ++++++ mac/Sources/Roost/IPCHandlerImpl.swift | 24 ++- mac/Sources/Roost/RoostBackend.swift | 8 + mac/Tests/RoostTests/WindowMetricsTests.swift | 171 ++++++++++++++++++ tools/roosttest/test_sidebar_resize.py | 19 ++ 6 files changed, 285 insertions(+), 8 deletions(-) create mode 100644 mac/Tests/RoostTests/WindowMetricsTests.swift diff --git a/docs/reference/ipc.md b/docs/reference/ipc.md index 42a88348..ac50b254 100644 --- a/docs/reference/ipc.md +++ b/docs/reference/ipc.md @@ -558,15 +558,20 @@ Request: `{"params": {}}`. ```json {"window_width":1100.0,"window_height":700.0,"sidebar_width":220.0, - "sidebar_collapsed":false,"terminal_top":34.0} + "sidebar_collapsed":false,"terminal_top":34.0,"terminal_font_family":"Berkeley Mono"} ``` -`terminal_top` is optional for wire compatibility. Iced reports the exact -application-owned top edge of its terminal viewport; native adapters may omit -the field until they can expose equivalent trustworthy geometry. Consumers -that require exact Iced coordinates must reject a missing, non-finite, or -non-positive value instead of copying a chrome-height constant. This operation -is ungated and read-only. +`terminal_top` and `terminal_font_family` are optional for wire compatibility +(omitted, not `null`, when an adapter has nothing to report). Iced reports the +exact application-owned top edge of its terminal viewport (the chrome-band +height above it); the Mac UI reports the AppKit terminal view's top offset, +measured from the content view's top edge; GTK omits `terminal_top` until it +can expose equivalent trustworthy geometry. Consumers that require exact +coordinates must reject a missing, non-finite, or non-positive value instead +of copying a chrome-height constant. `terminal_font_family` is the resolved +family the live terminal is actually rendering with (post-fallback-chain, not +a config echo) and is reported by all three adapters. This operation is +ungated and read-only. ### `app.sidebar_dump` diff --git a/mac/Sources/Roost/App.swift b/mac/Sources/Roost/App.swift index da457949..2d5d6cf0 100644 --- a/mac/Sources/Roost/App.swift +++ b/mac/Sources/Roost/App.swift @@ -5915,6 +5915,58 @@ extension RoostApp: UiBridge { return session.terminalView.currentCursorShapeName() } + /// `app.window_metrics`'s optional `terminal_top` / + /// `terminal_font_family` — the active tab's terminal viewport top + /// offset + resolved font family (issue #287, one-op-set parity + /// with iced's `chrome::BAND_HEIGHT` + resolved-family reply and + /// GTK's family-only reply). `nil` when there's no live terminal + /// view (fresh launch before any tab is opened). + /// + /// Gathers the raw inputs (which need a live `RoostApp`/window, + /// so aren't constructible headlessly) and hands them to the pure + /// `Self.terminalMetrics(contentView:terminalView:font:)` below, + /// which carries the actual derivation and IS unit-testable + /// without a live window (`WindowMetricsTests.swift`). + func terminalMetrics() -> (top: CGFloat, fontFamily: String)? { + guard let pid = activeProjectID, + let session = activeSessionByProject[pid] + else { return nil } + return Self.terminalMetrics( + contentView: window?.contentView, + terminalView: session.terminalView, + font: session.terminalView.font + ) + } + + /// Pure derivation behind `terminalMetrics()`. `nil` whenever any + /// input is missing/unmounted — `contentView` (no window yet), + /// `terminalView` (no active session), or a `terminalView` not + /// currently mounted in the view hierarchy (`superview == nil`) — + /// so a caller can drive this directly with stand-in `NSView`s to + /// pin the nil-tolerance contract headlessly. + /// + /// AppKit content views are unflipped (y=0 at the bottom), so the + /// terminal's on-screen top edge is `contentView.bounds.height - + /// terminalFrameInContentSpace.maxY`, not `frame.origin.y`. + /// `terminalView.convert(_:to:)` walks the actual (super)view + /// chain — root → split → pane → terminalContainer → + /// terminalView in the real app — rather than assuming a fixed + /// nesting depth, so this stays correct if a flipped container is + /// ever inserted along that path. + static func terminalMetrics( + contentView: NSView?, + terminalView: NSView?, + font: NSFont? + ) -> (top: CGFloat, fontFamily: String)? { + guard let contentView, let terminalView, let font, + terminalView.superview != nil + else { return nil } + let frameInContent = terminalView.convert(terminalView.bounds, to: contentView) + let top = contentView.bounds.height - frameInContent.maxY + let family = font.familyName ?? font.fontName + return (top: top, fontFamily: family) + } + /// Map the live `PalettePanel` (if any) to a `PaletteSnapshot`. private func paletteSnapshot() -> PaletteSnapshot { guard let panel = palette else { return .closed } diff --git a/mac/Sources/Roost/IPCHandlerImpl.swift b/mac/Sources/Roost/IPCHandlerImpl.swift index a58caff5..ef2e0e3f 100644 --- a/mac/Sources/Roost/IPCHandlerImpl.swift +++ b/mac/Sources/Roost/IPCHandlerImpl.swift @@ -1004,11 +1004,20 @@ actor IPCHandlerImpl: IPCHandler { // break cross-platform width/height equivalence for callers // that drive both UIs through the same op. let content = window.contentRect(forFrameRect: window.frame) + // Optional, terminal-view-only geometry/font (issue #287): nil + // when there's no live terminal (fresh launch, no tabs) rather + // than a fabricated 0/empty-string value — the wire contract's + // `skip_serializing_if = "Option::is_none"` then omits the + // fields entirely, matching iced/GTK's absent-vs-present + // semantics rather than emitting `null`. + let terminal = ui.terminalMetrics() return IPCWindowMetricsResult( windowWidth: Double(content.width), windowHeight: Double(content.height), sidebarWidth: Double(metrics.width), - sidebarCollapsed: metrics.collapsed + sidebarCollapsed: metrics.collapsed, + terminalTop: terminal.map { Double($0.top) }, + terminalFontFamily: terminal?.fontFamily ) } @@ -1342,11 +1351,24 @@ private struct IPCWindowMetricsResult: Codable { let windowHeight: Double let sidebarWidth: Double let sidebarCollapsed: Bool + /// Optional wire fields (mirrors `WindowMetricsResult` in + /// `crates/roost-ipc/src/messages.rs`'s `#[serde(default, + /// skip_serializing_if = "Option::is_none")]`). `Optional`-typed + /// stored properties get `encodeIfPresent`/`decodeIfPresent` from + /// Swift's synthesized `Codable` conformance (no custom + /// `init(from:)`/`encode(to:)` needed here, matching + /// `IPCPaletteStateResult.frame` elsewhere in this file), so `nil` + /// omits the key on encode rather than emitting `null` — matching + /// the Rust `skip_serializing_if` behavior byte-for-byte. + let terminalTop: Double? + let terminalFontFamily: String? enum CodingKeys: String, CodingKey { case windowWidth = "window_width" case windowHeight = "window_height" case sidebarWidth = "sidebar_width" case sidebarCollapsed = "sidebar_collapsed" + case terminalTop = "terminal_top" + case terminalFontFamily = "terminal_font_family" } } diff --git a/mac/Sources/Roost/RoostBackend.swift b/mac/Sources/Roost/RoostBackend.swift index bac5caff..17761059 100644 --- a/mac/Sources/Roost/RoostBackend.swift +++ b/mac/Sources/Roost/RoostBackend.swift @@ -136,6 +136,14 @@ protocol UiBridge: AnyObject { /// canonical form — empty body and `"default"` both map to /// `"default"`). Used by the `app.cursor_shape` IPC op. func currentCursorShape() -> String + + /// Active tab's terminal viewport top offset (points, measured + /// down from the content view's top edge) + the resolved font + /// family in use — the optional `terminal_top` / + /// `terminal_font_family` fields on `app.window_metrics` (iced/GTK + /// parity, issue #287). `nil` when no terminal view is live (fresh + /// launch, no tabs). + func terminalMetrics() -> (top: CGFloat, fontFamily: String)? } /// Outcome from `UiBridge.expandTabSelectionAt` — mirrors diff --git a/mac/Tests/RoostTests/WindowMetricsTests.swift b/mac/Tests/RoostTests/WindowMetricsTests.swift new file mode 100644 index 00000000..70ea65c2 --- /dev/null +++ b/mac/Tests/RoostTests/WindowMetricsTests.swift @@ -0,0 +1,171 @@ +// WindowMetricsTests — pins the `app.window_metrics` `terminal_top` / +// `terminal_font_family` derivation (issue #287, one-op-set parity: iced +// reports the chrome-band height + resolved family, GTK reports family only +// with `terminal_top: None`, and Mac was reporting neither before this). +// +// `RoostApp.terminalMetrics(contentView:terminalView:font:)` is the pure +// lever `UiBridge.terminalMetrics()` calls through — it takes plain `NSView` +// stand-ins rather than a live `RoostApp`/window (which isn't constructible +// headlessly; there's no existing test that builds one), so the flip-aware +// math and the nil-tolerance contract are both directly testable. +// +// XCTest, not swift-testing: a swarm of fast value-checks in the +// swift-testing suite reliably SIGABRTs `swiftpm-testing-helper` under +// Xcode 26.x (see `ShellEscapeTests.swift`'s header for the same note). + +import AppKit +import Foundation +import XCTest + +@testable import Roost + +@MainActor +final class WindowMetricsTests: XCTestCase { + /// Mirrors the real app's nesting (window.contentView → split → pane → + /// terminalContainer → terminalView) closely enough to exercise + /// `convert(_:to:)` across more than one hop, without needing the real + /// private view types — a plain `NSView` stand-in is all the math + /// touches. + private func mountedTerminalStandIn( + contentHeight: CGFloat, + tabBarHeight: CGFloat, + terminalHeight: CGFloat, + terminalWidth: CGFloat = 800 + ) -> (contentView: NSView, terminalView: NSView) { + let contentView = NSView(frame: NSRect(x: 0, y: 0, width: 1100, height: contentHeight)) + + let pane = NSView(frame: contentView.bounds) + contentView.addSubview(pane) + + // terminalContainer: pinned below a tabBarHeight-tall band at the + // pane's top, flush to the pane's bottom — same shape as + // `terminalContainer`'s constraints in App.swift. + let container = NSView( + frame: NSRect(x: 0, y: 0, width: pane.bounds.width, height: terminalHeight) + ) + pane.addSubview(container) + + // terminalView fills its container exactly, matching the + // edge-pin constraints `selectTab` applies in App.swift. + let terminalView = NSView(frame: container.bounds) + terminalView.frame.size.width = terminalWidth + container.addSubview(terminalView) + + return (contentView, terminalView) + } + + // MARK: - Positive derivation + + /// Pinned regression for the flip derivation: a tab bar band of 32pt + /// (the Mac `tabBarHeight`) above a terminal filling the rest of a + /// 700pt-tall content view must report `terminal_top == 32`, not the + /// terminal view's raw (bottom-relative) `origin.y`. + func testTerminalTopIsTheOffsetFromTheTopNotOriginY() { + let tabBarHeight: CGFloat = 32 + let contentHeight: CGFloat = 700 + let (contentView, terminalView) = mountedTerminalStandIn( + contentHeight: contentHeight, + tabBarHeight: tabBarHeight, + terminalHeight: contentHeight - tabBarHeight + ) + + // Sanity: the stand-in's origin.y is NOT the expected top offset — + // proves the test would catch a regression back to raw frame math. + XCTAssertEqual(terminalView.frame.origin.y, 0) + + let metrics = RoostApp.terminalMetrics( + contentView: contentView, + terminalView: terminalView, + font: NSFont.systemFont(ofSize: 13) + ) + XCTAssertEqual(metrics?.top, tabBarHeight) + } + + /// A shorter/taller tab-bar band shifts the derived top by the same + /// amount — pins that this is a live measurement, not a hardcoded + /// constant. + func testTerminalTopTracksAnArbitraryBandHeight() { + let contentHeight: CGFloat = 500 + let bandHeight: CGFloat = 47 + let (contentView, terminalView) = mountedTerminalStandIn( + contentHeight: contentHeight, + tabBarHeight: bandHeight, + terminalHeight: contentHeight - bandHeight + ) + let metrics = RoostApp.terminalMetrics( + contentView: contentView, + terminalView: terminalView, + font: NSFont.systemFont(ofSize: 13) + ) + XCTAssertEqual(metrics?.top, bandHeight) + } + + /// Font family resolution: `familyName` wins when present. + func testFontFamilyPrefersFamilyNameOverFontName() { + let (contentView, terminalView) = mountedTerminalStandIn( + contentHeight: 700, tabBarHeight: 32, terminalHeight: 668 + ) + let font = NSFont.monospacedSystemFont(ofSize: 14, weight: .regular) + let metrics = RoostApp.terminalMetrics( + contentView: contentView, terminalView: terminalView, font: font + ) + XCTAssertEqual(metrics?.fontFamily, font.familyName) + XCTAssertFalse(metrics?.fontFamily.isEmpty ?? true) + } + + // MARK: - Nil tolerance + + /// No active terminal view (fresh app, no tabs) → nils, not zeros or + /// an empty-string family. + func testNoTerminalViewYieldsNil() { + let contentView = NSView(frame: NSRect(x: 0, y: 0, width: 1100, height: 700)) + let metrics = RoostApp.terminalMetrics( + contentView: contentView, + terminalView: nil, + font: NSFont.systemFont(ofSize: 13) + ) + XCTAssertNil(metrics) + } + + /// No content view (no window yet) → nils. + func testNoContentViewYieldsNil() { + let terminalView = NSView(frame: NSRect(x: 0, y: 0, width: 800, height: 668)) + let container = NSView() + container.addSubview(terminalView) + let metrics = RoostApp.terminalMetrics( + contentView: nil, + terminalView: terminalView, + font: NSFont.systemFont(ofSize: 13) + ) + XCTAssertNil(metrics) + } + + /// A terminal view that exists but isn't mounted (no superview) → + /// nils, matching the "session exists but its view was just torn + /// down/never attached" edge the guard exists for. + func testUnmountedTerminalViewYieldsNil() { + let contentView = NSView(frame: NSRect(x: 0, y: 0, width: 1100, height: 700)) + let terminalView = NSView(frame: NSRect(x: 0, y: 0, width: 800, height: 668)) + XCTAssertNil(terminalView.superview) + let metrics = RoostApp.terminalMetrics( + contentView: contentView, + terminalView: terminalView, + font: NSFont.systemFont(ofSize: 13) + ) + XCTAssertNil(metrics) + } + + /// No font (defensive — shouldn't happen in practice since + /// `TerminalView.font` is non-optional) → nils rather than crashing. + func testNoFontYieldsNil() { + let contentView = NSView(frame: NSRect(x: 0, y: 0, width: 1100, height: 700)) + let terminalView = NSView(frame: NSRect(x: 0, y: 0, width: 800, height: 668)) + contentView.addSubview(terminalView) + let metrics = RoostApp.terminalMetrics( + contentView: contentView, + terminalView: terminalView, + font: nil + ) + XCTAssertNil(metrics) + } +} diff --git a/tools/roosttest/test_sidebar_resize.py b/tools/roosttest/test_sidebar_resize.py index 856d987c..d254f620 100644 --- a/tools/roosttest/test_sidebar_resize.py +++ b/tools/roosttest/test_sidebar_resize.py @@ -239,6 +239,25 @@ def test_set_width_reflects_in_metrics_and_pty_cols(self, roost, project): a user: a wider sidebar MUST take columns away from the shell. """ _seed_baseline(roost) + + # One-op-set parity (issue #287): every UI answers + # `terminal_font_family` as a non-empty string on `app.window_metrics` + # (iced resolves the active family, GTK reports the family it fell + # back to, Mac now reports `TerminalView.font`'s family). `terminal_top` + # stays optional — GTK omits the key entirely — but when a UI does + # report it, it must be a real positive offset, not `0`/a placeholder. + metrics = roost.window_metrics() + family = metrics.get("terminal_font_family") + assert isinstance(family, str) and family, ( + f"terminal_font_family must be a non-empty string on every UI; " + f"got {family!r} (metrics {metrics})" + ) + top = metrics.get("terminal_top") + if top is not None: + assert isinstance(top, (int, float)) and not isinstance(top, bool) and top > 0, ( + f"terminal_top, when reported, must be > 0; got {top!r} (metrics {metrics})" + ) + tab = _live_tab(roost, project) baseline_cols = _settled_cols(roost, tab, BASELINE_WIDTH_PT) From 4db242af826dd75e24152b82e821788d46de9332 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Wed, 5 Aug 2026 00:04:41 -0500 Subject: [PATCH 4/7] fix(drop): converge control-char filtering and filter the Swift URL branch MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Security-adjacent closeout of the drop-path divergence: both languages now reject dropped paths AND dragged URLs carrying any of LF, VT, FF, CR, NEL, LS, PS, or ESC — Swift's Character.isNewline scalar classes plus ESC, expressed in Rust as an explicit 8-scalar const (Rust ends up strictly stricter on pathological grapheme clusters, which only ever rejects more). Swift's dragged-URL branch — previously completely unfiltered, letting escape bytes reach the PTY — now applies the same predicate as the path branch; a rejected URL is treated as absent and falls through to the plain-string fallback, which stays deliberately unfiltered (reject, don't strip: #280's bracketed-paste mitigations own that boundary). Rust resolve() gains the equivalent url branch (paths → url → text, mirroring Swift) for cross-UI parity; both Rust call sites pass None — no toolkit surfaces a distinct URL drop payload yet, so the branch is production-dead and its tests are its only exercise (by design). Every rejected class has cross-pinned twin vectors in both test suites, with individual VT/FF vectors on the Rust side (the classes it newly rejects). Adversarial review's one finding is pinned as a documenting test on both sides: a drag populating .URL and .string with the same control-bearing text reaches the PTY via the string arm — the accepted #282 baseline, made explicit rather than incidental. Closes #282 (plan 014 C4) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- crates/roost-iced/src/app/interactions.rs | 2 +- crates/roost-linux/src/terminal_view.rs | 6 +- crates/roost-ui-model/src/drop_content.rs | 203 +++++++++++++++++--- mac/Sources/Roost/TerminalView.swift | 16 +- mac/Tests/RoostTests/ShellEscapeTests.swift | 103 ++++++++++ 5 files changed, 294 insertions(+), 36 deletions(-) diff --git a/crates/roost-iced/src/app/interactions.rs b/crates/roost-iced/src/app/interactions.rs index c78779b4..f09416a2 100644 --- a/crates/roost-iced/src/app/interactions.rs +++ b/crates/roost-iced/src/app/interactions.rs @@ -1491,7 +1491,7 @@ fn dispatch_file_drop_batch( if !origin_live { return FileDropDisposition::ClosedOrigin; } - let Some(text) = roost_ui_model::drop_content::resolve(batch.paths, None) else { + let Some(text) = roost_ui_model::drop_content::resolve(batch.paths, None, None) else { return FileDropDisposition::Invalid; }; paste(&text); diff --git a/crates/roost-linux/src/terminal_view.rs b/crates/roost-linux/src/terminal_view.rs index 62e400b2..b95603ed 100644 --- a/crates/roost-linux/src/terminal_view.rs +++ b/crates/roost-linux/src/terminal_view.rs @@ -2522,14 +2522,14 @@ fn drop_value_to_text(value: &glib::Value) -> Option { .iter() .filter_map(|file| file.path()) .collect::>(); - return roost_ui_model::drop_content::resolve(&paths, None); + return roost_ui_model::drop_content::resolve(&paths, None, None); } if let Ok(file) = value.get::() { let paths = file.path().into_iter().collect::>(); - return roost_ui_model::drop_content::resolve(&paths, None); + return roost_ui_model::drop_content::resolve(&paths, None, None); } if let Ok(s) = value.get::() { - return roost_ui_model::drop_content::resolve(std::iter::empty::<&Path>(), Some(&s)); + return roost_ui_model::drop_content::resolve(std::iter::empty::<&Path>(), None, Some(&s)); } None } diff --git a/crates/roost-ui-model/src/drop_content.rs b/crates/roost-ui-model/src/drop_content.rs index b26540d5..09c153c4 100644 --- a/crates/roost-ui-model/src/drop_content.rs +++ b/crates/roost-ui-model/src/drop_content.rs @@ -9,20 +9,43 @@ use std::path::{Path, PathBuf}; use crate::shell_escape; -/// Resolve local file paths and an optional plain-text fallback into terminal -/// input. +/// Characters no dropped path or URL may legitimately carry — aligned with +/// Swift's `Character.isNewline` scalar classes plus ESC. Swift classifies +/// grapheme clusters where this classifies scalars, so Rust is strictly +/// stricter on pathological clusters; that asymmetry only ever rejects more. +const REJECTED_DROP_CONTROLS: [char; 8] = [ + '\n', // LF + '\u{0b}', // VT + '\u{0c}', // FF + '\r', // CR + '\u{0085}', // NEL + '\u{2028}', // LS + '\u{2029}', // PS + '\u{1b}', // ESC +]; + +/// Resolve local file paths, an optional dragged URL, and an optional +/// plain-text fallback into terminal input. /// /// Paths are de-duplicated by their raw platform representation before UTF-8 -/// conversion. Non-UTF-8 paths and paths bearing a control character no -/// filename may legitimately carry (`\n`, `\r`, ESC) are ignored rather than -/// repaired: a newline would split the join into bogus extra shell lines and an -/// ESC would smuggle a control sequence (e.g. a bracketed-paste marker) into -/// the PTY, while stripping either would silently turn the path into a -/// different filename. Rejecting keeps `shell_escape::escape` lossless. -/// If at least one safe path remains, paths take priority over `text` and are -/// newline-joined in first-seen order. Otherwise non-empty text is returned -/// verbatim. -pub fn resolve(paths: I, text: Option<&str>) -> Option +/// conversion. Non-UTF-8 paths, and paths or URLs bearing one of +/// [`REJECTED_DROP_CONTROLS`], are ignored rather than repaired: a line break +/// would split the join into bogus extra shell lines and an ESC would smuggle a +/// control sequence (e.g. a bracketed-paste marker) into the PTY, while +/// stripping either would silently turn the path into a different filename. +/// Rejecting keeps `shell_escape::escape` lossless. +/// +/// Priority is paths → `url` → `text`, mirroring the Mac's +/// `TerminalView.dropContentString`. Surviving paths are newline-joined in +/// first-seen order and a surviving URL is shell-escaped; `text` is returned +/// verbatim and deliberately unfiltered (it may be a command the user wants to +/// run — the bracketed-paste mitigations own that boundary). +/// +/// `url` exists for cross-UI parity with the Mac's dragged-URL branch. No Rust +/// toolkit surfaces a distinct URL drop payload yet, so both call sites pass +/// `None` and this branch is production-dead until one does — the tests are its +/// only exercise. +pub fn resolve(paths: I, url: Option<&str>, text: Option<&str>) -> Option where I: IntoIterator, P: AsRef, @@ -36,12 +59,15 @@ where return None; } let path = path.to_str()?; - (!path.contains(['\n', '\r', '\u{1b}'])).then(|| shell_escape::escape(path)) + (!path.contains(REJECTED_DROP_CONTROLS)).then(|| shell_escape::escape(path)) }) .collect::>(); if !escaped.is_empty() { return Some(escaped.join("\n")); } + if let Some(url) = url.filter(|url| !url.is_empty() && !url.contains(REJECTED_DROP_CONTROLS)) { + return Some(shell_escape::escape(url)); + } text.filter(|text| !text.is_empty()).map(str::to_string) } @@ -49,10 +75,14 @@ where mod tests { use super::*; + fn no_paths() -> std::iter::Empty<&'static Path> { + std::iter::empty::<&Path>() + } + #[test] fn single_file_is_escaped() { assert_eq!( - resolve(["/tmp/My File.png"], None), + resolve(["/tmp/My File.png"], None, None), Some("/tmp/My\\ File.png".to_string()) ); } @@ -60,7 +90,7 @@ mod tests { #[test] fn multiple_files_keep_first_seen_order_and_newline_join() { assert_eq!( - resolve(["/tmp/a b.png", "/tmp/c.png"], None), + resolve(["/tmp/a b.png", "/tmp/c.png"], None, None), Some("/tmp/a\\ b.png\n/tmp/c.png".to_string()) ); } @@ -68,16 +98,61 @@ mod tests { #[test] fn duplicate_raw_paths_are_collapsed() { assert_eq!( - resolve(["/tmp/shot.png", "/tmp/shot.png"], None), + resolve(["/tmp/shot.png", "/tmp/shot.png"], None, None), Some("/tmp/shot.png".to_string()) ); } + /// Shared with the Swift `testNewlineBearingPathIsDropped` vector. #[test] fn newline_and_carriage_return_paths_are_rejected() { - assert_eq!(resolve(["/tmp/ev\nil.png", "/tmp/ev\ril.png"], None), None); assert_eq!( - resolve(["/tmp/ev\nil.png", "/tmp/ok.png"], None), + resolve(["/tmp/ev\nil.png", "/tmp/ev\ril.png"], None, None), + None + ); + assert_eq!( + resolve(["/tmp/ev\nil.png", "/tmp/ok.png"], None, None), + Some("/tmp/ok.png".to_string()) + ); + } + + /// Shared with the Swift `testVerticalTabBearingPathIsDropped` vector. + #[test] + fn vertical_tab_bearing_paths_are_rejected() { + assert_eq!(resolve(["/tmp/ev\u{0b}il.png"], None, None), None); + assert_eq!( + resolve(["/tmp/ev\u{0b}il.png", "/tmp/ok.png"], None, None), + Some("/tmp/ok.png".to_string()) + ); + } + + /// Shared with the Swift `testFormFeedBearingPathIsDropped` vector. + #[test] + fn form_feed_bearing_paths_are_rejected() { + assert_eq!(resolve(["/tmp/ev\u{0c}il.png"], None, None), None); + assert_eq!( + resolve(["/tmp/ev\u{0c}il.png", "/tmp/ok.png"], None, None), + Some("/tmp/ok.png".to_string()) + ); + } + + /// Shared with the Swift `testUnicodeNewlineBearingPathIsDropped` vector. + #[test] + fn unicode_newline_bearing_paths_are_rejected() { + assert_eq!(resolve(["/tmp/ev\u{0085}il.png"], None, None), None); + assert_eq!(resolve(["/tmp/ev\u{2028}il.png"], None, None), None); + assert_eq!(resolve(["/tmp/ev\u{2029}il.png"], None, None), None); + assert_eq!( + resolve( + [ + "/tmp/ev\u{0085}il.png", + "/tmp/ev\u{2028}il.png", + "/tmp/ev\u{2029}il.png", + "/tmp/ok.png", + ], + None, + None + ), Some("/tmp/ok.png".to_string()) ); } @@ -85,9 +160,9 @@ mod tests { /// Shared with the Swift `testControlBearingPathIsDropped` vector. #[test] fn escape_bearing_paths_are_rejected() { - assert_eq!(resolve(["/tmp/ev\u{1b}[201~il.png"], None), None); + assert_eq!(resolve(["/tmp/ev\u{1b}[201~il.png"], None, None), None); assert_eq!( - resolve(["/tmp/ev\u{1b}[201~il.png", "/tmp/ok.png"], None), + resolve(["/tmp/ev\u{1b}[201~il.png", "/tmp/ok.png"], None, None), Some("/tmp/ok.png".to_string()) ); } @@ -99,21 +174,95 @@ mod tests { use std::os::unix::ffi::OsStringExt; let invalid = PathBuf::from(OsString::from_vec(b"/tmp/invalid-\xff".to_vec())); - assert_eq!(resolve([invalid], None), None); + assert_eq!(resolve([invalid], None, None), None); + } + + /// Shared with the Swift `testWebURLIsEscapedWhenNoFiles` vector. + #[test] + fn url_is_escaped_when_no_safe_path_remains() { + assert_eq!( + resolve( + no_paths(), + Some("https://example.com/a?b=c&d=e"), + Some("ignored") + ), + Some("https://example.com/a\\?b=c\\&d=e".to_string()) + ); + assert_eq!( + resolve( + ["/tmp/ev\nil.png"], + Some("https://example.com/x"), + Some("ignored") + ), + Some("https://example.com/x".to_string()) + ); + } + + #[test] + fn safe_paths_take_priority_over_url() { + assert_eq!( + resolve(["/tmp/a.png"], Some("https://example.com/x"), None), + Some("/tmp/a.png".to_string()) + ); + } + + /// Shared with the Swift `testControlBearingURLFallsThroughToString` + /// vector: a rejected URL is absent, not stripped, so the unfiltered text + /// fallback answers instead. + #[test] + fn control_bearing_url_falls_through_to_text() { + for control in REJECTED_DROP_CONTROLS { + let url = format!("https://example.com/{control}evil"); + assert_eq!( + resolve(no_paths(), Some(&url), Some("fallback")), + Some("fallback".to_string()), + "url bearing U+{:04X} should fall through", + control as u32 + ); + } + } + + /// Shared with the Swift + /// `testControlBearingURLAndStringYieldsRawString` vector. Documents the + /// accepted #282 baseline: when a drag carries the same control-bearing + /// text on both the URL and the text side, the rejected URL falls through + /// and the raw text reaches the PTY unchanged. That is the plain-text + /// boundary #280's bracketed-paste mitigations own — reject-don't-strip + /// means this arm must not launder the payload into an escaped URL either. + #[test] + fn control_bearing_url_and_text_yields_raw_text() { + let payload = "https://example.com/\u{1b}[201~evil"; + assert_eq!( + resolve(no_paths(), Some(payload), Some(payload)), + Some(payload.to_string()) + ); + } + + /// Shared with the Swift `testControlBearingURLWithoutStringIsNil` vector. + #[test] + fn control_bearing_url_without_text_is_none() { + assert_eq!( + resolve( + no_paths(), + Some("https://example.com/\u{1b}[201~evil"), + None + ), + None + ); } #[test] fn plain_text_is_verbatim_when_no_safe_file_remains() { assert_eq!( - resolve(std::iter::empty::<&Path>(), Some("git status && ls")), + resolve(no_paths(), None, Some("git status && ls")), Some("git status && ls".to_string()) ); assert_eq!( - resolve(std::iter::empty::<&Path>(), Some("line one\nline two")), + resolve(no_paths(), None, Some("line one\nline two")), Some("line one\nline two".to_string()) ); assert_eq!( - resolve(["/tmp/ev\nil.png"], Some("fallback")), + resolve(["/tmp/ev\nil.png"], None, Some("fallback")), Some("fallback".to_string()) ); } @@ -121,14 +270,14 @@ mod tests { #[test] fn safe_files_take_priority_over_text() { assert_eq!( - resolve(["/tmp/a.png"], Some("ignored")), + resolve(["/tmp/a.png"], None, Some("ignored")), Some("/tmp/a.png".to_string()) ); } #[test] fn empty_payload_is_none() { - assert_eq!(resolve(std::iter::empty::<&Path>(), None), None); - assert_eq!(resolve(std::iter::empty::<&Path>(), Some("")), None); + assert_eq!(resolve(no_paths(), None, None), None); + assert_eq!(resolve(no_paths(), Some(""), Some("")), None); } } diff --git a/mac/Sources/Roost/TerminalView.swift b/mac/Sources/Roost/TerminalView.swift index 9a1b5d2a..fb339a21 100644 --- a/mac/Sources/Roost/TerminalView.swift +++ b/mac/Sources/Roost/TerminalView.swift @@ -1778,9 +1778,12 @@ final class TerminalView: NSView { /// `.fileURL` and `.URL` — we must insert the path, not the `file://` URL), /// then a dragged web URL, then plain text. Paths and URLs are shell-escaped; /// plain text is not (it may be a command the user wants to run). Multiple - /// files are newline-joined. Returns nil for an empty payload so the caller - /// emits no stray `ESC[200~ESC[201~`. Factored out so it's unit-testable - /// without a synthesised `NSDraggingInfo`; mirrors `drop_text` on GTK. + /// files are newline-joined. A path or URL carrying a newline or an ESC is + /// treated as absent, so a rejected URL falls through to the plain-text + /// branch rather than being silently repaired. Returns nil for an empty + /// payload so the caller emits no stray `ESC[200~ESC[201~`. Factored out so + /// it's unit-testable without a synthesised `NSDraggingInfo`; mirrors + /// `drop_content::resolve`. static func dropContentString(fileURLs: [URL], url: String?, string: String?) -> String? { // De-duplicate by standardized path (Finder lists one file under several // URL-shaped entries) and drop any path carrying a newline or an ESC — a @@ -1790,15 +1793,18 @@ final class TerminalView: NSView { // stripping, so the escaped text always names the real file. Mirrors // `drop_content::resolve`. Such filenames are pathological; screenshots // never have them. + func isSafeDropContent(_ candidate: String) -> Bool { + !candidate.contains(where: { $0.isNewline || $0 == "\u{1b}" }) + } var seen = Set() let paths = fileURLs .map { $0.standardizedFileURL.path } - .filter { !$0.contains(where: { $0.isNewline || $0 == "\u{1b}" }) } + .filter(isSafeDropContent) .filter { seen.insert($0).inserted } if !paths.isEmpty { return paths.map { ShellEscape.escape($0) }.joined(separator: "\n") } - if let url, !url.isEmpty { + if let url, !url.isEmpty, isSafeDropContent(url) { return ShellEscape.escape(url) } if let string, !string.isEmpty { diff --git a/mac/Tests/RoostTests/ShellEscapeTests.swift b/mac/Tests/RoostTests/ShellEscapeTests.swift index 3dbbb543..5db932db 100644 --- a/mac/Tests/RoostTests/ShellEscapeTests.swift +++ b/mac/Tests/RoostTests/ShellEscapeTests.swift @@ -84,6 +84,7 @@ final class DropContentResolverTests: XCTestCase { ) } + /// Shared with the Rust `url_is_escaped_when_no_safe_path_remains` vector. func testWebURLIsEscapedWhenNoFiles() { // `?` and `&` are in the escape set; `:` `/` `.` `=` are not. XCTAssertEqual( @@ -94,6 +95,46 @@ final class DropContentResolverTests: XCTestCase { ) } + /// Shared with the Rust `control_bearing_url_falls_through_to_text` vector: + /// a rejected URL is absent, not stripped, so the deliberately unfiltered + /// string fallback answers instead. + func testControlBearingURLFallsThroughToString() { + for control in ["\n", "\u{0B}", "\u{0C}", "\r", "\u{85}", "\u{2028}", "\u{2029}", "\u{1B}"] { + XCTAssertEqual( + TerminalView.dropContentString( + fileURLs: [], url: "https://example.com/\(control)evil", string: "fallback" + ), + "fallback", + "url bearing \(control.unicodeScalars.map(\.value)) should fall through" + ) + } + } + + /// Shared with the Rust `control_bearing_url_and_text_yields_raw_text` + /// vector. Documents the accepted #282 baseline: a drag can populate both + /// `.URL` and `.string` with the same control-bearing text, and the + /// rejected URL then falls through to the deliberately unfiltered string + /// arm, so the raw text reaches the PTY. That plain-text boundary is owned + /// by #280's bracketed-paste mitigations — and because we reject rather + /// than strip, the URL arm must not launder the payload into an escaped + /// form here either. + func testControlBearingURLAndStringYieldsRawString() { + let payload = "https://example.com/\u{1B}[201~evil" + XCTAssertEqual( + TerminalView.dropContentString(fileURLs: [], url: payload, string: payload), + payload + ) + } + + /// Shared with the Rust `control_bearing_url_without_text_is_none` vector. + func testControlBearingURLWithoutStringIsNil() { + XCTAssertNil( + TerminalView.dropContentString( + fileURLs: [], url: "https://example.com/\u{1B}[201~evil", string: nil + ) + ) + } + func testPlainStringIsNotEscaped() { XCTAssertEqual( TerminalView.dropContentString(fileURLs: [], url: nil, string: "git status && ls"), @@ -110,6 +151,8 @@ final class DropContentResolverTests: XCTestCase { ) } + /// Shared with the Rust `newline_and_carriage_return_paths_are_rejected` + /// vector. func testNewlineBearingPathIsDropped() { // A lone pathological path → nil (no stray brackets). XCTAssertNil( @@ -122,6 +165,66 @@ final class DropContentResolverTests: XCTestCase { ), "/tmp/ok.png" ) + XCTAssertNil( + TerminalView.dropContentString(fileURLs: [fileURL("/tmp/ev\ril.png")], url: nil, string: nil) + ) + } + + /// Shared with the Rust `vertical_tab_bearing_paths_are_rejected` vector. + func testVerticalTabBearingPathIsDropped() { + XCTAssertNil( + TerminalView.dropContentString( + fileURLs: [fileURL("/tmp/ev\u{0B}il.png")], url: nil, string: nil + ) + ) + XCTAssertEqual( + TerminalView.dropContentString( + fileURLs: [fileURL("/tmp/ev\u{0B}il.png"), fileURL("/tmp/ok.png")], + url: nil, string: nil + ), + "/tmp/ok.png" + ) + } + + /// Shared with the Rust `form_feed_bearing_paths_are_rejected` vector. + func testFormFeedBearingPathIsDropped() { + XCTAssertNil( + TerminalView.dropContentString( + fileURLs: [fileURL("/tmp/ev\u{0C}il.png")], url: nil, string: nil + ) + ) + XCTAssertEqual( + TerminalView.dropContentString( + fileURLs: [fileURL("/tmp/ev\u{0C}il.png"), fileURL("/tmp/ok.png")], + url: nil, string: nil + ), + "/tmp/ok.png" + ) + } + + /// Shared with the Rust `unicode_newline_bearing_paths_are_rejected` + /// vector: NEL, LS and PS are `isNewline` scalars too. + func testUnicodeNewlineBearingPathIsDropped() { + for control in ["\u{85}", "\u{2028}", "\u{2029}"] { + XCTAssertNil( + TerminalView.dropContentString( + fileURLs: [fileURL("/tmp/ev\(control)il.png")], url: nil, string: nil + ), + "path bearing \(control.unicodeScalars.map(\.value)) should be dropped" + ) + } + XCTAssertEqual( + TerminalView.dropContentString( + fileURLs: [ + fileURL("/tmp/ev\u{85}il.png"), + fileURL("/tmp/ev\u{2028}il.png"), + fileURL("/tmp/ev\u{2029}il.png"), + fileURL("/tmp/ok.png"), + ], + url: nil, string: nil + ), + "/tmp/ok.png" + ) } /// Shared with the Rust `escape_bearing_paths_are_rejected` vector: an ESC From ad493b2a0b3e3703ab8a1e47aa6df78161f12820 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Wed, 5 Aug 2026 00:16:03 -0500 Subject: [PATCH 5/7] refactor(gtk): return the wire struct from ipc_window_metrics, tighten lint gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The last clippy type_complexity in roost-linux was ipc_window_metrics's six-field tuple return. Instead of a type alias over the same tuple, the function (and the shared WindowMetricsReply channel in roost-engine) now carries roost_ipc's WindowMetricsResult directly — the wire struct already names every field, and the dispatch arm's destructure-and- rebuild step disappears (encode(&result), mirroring SidebarDump). The iced handler builds the struct at its reply site for the same reason. With roost-linux clippy-clean, the gtk-build job's narrow denylist step (-A warnings -D disallowed_types -D disallowed_methods) becomes the same full -D warnings gate every other crate gets; Makefile mirrors. The GtkDnD (#236) and grab_focus (#234) guards lose nothing: both disallowed lints are warn-by-default, so -D warnings still enforces them, and clippy.toml keeps the full rationale as the source of truth. rust-lint's exclusion stays (no GTK toolchain there). Both panel reviewers independently confirmed this was the sole remaining warning, so the tightened gate is green from this commit on. Cursor CLI returned empty on review (flaking); self-review verified field mapping against the old positional order at all three construction sites, identical wire JSON via the shared struct, YAML validity, and make clippy end to end. Closes #283 (plan 014 C5) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- .github/workflows/ci.yml | 30 ++++++++++---------------- Makefile | 6 ++++-- clippy.toml | 10 +++++---- crates/roost-engine/src/ipc.rs | 23 ++++++-------------- crates/roost-iced/src/app.rs | 2 +- crates/roost-iced/src/app/servicing.rs | 16 +++++++------- crates/roost-linux/src/app.rs | 15 ++++++++----- 7 files changed, 46 insertions(+), 56 deletions(-) diff --git a/.github/workflows/ci.yml b/.github/workflows/ci.yml index 4f619332..a4bc316a 100644 --- a/.github/workflows/ci.yml +++ b/.github/workflows/ci.yml @@ -429,25 +429,17 @@ jobs: - name: cargo test -p roost-linux run: cargo test -p roost-linux - # Regression guard: tab + project reorder were converted from GtkDnD to - # GtkGestureDrag because the DnD drag-icon surface aborts the process on - # Wayland (gdksurface-wayland.c:348:frame_callback), and synthetic input - # can't reliably reproduce that in CI. clippy's disallowed-types (see - # clippy.toml) forbids the DnD transport types semantically — it resolves - # real references, so it ignores the comments that name those types and - # catches builder/glob-import forms a text grep would miss. Only - # disallowed_types is denied, so roost-linux's other (un-linted) clippy - # warnings don't fail the build; rust-lint excludes roost-linux because it - # needs the GTK toolchain that this job already has. - - name: No GtkDnD reorder + no raw grab_focus (clippy disallowed-types/methods) - # `-A warnings` first: roost-linux is NOT clippy-clean (that's exactly - # why rust-lint excludes it), and the toolchain action sets - # RUSTFLAGS=-D warnings — so without allowing the rest, every - # pre-existing clippy lint would error here. We deny ONLY the - # disallowed_types (GtkDnD #236) + disallowed_methods (raw grab_focus - # #234) guards (they survive the `-A warnings` as later, more-specific - # flags). - run: cargo clippy -p roost-linux --all-targets -- -A warnings -D clippy::disallowed_types -D clippy::disallowed_methods + # roost-linux is clippy-clean (issue #283 closed out the last + # type_complexity holdout), so this runs the same full gate as + # rust-lint / Iced's "Test and lint Iced" step below — no more + # `-A warnings` narrow denylist. disallowed_types (GtkDnD #236) and + # disallowed_methods (raw grab_focus #234) are warn-by-default clippy + # lints, so `-D warnings` still catches regressions on both; clippy.toml + # stays the source of truth for which types/methods are disallowed. + # rust-lint excludes roost-linux because it needs the GTK toolchain + # that only this job has. + - name: cargo clippy -p roost-linux + run: cargo clippy -p roost-linux --all-targets -- -D warnings # Iced walking skeleton: exact released Iced + libghostty-vt on both host # platforms, with the common IPC harness driving a real PTY-backed window. diff --git a/Makefile b/Makefile index d497ab8b..6507895a 100644 --- a/Makefile +++ b/Makefile @@ -152,9 +152,11 @@ fmt-check: ## Check formatting (what CI's rust-lint runs) clippy: ## Lint Rust at CI parity (warnings are errors) # `-D warnings` matches the `rust-lint` CI job. Without it `make check` # passed while CI failed, which is worse than no local gate at all. - # roost-linux is linted separately (it needs GTK), mirroring CI's split. + # roost-linux is linted separately (it needs GTK), mirroring CI's split; + # it's clippy-clean (issue #283) so it gets the same full `-D warnings` + # gate, not a narrow denylist. cargo clippy --workspace --exclude roost-linux --all-targets -- -D warnings - cargo clippy -p roost-linux --all-targets -- -A warnings -D clippy::disallowed_types -D clippy::disallowed_methods + cargo clippy -p roost-linux --all-targets -- -D warnings check-iced: fmt-check test-iced ## Iced formatting, lint, tests, and dependency boundaries cargo clippy -p roost-iced --all-targets -- -D warnings diff --git a/clippy.toml b/clippy.toml index 25ae86a0..50d50f13 100644 --- a/clippy.toml +++ b/clippy.toml @@ -4,8 +4,9 @@ # types so a reorder can't regress back onto that crash. clippy resolves real # type references, so this (unlike a text grep) ignores the comments that name # these types to explain their removal, and catches builder/glob-import forms. -# Enforced by `cargo clippy -p roost-linux -- -D clippy::disallowed_types` in the -# gtk-build CI job (rust-lint excludes roost-linux — it needs the GTK toolchain). +# Enforced by the full `cargo clippy -p roost-linux -- -D warnings` gate in the +# gtk-build CI job (disallowed-types is warn-by-default, so `-D warnings` catches +# it; rust-lint excludes roost-linux — it needs the GTK toolchain). disallowed-types = [ { path = "gtk4::DragSource", reason = "reorder must use GtkGestureDrag — GtkDnD's drag-icon surface aborts on Wayland (#236)" }, { path = "gtk4::DropTarget", reason = "reorder must use GtkGestureDrag — GtkDnD's drag-icon surface aborts on Wayland (#236)" }, @@ -16,8 +17,9 @@ disallowed-types = [ # widget (mid attach / tab switch) walks a dead focus chain and trips # `gtk_widget_get_parent: GTK_IS_WIDGET` — the #234 storm/crash. Sites that must # always focus (the palette / rename entries, where a *missed* focus is the bug) -# opt out with a local `#[allow]`. Enforced by `-D clippy::disallowed_methods` -# in the gtk-build CI job alongside the disallowed-types guard above. +# opt out with a local `#[allow]`. Enforced by the same full `-D warnings` gate +# in the gtk-build CI job (disallowed-methods is warn-by-default) alongside the +# disallowed-types guard above. disallowed-methods = [ { path = "gtk4::prelude::WidgetExt::grab_focus", reason = "use crate::focus::safe_grab_focus — a raw grab on an un-rooted widget walks a dead focus chain (#234)" }, ] diff --git a/crates/roost-engine/src/ipc.rs b/crates/roost-engine/src/ipc.rs index 4c011b61..336e09d8 100644 --- a/crates/roost-engine/src/ipc.rs +++ b/crates/roost-engine/src/ipc.rs @@ -55,16 +55,12 @@ pub struct DumpData { /// on success, an error message on failure. type ScreenshotReply = tokio::sync::oneshot::Sender, u32, u32), String>>; -/// Reply for a [`UiRequest::WindowMetrics`]: -/// `(window_width, window_height, sidebar_width, sidebar_collapsed, -/// terminal_top, terminal_font_family)` -/// in logical points. The `Result<_, String>` envelope shape matches -/// every sibling reply (so the shared `ui_call` helper works), but +/// Reply for a [`UiRequest::WindowMetrics`]: the window/sidebar/terminal +/// geometry in logical points. The `Result<_, String>` envelope shape +/// matches every sibling reply (so the shared `ui_call` helper works), but /// the UI side always answers `Ok` — UI adapter widget/state queries /// never fail. -type WindowMetricsReply = tokio::sync::oneshot::Sender< - Result<(f64, f64, f64, bool, Option, Option), String>, ->; +type WindowMetricsReply = tokio::sync::oneshot::Sender>; /// Reply for [`UiRequest::SidebarDump`]. Read-only: always answers /// `Ok`, matching `WindowMetricsReply`. @@ -679,18 +675,11 @@ async fn dispatch( } ops::WINDOW_METRICS => { let _p: WindowMetricsParams = decode(params)?; - let (w, h_, sw, collapsed, terminal_top, terminal_font_family) = h + let result = h .ui_call(|reply| UiRequest::WindowMetrics { reply }) .await? .map_err(|m| HandlerError::new("internal", m))?; - encode(&WindowMetricsResult { - window_width: w, - window_height: h_, - sidebar_width: sw, - sidebar_collapsed: collapsed, - terminal_top, - terminal_font_family, - }) + encode(&result) } ops::SIDEBAR_DUMP => { let _p: SidebarDumpParams = decode(params)?; diff --git a/crates/roost-iced/src/app.rs b/crates/roost-iced/src/app.rs index d9a78f39..cde2e54a 100644 --- a/crates/roost-iced/src/app.rs +++ b/crates/roost-iced/src/app.rs @@ -30,7 +30,7 @@ use roost_engine::{ use roost_ipc::agent; use roost_ipc::messages::{ PaletteItemView, PalettePresentResult, PaletteStateResult, Project, SidebarDumpAgentRow, - SidebarDumpProject, SidebarDumpResult, + SidebarDumpProject, SidebarDumpResult, WindowMetricsResult, }; use roost_ipc::paths::BundleProfile; use roost_ipc::IpcServer; diff --git a/crates/roost-iced/src/app/servicing.rs b/crates/roost-iced/src/app/servicing.rs index 341aa62a..7afbbae6 100644 --- a/crates/roost-iced/src/app/servicing.rs +++ b/crates/roost-iced/src/app/servicing.rs @@ -720,14 +720,14 @@ impl App { .resolve(self.typography.effective_family()) .name .to_string(); - let _ = reply.send(Ok(( - f64::from(self.window_size.width), - f64::from(self.window_size.height), - f64::from(self.effective_sidebar_width()), - collapsed, - Some(f64::from(chrome::BAND_HEIGHT)), - Some(resolved_family), - ))); + let _ = reply.send(Ok(WindowMetricsResult { + window_width: f64::from(self.window_size.width), + window_height: f64::from(self.window_size.height), + sidebar_width: f64::from(self.effective_sidebar_width()), + sidebar_collapsed: collapsed, + terminal_top: Some(f64::from(chrome::BAND_HEIGHT)), + terminal_font_family: Some(resolved_family), + })); } UiRequest::WindowResize { width, diff --git a/crates/roost-linux/src/app.rs b/crates/roost-linux/src/app.rs index 38ac7d65..1b2fea15 100644 --- a/crates/roost-linux/src/app.rs +++ b/crates/roost-linux/src/app.rs @@ -27,7 +27,7 @@ use libadwaita::{ApplicationWindow, TabView, WindowTitle}; use roost_ipc::agent::{self, AgentLifecycle, AgentTabState}; use roost_ipc::messages::{ PaletteItemView, PaletteStateResult, Project, SidebarDumpAgentRow, SidebarDumpProject, - SidebarDumpResult, Tab, + SidebarDumpResult, Tab, WindowMetricsResult, }; use tokio::runtime::Handle; @@ -5607,9 +5607,7 @@ impl App { /// sidebar that's the start child of the `gtk4::Paned` we built /// with `resize_start_child(false) + shrink_start_child(false)`, /// so it equals the paned position when visible. - fn ipc_window_metrics( - self: &Rc, - ) -> Result<(f64, f64, f64, bool, Option, Option), String> { + fn ipc_window_metrics(self: &Rc) -> Result { let w = self.window.width() as f64; let h = self.window.height() as f64; let collapsed = !self.sidebar_box.is_visible(); @@ -5621,7 +5619,14 @@ impl App { let families = self.installed_font_family_names(); let family = typography::resolve_family_name(self.typography.borrow().effective_family(), &families); - Ok((w, h, sw, collapsed, None, Some(family))) + Ok(WindowMetricsResult { + window_width: w, + window_height: h, + sidebar_width: sw, + sidebar_collapsed: collapsed, + terminal_top: None, + terminal_font_family: Some(family), + }) } /// `app.sidebar_dump` — read every project's *last-rendered* agent From 2b719959d0c075171e79613c49c34890de389fd4 Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Wed, 5 Aug 2026 00:24:50 -0500 Subject: [PATCH 6/7] test(gtk): unit-pin the asymmetric grab zone instead of screen-probing it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The shed cage run proved C2's sidebar-side probe wrong by design: the separator's screen x carries an unknown CSD margin (the reason the terminal-side scan probes a range), so a fixed sb-relative range misses the 4px sidebar-side band for most margin values — a screen-coordinate probe cannot discriminate the widening. The probe is removed; the claim bounds are extracted into a pure paned_claims_press(x, pos, sep_end) and unit-pinned at all four boundary points (claims pos-4 / sep_end+2, denies pos-5 / sep_end+3), which fails deterministically on any revert to a symmetric zone. The cage tier keeps its drift-tolerant checks: seam resizes, left-edge drag still selects. Refs #252 (plan 014 C2 follow-up; shed verification finding) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- crates/roost-linux/src/app.rs | 53 ++++++++++++++++++++++----- tools/input/linux/real_input_check.py | 50 ++++++------------------- 2 files changed, 55 insertions(+), 48 deletions(-) diff --git a/crates/roost-linux/src/app.rs b/crates/roost-linux/src/app.rs index 1b2fea15..73e75c66 100644 --- a/crates/roost-linux/src/app.rs +++ b/crates/roost-linux/src/app.rs @@ -5082,18 +5082,14 @@ impl App { let start_pos = start_pos.clone(); move |g, x, _y| { let pos = paned.position(); - // Separator box: [position .. end-child x]. The grab zone is - // ASYMMETRIC: 4px into the sidebar side, 2px into the terminal - // side. Sidebar side is widened because nothing there competes - // for the gesture (no selectable text); the terminal side stays - // tight so a drag starting at column 0 still resolves to text - // selection, not a resize — the deliberate outcome of #251. + // Separator box: [position .. end-child x]. See + // `paned_claims_press` for the (asymmetric) grab zone. let sep_end = paned .end_child() .and_then(|c| c.compute_bounds(&paned)) .map(|b| b.x() as i32) .unwrap_or(pos + 6); - if (x as i32) < pos - 4 || (x as i32) > sep_end + 2 { + if !paned_claims_press(x as i32, pos, sep_end) { g.set_state(gtk4::EventSequenceState::Denied); return; } @@ -6689,18 +6685,55 @@ fn agent_rows_visible(toggle_on: bool, dragging: bool) -> bool { toggle_on && !dragging } +/// Whether `App::tighten_paned_grab_zone`'s resize gesture claims a press at +/// screen x `x`, given the separator box `[pos .. sep_end]`. The zone is +/// ASYMMETRIC (#252): 4px into the sidebar side (nothing there competes for +/// the gesture — no selectable text), 2px into the terminal side (kept tight +/// so a drag starting at column 0 still resolves to text selection, not a +/// resize — the deliberate outcome of #251). Pulled out as a pure function +/// so the asymmetry is unit-pinned here rather than only reachable through a +/// real GTK press — a screen-coordinate input-injection probe can't +/// discriminate the widening under the separator's unknown CSD margin. +fn paned_claims_press(x: i32, pos: i32, sep_end: i32) -> bool { + x >= pos - 4 && x <= sep_end + 2 +} + #[cfg(test)] mod tests { use super::{ activation_target, agent_rows_visible, drain_server_driven_marker, - is_already_attached_or_pending, pick_next_active_project, resolve_launch_cwd, - restore_open_specs, reveal_scroll_value, tilde_abbreviate_with_home, ActivationTarget, - RestoreTab, + is_already_attached_or_pending, paned_claims_press, pick_next_active_project, + resolve_launch_cwd, restore_open_specs, reveal_scroll_value, tilde_abbreviate_with_home, + ActivationTarget, RestoreTab, }; use roost_ui_model::reorder::compute_insert_idx; use std::cell::RefCell; use std::collections::{HashMap, HashSet}; + #[test] + fn paned_claims_press_asymmetric_bounds() { + // Separator box [pos=100 .. sep_end=106]. Sidebar side widened 4px, + // terminal side kept tight at 2px (#252 / #251) — pin BOTH edges on + // BOTH sides so a revert or a symmetrizing "fix" fails this test. + let (pos, sep_end) = (100, 106); + assert!( + paned_claims_press(pos - 4, pos, sep_end), + "sidebar-side edge (pos-4) must claim" + ); + assert!( + !paned_claims_press(pos - 5, pos, sep_end), + "one px past the sidebar-side edge (pos-5) must deny" + ); + assert!( + paned_claims_press(sep_end + 2, pos, sep_end), + "terminal-side edge (sep_end+2) must claim" + ); + assert!( + !paned_claims_press(sep_end + 3, pos, sep_end), + "one px past the terminal-side edge (sep_end+3) must deny" + ); + } + #[test] fn agent_rows_stay_hidden_while_a_reorder_drag_is_armed() { assert!(agent_rows_visible(true, false)); diff --git a/tools/input/linux/real_input_check.py b/tools/input/linux/real_input_check.py index 69e733c3..9ca2b936 100644 --- a/tools/input/linux/real_input_check.py +++ b/tools/input/linux/real_input_check.py @@ -505,9 +505,13 @@ def _check_left_edge_drag_selects(r, env_x, wait_tab_attached) -> None: the separator's styled box (~22px into the terminal), so selecting a line from its start silently resized the sidebar instead; `App::tighten_paned_grab_zone` replaces it with a separator-only hit - zone. Also proves the separator itself still resizes, from BOTH the - terminal side (tight, 2px) and the sidebar side (widened, 4px — #252, - a press a few px shy of the seam used to miss it).""" + zone. Also proves the separator itself still resizes. #252 widened the + zone 4px into the sidebar side (no selectable text competes there); + that asymmetry is unit-pinned in app.rs (`paned_claims_press` tests) + rather than probed here — the separator's screen x carries an unknown + CSD margin (that's why the scan below covers sb+1..sb+9 rather than a + single offset), so a fixed sidebar-side offset range can't reliably + land inside a 4px band under that margin.""" pid = r.create_project(name="edge-drag", cwd="/tmp") tab = r.open_tab(pid, cwd="/tmp") wait_tab_attached(r, tab) @@ -537,7 +541,6 @@ def _check_left_edge_drag_selects(r, env_x, wait_tab_attached) -> None: # The separator must still resize: probe the few px around the seam # (its exact screen x shifts with the CSD margin). - resized_at = None for probe in range(sb + 1, sb + 10): _drag(env_x, probe, y0, probe + 60, y0) now = int(r.window_metrics()["sidebar_width"]) @@ -548,40 +551,11 @@ def _check_left_edge_drag_selects(r, env_x, wait_tab_attached) -> None: restored = int(r.window_metrics()["sidebar_width"]) assert restored == sb, \ f"separator undo failed: sidebar width {restored}, expected {sb}" - resized_at = probe - sb - break - if resized_at is None: - raise AssertionError("separator drag never resized the sidebar") - - # #252: the grab zone is asymmetric — widened 4px into the sidebar side - # (no selectable text competes there), so a press a couple px shy of the - # seam still grabs instead of missing. Probe sep-6..sep-3, mirroring the - # terminal-side range probe above (a single offset would flake the same - # way against CSD margin drift). - sidebar_resized_at = None - for probe in range(sb - 6, sb - 2): - _drag(env_x, probe, y0, probe - 40, y0) - now = int(r.window_metrics()["sidebar_width"]) - if now != sb: - # Undo from a seam-relative point guaranteed inside the 4px band - # of the NEW seam (`now`), not `probe`'s original offset from the - # OLD seam (`sb`): a probe that only hit because of the overscan - # (sep-6/sep-5, outside the 4px band under aligned coords) would - # put `probe + (now - sb)` outside the band relative to `now` - # too, and the undo press itself would get Denied. - undo_x0 = now - 2 - _drag(env_x, undo_x0, y0, undo_x0 + (sb - now), y0) - restored = int(r.window_metrics()["sidebar_width"]) - assert restored == sb, \ - f"sidebar-side separator undo failed: sidebar width {restored}, expected {sb}" - sidebar_resized_at = sb - probe - break - assert sidebar_resized_at is not None, \ - "sidebar-side grab zone (sep-6..sep-3) never resized the sidebar (#252)" - - print(f" left-edge drag selection OK " - f"(selected {len(text)} chars; separator resizes at " - f"x=+{resized_at}, sidebar-side grab at x=-{sidebar_resized_at})") + print(f" left-edge drag selection OK " + f"(selected {len(text)} chars; separator resizes at " + f"x=+{probe - sb})") + return + raise AssertionError("separator drag never resized the sidebar") def _check_sidebar_reorder(r, rc, env_x, tmp: Path, roost) -> None: From 5e17b621e35097d96a32545248e096d2ddc61fef Mon Sep 17 00:00:00 2001 From: Charlie Knudsen Date: Wed, 5 Aug 2026 00:52:10 -0500 Subject: [PATCH 7/7] fix(review): tighten parity asserts + comment/doc accuracy (PR #301 review) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Compensating-review findings applied (the PR's CodeRabbit check was rate-limited): the terminal_top e2e assert now distinguishes an omitted key from an explicit null (the wire contract is omit-not-null, and serde emits null for non-finite f64s — exactly the arm .get() flattened away) and requires a finite value; the parity asserts moved after _live_tab so the Mac fields are read with a terminal mounted instead of racing the async first-tab spawn; ipc.md no longer overstates terminal_font_family as unconditional on Mac; the grab-zone doc names the overlay-scrollbar tradeoff the 4px bound exists for; the grip comment records the accepted overlay-staleness window; clippy.toml quotes the gate verbatim. Dispositions: scrollbar overlap remains the plan-recorded accepted tradeoff (now stated at the code site too); XCTest-vs-swift-testing findings declined per the documented Xcode 26 SIGABRT constraint. Refs #287 #252 #295 (plan 014, post-review) Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01WLKWsLV45DAk61xG6Utj3e --- clippy.toml | 3 ++- crates/roost-iced/src/sidebar_resize.rs | 5 +++- crates/roost-linux/src/app.rs | 8 +++--- docs/reference/ipc.md | 5 ++-- tools/roosttest/test_sidebar_resize.py | 34 ++++++++++++++++--------- 5 files changed, 36 insertions(+), 19 deletions(-) diff --git a/clippy.toml b/clippy.toml index 50d50f13..ea7884f7 100644 --- a/clippy.toml +++ b/clippy.toml @@ -4,7 +4,8 @@ # types so a reorder can't regress back onto that crash. clippy resolves real # type references, so this (unlike a text grep) ignores the comments that name # these types to explain their removal, and catches builder/glob-import forms. -# Enforced by the full `cargo clippy -p roost-linux -- -D warnings` gate in the +# Enforced by the full `cargo clippy -p roost-linux --all-targets -- -D +# warnings` gate in the # gtk-build CI job (disallowed-types is warn-by-default, so `-D warnings` catches # it; rust-lint excludes roost-linux — it needs the GTK toolchain). disallowed-types = [ diff --git a/crates/roost-iced/src/sidebar_resize.rs b/crates/roost-iced/src/sidebar_resize.rs index e819b232..b05661bc 100644 --- a/crates/roost-iced/src/sidebar_resize.rs +++ b/crates/roost-iced/src/sidebar_resize.rs @@ -101,7 +101,10 @@ fn owns_event( // travelled *after* the button went down, so the last move the grip // actually saw is the honest press anchor (issue #295). A pointer that // left the window invalidates it — the next entry can land anywhere, and - // no move need be observed before the press. + // no move need be observed before the press. Moves consumed by an iced + // overlay never reach the grip and fire no CursorLeft, so the anchor can + // sit stale across an overlay session; accepted — the first post-overlay + // move refreshes it, and a press before that is a narrow window. match event { Event::Mouse(mouse::Event::CursorMoved { position, .. }) => { state.last_cursor = Some(*position); diff --git a/crates/roost-linux/src/app.rs b/crates/roost-linux/src/app.rs index 73e75c66..80df539b 100644 --- a/crates/roost-linux/src/app.rs +++ b/crates/roost-linux/src/app.rs @@ -5038,9 +5038,11 @@ impl App { /// Replace `GtkPaned`'s built-in resize gestures with one whose hit /// zone is the separator itself, plus a small asymmetric grab margin: - /// 4px into the sidebar side (no selectable text competes there), 2px - /// into the terminal side (kept tight so column-0 text selection wins - /// — see #251). + /// 4px into the sidebar side, 2px into the terminal side (kept tight + /// so column-0 text selection wins — see #251). The sidebar side has + /// no selectable text, but its overlay scrollbar hugs the seam — the + /// 4px (not the 6-10px #252 floated) bounds how much of that + /// scrollbar's ~16px hover column a seam press can steal. /// /// GTK's internal capture-phase drag gesture claims any press within /// `HANDLE_EXTRA_SIZE` (6px) of the separator's styled box — with the diff --git a/docs/reference/ipc.md b/docs/reference/ipc.md index ac50b254..70b24cac 100644 --- a/docs/reference/ipc.md +++ b/docs/reference/ipc.md @@ -570,8 +570,9 @@ can expose equivalent trustworthy geometry. Consumers that require exact coordinates must reject a missing, non-finite, or non-positive value instead of copying a chrome-height constant. `terminal_font_family` is the resolved family the live terminal is actually rendering with (post-fallback-chain, not -a config echo) and is reported by all three adapters. This operation is -ungated and read-only. +a config echo) and is reported by all three adapters once a terminal is live +— the Mac adapter omits both fields until a terminal view is mounted (fresh +launch, no tabs). This operation is ungated and read-only. ### `app.sidebar_dump` diff --git a/tools/roosttest/test_sidebar_resize.py b/tools/roosttest/test_sidebar_resize.py index d254f620..4e968fc6 100644 --- a/tools/roosttest/test_sidebar_resize.py +++ b/tools/roosttest/test_sidebar_resize.py @@ -29,6 +29,7 @@ from __future__ import annotations +import math import os import pytest @@ -240,25 +241,34 @@ def test_set_width_reflects_in_metrics_and_pty_cols(self, roost, project): """ _seed_baseline(roost) - # One-op-set parity (issue #287): every UI answers - # `terminal_font_family` as a non-empty string on `app.window_metrics` - # (iced resolves the active family, GTK reports the family it fell - # back to, Mac now reports `TerminalView.font`'s family). `terminal_top` - # stays optional — GTK omits the key entirely — but when a UI does - # report it, it must be a real positive offset, not `0`/a placeholder. + tab = _live_tab(roost, project) + + # One-op-set parity (issue #287): with a terminal live and mounted + # (guaranteed by `_live_tab` above — Mac omits the fields until one + # is), every UI answers `terminal_font_family` as a non-empty string + # on `app.window_metrics` (iced resolves the active family, GTK + # reports the family it fell back to, Mac reports + # `TerminalView.font`'s family). `terminal_top` stays optional — + # GTK omits the KEY entirely; an explicit `null` (also what serde + # emits for a non-finite f64) is a contract violation, so the + # omitted-vs-null distinction is asserted, not `.get()`-flattened. metrics = roost.window_metrics() family = metrics.get("terminal_font_family") assert isinstance(family, str) and family, ( f"terminal_font_family must be a non-empty string on every UI; " f"got {family!r} (metrics {metrics})" ) - top = metrics.get("terminal_top") - if top is not None: - assert isinstance(top, (int, float)) and not isinstance(top, bool) and top > 0, ( - f"terminal_top, when reported, must be > 0; got {top!r} (metrics {metrics})" + if "terminal_top" in metrics: + top = metrics["terminal_top"] + assert ( + isinstance(top, (int, float)) + and not isinstance(top, bool) + and math.isfinite(top) + and top > 0 + ), ( + f"terminal_top, when reported, must be a finite positive " + f"number; got {top!r} (metrics {metrics})" ) - - tab = _live_tab(roost, project) baseline_cols = _settled_cols(roost, tab, BASELINE_WIDTH_PT) roost.sidebar_set_width(WIDER_WIDTH_PT)