fix(test): wait for rendered milestone fences

Cargo.toml

@@ -40,7 +40,7 @@ future_not_send = "allow"
 
 [workspace.dependencies]
 anstream = "1.0.0"
-anyhow = "1.0.98"
+anyhow = "1.0.103"
 assert2 = "0.4.0"
 assertables = "10.0.0"
 async-trait = "0.1.89"
@@ -84,7 +84,7 @@ globset = "0.4.18"
 jsonc-parser = { version = "0.32.0", features = ["serde"] }
 libc = "0.2.185"
 libtest-mimic = "0.8.2"
-memmap2 = "0.9.7"
+memmap2 = "0.9.11"
 monostate = "1.0.2"
 napi = "3"
 napi-build = "2"
@@ -161,6 +161,7 @@ vite_task_plan = { path = "crates/vite_task_plan" }
 vite_task_server = { path = "crates/vite_task_server" }
 vite_workspace = { path = "crates/vite_workspace" }
 vt100 = "0.16.2"
+vte = "0.15.0"
 wax = "0.7.0"
 which = "8.0.0"
 widestring = "1.2.0"

crates/pty_terminal_test/Cargo.toml

@@ -12,6 +12,7 @@ anyhow = { workspace = true }
 portable-pty = { workspace = true }
 pty_terminal = { workspace = true }
 pty_terminal_test_client = { workspace = true }
+vte = { workspace = true }
 
 [dev-dependencies]
 crossterm = { workspace = true }
@@ -24,7 +25,6 @@ subprocess_test = { workspace = true, features = ["portable-pty"] }
 workspace = true
 
 [lib]
-test = false
 doctest = false
 
 [package.metadata.cargo-shear]

crates/pty_terminal_test/README.md

@@ -53,10 +53,10 @@ Milestones are encoded as an OSC 8 hyperlink:
 
 `Reader::expect_milestone` works like this:
 
-1. Drain parsed unhandled OSC sequences from `PtyReader`.
-2. Decode OSC 8 URI payload back into milestone name.
-3. If no match yet, continue reading from PTY and repeat.
-4. On match, return current `screen_contents()`.
+1. Decode OSC 8 URI payloads from the PTY stream back into milestone names.
+2. Pair each marker with its following zero-width rendered anchor.
+3. Wait until both the requested marker and its anchor have arrived.
+4. Return the current `screen_contents()`.
 
 The helper strips the protocol's zero-width space from returned screen text.
 
@@ -65,6 +65,8 @@ The helper strips the protocol's zero-width space from returned screen text.
 The OSC 8 + zero-width anchor approach is used because it works across Unix and
 Windows ConPTY in this project. In particular, zero-length hyperlink opens can
 be lost on some Windows output paths, so the zero-width anchor is intentional.
+Waiting for the anchor also prevents ConPTY's control-sequence path from
+delivering a milestone before earlier rendered text.
 
 ## Typical test pattern
 

crates/pty_terminal_test/src/lib.rs

@@ -1,4 +1,4 @@
-use std::io::{BufReader, Read};
+use std::{collections::VecDeque, io::Read};
 
 pub use portable_pty::CommandBuilder;
 use pty_terminal::terminal::{PtyReader, Terminal};
@@ -10,6 +10,58 @@ pub use pty_terminal::{
 
 const MILESTONE_HYPERTEXT: char = '\u{200b}';
 
+/// Tracks the two independently delivered parts of each milestone.
+///
+/// A milestone starts with an OSC 8 hyperlink carrying its name and contains a
+/// zero-width printable character. On `ConPTY`, the OSC control sequence can be
+/// forwarded before earlier screen updates, while the printable character
+/// follows those updates through the asynchronous rendering path. A milestone
+/// is therefore complete only after both parts have arrived.
+///
+/// Several OSC markers can overtake their anchors. The two queues preserve the
+/// protocol order so an earlier marker's delayed anchor cannot complete a later
+/// marker by mistake.
+#[derive(Default)]
+struct MilestoneTracker {
+    /// Marker names whose rendered zero-width anchors have not arrived yet.
+    awaiting_fence: VecDeque<String>,
+    /// Marker names whose matching rendered anchors have arrived.
+    completed: VecDeque<String>,
+}
+
+impl MilestoneTracker {
+    fn take_completed(&mut self, name: &str) -> bool {
+        // Keep unrelated completed milestones available for later calls. A PTY
+        // read can contain more than the milestone currently being requested.
+        self.completed
+            .iter()
+            .position(|completed| completed == name)
+            .and_then(|index| self.completed.remove(index))
+            .is_some()
+    }
+}
+
+impl vte::Perform for MilestoneTracker {
+    fn print(&mut self, character: char) {
+        // `print` is called only for rendered characters, not for bytes inside
+        // OSC metadata. ConPTY preserves the order of these rendered anchors,
+        // so each anchor completes the oldest marker still awaiting one.
+        if character == MILESTONE_HYPERTEXT
+            && let Some(name) = self.awaiting_fence.pop_front()
+        {
+            self.completed.push_back(name);
+        }
+    }
+
+    fn osc_dispatch(&mut self, params: &[&[u8]], _bell_terminated: bool) {
+        // The decoder accepts only milestone hyperlink opens. Ordinary OSC
+        // sequences and the empty OSC 8 close sequence are ignored.
+        if let Some(name) = pty_terminal_test_client::decode_milestone_from_osc8_params(params) {
+            self.awaiting_fence.push_back(name);
+        }
+    }
+}
+
 /// A test-oriented terminal that provides milestone-based synchronization.
 ///
 /// Wraps a PTY terminal, splitting it into a [`PtyWriter`] for sending input
@@ -23,7 +75,17 @@ pub struct TestTerminal {
 
 /// The read half of a test terminal, wrapping [`PtyReader`] with milestone support.
 pub struct Reader {
-    pty: BufReader<PtyReader>,
+    /// Reads bytes and updates the terminal's primary `vt100` screen parser.
+    ///
+    /// This is deliberately not wrapped in `BufReader`: its read-ahead would
+    /// let the primary parser consume bytes the milestone parser has not seen.
+    pty: PtyReader,
+    /// Observes the same byte stream to distinguish OSC markers from printable
+    /// anchors. `vt100::Callbacks` exposes unhandled OSC sequences but has no
+    /// callback for ordinary rendered characters, hence this small second parser.
+    milestone_parser: vte::Parser,
+    /// Persists protocol state across reads and `expect_milestone` calls.
+    milestone_tracker: MilestoneTracker,
     child_handle: ChildHandle,
 }
 
@@ -37,17 +99,38 @@ impl TestTerminal {
         let Terminal { pty_reader, pty_writer, child_handle, .. } = Terminal::spawn(size, cmd)?;
         Ok(Self {
             writer: pty_writer,
-            reader: Reader { pty: BufReader::new(pty_reader), child_handle: child_handle.clone() },
+            reader: Reader {
+                pty: pty_reader,
+                milestone_parser: vte::Parser::new(),
+                milestone_tracker: MilestoneTracker::default(),
+                child_handle: child_handle.clone(),
+            },
             child_handle,
         })
     }
 }
 
 impl Reader {
+    /// Reads once while keeping the screen parser and milestone parser in lockstep.
+    ///
+    /// All PTY draining, including shutdown, must go through this method. Reading
+    /// directly from `pty` would update the screen while silently skipping those
+    /// bytes in the milestone protocol state.
+    fn read(&mut self, buf: &mut [u8]) -> std::io::Result<usize> {
+        let n = self.pty.read(buf)?;
+        self.milestone_parser.advance(&mut self.milestone_tracker, &buf[..n]);
+
+        // `PtyReader`'s primary parser also records the OSC sequences. The
+        // dedicated tracker above owns milestone handling, so discard this
+        // duplicate copy rather than letting it grow for the lifetime of a test.
+        drop(self.pty.take_unhandled_osc_sequences());
+        Ok(n)
+    }
+
     /// Returns terminal screen contents with milestone hyperlink text removed.
     #[must_use]
     pub fn screen_contents(&self) -> String {
-        let mut contents = self.pty.get_ref().screen_contents();
+        let mut contents = self.pty.screen_contents();
         contents.retain(|ch| ch != MILESTONE_HYPERTEXT);
         contents
     }
@@ -56,18 +139,20 @@ impl Reader {
     /// Useful for snapshot tests that need to assert colour or style attributes.
     #[must_use]
     pub fn screen_contents_formatted(&self) -> Vec<u8> {
-        self.pty.get_ref().screen_contents_formatted()
+        self.pty.screen_contents_formatted()
     }
 
     /// Reads from the PTY until a milestone with the given name is encountered.
     ///
     /// Returns the terminal screen contents at the moment the milestone is detected.
     ///
     /// Milestones use a uniform protocol across platforms: the milestone name
-    /// is encoded in an OSC 8 hyperlink URI. We parse unhandled OSC sequences
-    /// from the VT parser state (instead of raw byte matching), then decode the
-    /// milestone URI payload. The zero-width milestone hyperlink anchor is
-    /// stripped from returned screen contents.
+    /// is encoded in an OSC 8 hyperlink URI. A zero-width hyperlink anchor follows
+    /// each marker through the rendered output path. The reader waits for both the
+    /// marker and its corresponding anchor before returning, then strips the anchor
+    /// from the returned screen contents. Marker and anchor parsing is incremental,
+    /// so either sequence may be split across PTY reads or share a read with other
+    /// milestones.
     ///
     /// # Panics
     ///
@@ -78,20 +163,11 @@ impl Reader {
         let mut buf = [0u8; 4096];
 
         loop {
-            let found = self
-                .pty
-                .get_ref()
-                .take_unhandled_osc_sequences()
-                .into_iter()
-                .filter_map(|params| {
-                    pty_terminal_test_client::decode_milestone_from_osc8_params(&params)
-                })
-                .any(|decoded| decoded == name);
-            if found {
+            if self.milestone_tracker.take_completed(name) {
                 return self.screen_contents();
             }
 
-            let n = self.pty.read(&mut buf).expect("PTY read failed");
+            let n = self.read(&mut buf).expect("PTY read failed");
             assert!(n > 0, "EOF reached before milestone '{name}'");
         }
     }
@@ -106,8 +182,75 @@ impl Reader {
     ///
     /// Panics if reading from the PTY fails.
     pub fn wait_for_exit(&mut self) -> anyhow::Result<ExitStatus> {
-        let mut discard = Vec::new();
-        self.pty.read_to_end(&mut discard).expect("PTY read_to_end failed");
+        let mut buf = [0u8; 4096];
+        while self.read(&mut buf).expect("PTY read failed") > 0 {}
         self.child_handle.wait()
     }
 }
+
+#[cfg(test)]
+mod tests {
+    use super::*;
+
+    fn marker_without_fence(name: &str) -> Vec<u8> {
+        // Model ConPTY's fast control path by delivering the complete OSC marker
+        // before its printable anchor reaches the output pipe.
+        let mut marker = pty_terminal_test_client::encoded_milestone(name);
+        let index = marker
+            .windows(pty_terminal_test_client::MILESTONE_RENDER_FENCE.len())
+            .position(|window| window == pty_terminal_test_client::MILESTONE_RENDER_FENCE)
+            .unwrap();
+        marker.drain(index..index + pty_terminal_test_client::MILESTONE_RENDER_FENCE.len());
+        marker
+    }
+
+    fn advance(parser: &mut vte::Parser, tracker: &mut MilestoneTracker, bytes: &[u8]) {
+        parser.advance(tracker, bytes);
+    }
+
+    #[test]
+    fn milestone_waits_for_rendered_fence() {
+        let mut parser = vte::Parser::new();
+        let mut tracker = MilestoneTracker::default();
+
+        // Receiving the marker and subsequent printable output is insufficient:
+        // only the protocol's rendered anchor establishes the screen barrier.
+        advance(&mut parser, &mut tracker, &marker_without_fence("target"));
+        advance(&mut parser, &mut tracker, b"rendered output");
+        assert!(!tracker.take_completed("target"));
+
+        advance(&mut parser, &mut tracker, pty_terminal_test_client::MILESTONE_RENDER_FENCE);
+        assert!(tracker.take_completed("target"));
+    }
+
+    #[test]
+    fn milestone_parses_across_every_chunk_boundary() {
+        let marker = pty_terminal_test_client::encoded_milestone("target");
+
+        for split in 0..=marker.len() {
+            let mut parser = vte::Parser::new();
+            let mut tracker = MilestoneTracker::default();
+            advance(&mut parser, &mut tracker, &marker[..split]);
+            advance(&mut parser, &mut tracker, &marker[split..]);
+            assert!(tracker.take_completed("target"), "failed at split {split}");
+        }
+    }
+
+    #[test]
+    fn rendered_fences_complete_overtaken_markers_in_order() {
+        let mut parser = vte::Parser::new();
+        let mut tracker = MilestoneTracker::default();
+        let mut markers = marker_without_fence("first");
+        markers.extend(marker_without_fence("second"));
+
+        // Both controls overtake rendering. The first anchor must still complete
+        // `first`, never whichever marker the caller happens to be waiting for.
+        advance(&mut parser, &mut tracker, &markers);
+        advance(&mut parser, &mut tracker, pty_terminal_test_client::MILESTONE_RENDER_FENCE);
+        assert!(tracker.take_completed("first"));
+        assert!(!tracker.take_completed("second"));
+
+        advance(&mut parser, &mut tracker, pty_terminal_test_client::MILESTONE_RENDER_FENCE);
+        assert!(tracker.take_completed("second"));
+    }
+}