Skip to content

Commit 3ae2ef8

Browse files
charliekclaude
andcommitted
fix: let libghostty own OSC color-query replies, drop the engine's own
Upstream 14c829883 (in the new pin) answers OSC 4/10/11/12 and Kitty 21 color queries through the write_pty effect whenever the callback is installed. Roost's drain-side synthesis then became a second reply on the wire, failing test_osc_pipeline on every e2e lane. Remove the emission on both sides (engine map_events query arms + OscAction:: PtyInput and its consumers; the Swift colorQuery/paletteQuery arms and their formatters) — OscColorState keeps tracking colors for rendering; only reply synthesis moves to libghostty. Behavior change: same-chunk SET then QUERY now replies once with the just-set value (sequential, xterm-style) instead of the pre-chunk value the drain pipeline used to synthesize; OSC 4 and Kitty 21 queries are now answered too. Coverage repinned at the source of truth: +6 byte-exact reply tests in write_pty_test.rs (incl. terminator preservation and the unseeded-terminal no-reply pin), drain tests now assert queries enqueue nothing, e2e asserts exactly one reply. Local ROOST_TEST_MODE=1 make e2e-iced: 131 passed (was red). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01VVNzooL9ww3TgdMo1UABzb
1 parent dc77410 commit 3ae2ef8

13 files changed

Lines changed: 663 additions & 1119 deletions

File tree

crates/roost-engine/src/osc.rs

Lines changed: 123 additions & 204 deletions
Large diffs are not rendered by default.

crates/roost-engine/src/session.rs

Lines changed: 39 additions & 69 deletions
Original file line numberDiff line numberDiff line change
@@ -41,9 +41,7 @@ pub enum TabOutput {
4141
/// PTY emitted bytes and this session's drain already scanned them
4242
/// (the `attach_scanned` opt-in). The bytes still route into
4343
/// `Terminal::vt_write` unchanged; `actions` carries what the scan
44-
/// produced MINUS the query replies, which the drain has already
45-
/// enqueued onto the PTY input channel. The UI must not run a
46-
/// second router over these bytes.
44+
/// produced. The UI must not run a second router over these bytes.
4745
Scanned {
4846
data: Vec<u8>,
4947
actions: Vec<OscAction>,
@@ -82,12 +80,11 @@ pub struct TabSession {
8280
pub tab_id: i64,
8381
cmd_tx: tokio::sync::mpsc::UnboundedSender<PtyCommand>,
8482
/// Test-mode tap: when set, every payload enqueued onto the serial
85-
/// channel — from `send_input` OR from the drain's own OSC replies
86-
/// — is appended here first. `None` in production —
83+
/// channel is appended here first. `None` in production —
8784
/// allocated by `App` when `ROOST_TEST_MODE=1` so the
8885
/// `tab.capture_pty_input` IPC op can observe keystrokes,
89-
/// paste, and synthesised OSC replies that flow out to the
90-
/// PTY. The tap is upstream of the command queue, so it
86+
/// paste, and the terminal's own query replies that flow out to
87+
/// the PTY. The tap is upstream of the command queue, so it
9188
/// captures the bytes whether or not the supervisor write
9289
/// later succeeds — exactly what a test wants to assert on.
9390
input_capture: Option<InputCapture>,
@@ -127,42 +124,24 @@ fn enqueue_input(
127124

128125
/// Take the drain lock, recovering from poisoning: a poisoned mutex
129126
/// means a prior panic somewhere else in the process, and a tab that
130-
/// stops answering color queries for the rest of the session is a worse
127+
/// stops tracking its colors for the rest of the session is a worse
131128
/// outcome than continuing with the state we have.
132129
fn lock_osc(osc: &Mutex<OscDrain>) -> std::sync::MutexGuard<'_, OscDrain> {
133130
osc.lock().unwrap_or_else(|poisoned| poisoned.into_inner())
134131
}
135132

136-
/// Scan one chunk with a tab's drain-side router, then split what it
137-
/// produced: query replies go straight onto the PTY input channel,
138-
/// everything else is returned to travel to the UI with the bytes.
133+
/// Scan one chunk with a tab's drain-side router, moving the tab's
134+
/// color state and returning the actions that travel to the UI with the
135+
/// bytes.
139136
///
140-
/// The scan AND its replies happen under one hold of the drain lock.
141-
/// Releasing it in between would let a second scanner (the PTY drain
142-
/// and a test-mode `scan_osc` are separate threads) interleave its
143-
/// enqueue between this scan and this enqueue, putting replies on the
144-
/// channel in the opposite order to the bytes that asked for them.
145-
///
146-
/// Lock order is `OscDrain` → `InputCapture`, and only here: every
147-
/// other capture holder (`enqueue_input`, and each UI's
148-
/// `tab.capture_pty_input` reader) takes the capture alone, so no site
149-
/// can invert it.
150-
fn scan_and_reply(
151-
osc: &Mutex<OscDrain>,
152-
cmd_tx: &UnboundedSender<PtyCommand>,
153-
capture: Option<&InputCapture>,
154-
bytes: &[u8],
155-
) -> Vec<OscAction> {
137+
/// The scan holds the drain lock for its whole duration: the PTY drain
138+
/// and a test-mode `scan_osc` are separate threads feeding one streaming
139+
/// scanner and one color state, and interleaving them would corrupt
140+
/// both.
141+
fn scan(osc: &Mutex<OscDrain>, bytes: &[u8]) -> Vec<OscAction> {
156142
let mut drain = lock_osc(osc);
157143
let OscDrain { router, colors } = &mut *drain;
158-
let mut forwarded = Vec::new();
159-
for action in router.feed_stateful(bytes, colors) {
160-
match action {
161-
OscAction::PtyInput(reply) => enqueue_input(cmd_tx, capture, reply),
162-
other => forwarded.push(other),
163-
}
164-
}
165-
forwarded
144+
router.feed_stateful(bytes, colors)
166145
}
167146

168147
impl TabSession {
@@ -192,16 +171,14 @@ impl TabSession {
192171
///
193172
/// With `osc_seed` set, this session's forwarding task owns the
194173
/// SOLE `OscRouter` for the tab: it scans each PTY chunk as it
195-
/// arrives, enqueues color-query replies onto the same serial input
196-
/// channel keystrokes use — before the bytes have even reached the
197-
/// UI — and forwards the chunk as [`TabOutput::Scanned`] with the
198-
/// remaining actions. That is the whole point of the opt-in: a
199-
/// program that queries and exits (Go termenv's 1-frame probe) gets
200-
/// its answer off the drain instead of one event-loop turn later,
201-
/// which is where the reply used to leak into the shell prompt.
174+
/// arrives — moving the tab's color state before the bytes have
175+
/// even reached the UI — and forwards the chunk as
176+
/// [`TabOutput::Scanned`] with the actions the scan produced. The
177+
/// UI must not run a second router over those bytes.
202178
///
203179
/// `osc_seed` is the theme the tab launched with; see
204-
/// [`OscColorState`] for why the drain tracks colors itself.
180+
/// [`OscColorState`] for what the drain tracks and why it tracks it
181+
/// itself rather than reading the terminal.
205182
pub fn attach_with_receiver_scanned(
206183
supervisor: Arc<PtySupervisor>,
207184
tab_id: i64,
@@ -210,28 +187,23 @@ impl TabSession {
210187
input_capture: Option<InputCapture>,
211188
osc_seed: Option<OscColorSnapshot>,
212189
) -> Self {
213-
// The serial channel is created first: the forwarding task
214-
// needs its sender to enqueue replies from the drain.
215190
let (cmd_tx, mut cmd_rx) = tokio::sync::mpsc::unbounded_channel::<PtyCommand>();
216191
let osc = osc_seed.map(|seed| {
217192
Arc::new(Mutex::new(OscDrain {
218193
router: OscRouter::new(),
219194
colors: OscColorState::new(seed),
220195
}))
221196
});
222-
// Everything the scan needs travels as one option so the
223-
// default path takes nothing extra — not even a second sender
224-
// on the serial channel, whose lifetime ends the writer task.
225-
let scan = osc
226-
.clone()
227-
.map(|osc| (osc, cmd_tx.clone(), input_capture.clone()));
197+
// `None` without the opt-in, so the default path's forwarding
198+
// task carries nothing extra.
199+
let scanner = osc.clone();
228200
tokio::spawn(async move {
229201
loop {
230202
match output_rx.recv().await {
231203
Ok(PtyOutputEvent::Bytes(data)) => {
232-
let output = match &scan {
233-
Some((osc, cmd_tx, capture)) => {
234-
let actions = scan_and_reply(osc, cmd_tx, capture.as_ref(), &data);
204+
let output = match &scanner {
205+
Some(osc) => {
206+
let actions = scan(osc, &data);
235207
TabOutput::Scanned { data, actions }
236208
}
237209
None => TabOutput::Bytes(data),
@@ -282,8 +254,7 @@ impl TabSession {
282254
// supervisor in the exact order they were submitted. The
283255
// shared channel guarantees keystrokes (and resizes relative
284256
// to them) never reorder. Ends when the last `cmd_tx` drops
285-
// (TabSession dropped) — the forwarding task holds one too, so
286-
// a drain-side reply can never race the handle's teardown.
257+
// (TabSession dropped).
287258
tokio::spawn(async move {
288259
while let Some(cmd) = cmd_rx.recv().await {
289260
match cmd {
@@ -352,11 +323,11 @@ impl TabSession {
352323
/// — today only `tab.feed_pty_bytes`, the test-mode byte injector.
353324
///
354325
/// It runs the SAME router and the SAME color state the drain does,
355-
/// so an injected chunk is indistinguishable from a real one: query
356-
/// replies are enqueued here, the caller writes the bytes into its
357-
/// terminal and applies the returned actions. Without the OSC
358-
/// opt-in there is nothing to scan with and the caller keeps its
359-
/// own router — no actions come back.
326+
/// so an injected chunk is indistinguishable from a real one: the
327+
/// caller writes the bytes into its terminal and applies the
328+
/// returned actions. Without the OSC opt-in there is nothing to
329+
/// scan with and the caller keeps its own router — no actions come
330+
/// back.
360331
///
361332
/// **Callers must not race live PTY output.** Injected bytes and
362333
/// PTY bytes are two producers into one streaming scanner and one
@@ -374,7 +345,7 @@ impl TabSession {
374345
let Some(osc) = &self.osc else {
375346
return Vec::new();
376347
};
377-
scan_and_reply(osc, &self.cmd_tx, self.input_capture.as_ref(), bytes)
348+
scan(osc, bytes)
378349
}
379350

380351
/// Re-seed the drain-local color state after a theme change. A
@@ -390,16 +361,15 @@ impl TabSession {
390361
//
391362
// The single consumer is the `cmd_rx` task above: it is the
392363
// only thing that writes to the tab's master fd, so bytes reach
393-
// the PTY in exactly the order they were enqueued. What is NOT
394-
// single is the producer side — the UI adapter's main thread
395-
// (keystrokes, paste, resize, terminal replies) and, under the
396-
// OSC opt-in, this session's own drain task (color-query
397-
// replies, which is the point: they leave without waiting for
398-
// the UI's event loop). Both funnel through `enqueue_input`.
364+
// the PTY in exactly the order they were enqueued. The producer
365+
// side is the UI adapter's main thread — keystrokes, paste,
366+
// resize, and the terminal's own `write_pty` replies, which the
367+
// UI drains after each `vt_write`/`resize`. They all funnel
368+
// through `enqueue_input`.
399369
//
400370
// The ordering contract that follows from that: enqueue order
401371
// IS observed order, and user input may interleave with
402-
// synthesised replies. `tab.capture_pty_input` observes the
372+
// terminal replies. `tab.capture_pty_input` observes the
403373
// same order — its buffer is written under a lock held across
404374
// the enqueue — so a test may assert on the presence and
405375
// relative order of what it caused, never on the absence of an

0 commit comments

Comments
 (0)