fix(test): identify rendered milestone fences

Co-authored-by: GPT-5.6 <codex@openai.com>

Author: wan9chi

crates/pty_terminal_test/README.md

@@ -47,24 +47,23 @@ assert!(status.success());
 
 Milestones are encoded as an OSC 8 hyperlink:
 
-- open: `ESC ] 8 ; ; https://milestone.invalid/<hex(name)> ESC \`
-- hypertext: zero-width space (`U+200B`)
+- open: `ESC ] 8 ; ; https://milestone.invalid/<id>/<hex(name)> ESC \`
+- hypertext: the same ID encoded with zero-width rendered characters
 - close: `ESC ] 8 ; ; ESC \`
 
 `Reader::expect_milestone` works like this:
 
 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.
+2. Pair each marker with the rendered fence carrying the same ID.
 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.
+The helper strips the protocol's zero-width characters from returned screen text.
 
-Only one marker may await its rendered anchor at a time. ConPTY can coalesce
-intermediate screen frames, so anonymous anchors cannot be paired reliably when
-several renders are outstanding. Before sending input that can trigger another
-render, consume the current milestone with `expect_milestone`, or use
-`wait_for_next_milestone` for a silent flow-control wait.
+ConPTY can coalesce intermediate screen frames while forwarding their OSC
+markers. Self-identifying fences let `expect_milestone` consume and ignore
+completed non-target milestones until the requested state arrives, without
+mistaking a surviving older fence for a newer marker.
 
 ## Cross-platform behavior
 
@@ -92,19 +91,3 @@ writer.write_all(b"input")?;
 writer.flush()?;
 let screen = reader.expect_milestone("after-input");
 ```
-
-When a single test interaction sends several logical input events, synchronize
-each event before sending the next one:
-
-```rust
-let mut encoded = [0; 4];
-for character in "input".chars() {
-    writer.write_all(character.encode_utf8(&mut encoded).as_bytes())?;
-    writer.flush()?;
-    reader.wait_for_next_milestone();
-}
-let screen = reader.expect_milestone("after-input");
-```
-
-`wait_for_next_milestone` leaves the completed marker available, so the final
-named expectation still validates and snapshots the requested state.

crates/pty_terminal_test/src/lib.rs

@@ -8,68 +8,92 @@ pub use pty_terminal::{
     terminal::{ChildHandle, PtyWriter},
 };
 
-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.
+/// A milestone starts with an OSC 8 hyperlink carrying its name and identifier,
+/// followed by the same identifier encoded with zero-width rendered characters.
+/// On `ConPTY`, the OSC control sequence can be forwarded before earlier screen
+/// updates, while the fence follows those updates through the asynchronous
+/// rendering path. A milestone is complete only after both matching parts arrive.
 ///
-/// `ConPTY` may coalesce intermediate rendered frames, so the protocol permits
-/// only one marker to await its fence at a time. The test harness must consume
-/// that fence before sending input that can trigger another render.
+/// Self-identifying fences allow `ConPTY` to coalesce intermediate frames without
+/// making a later fence complete the wrong marker.
 #[derive(Default)]
 struct MilestoneTracker {
-    /// Marker whose rendered zero-width anchor has not arrived yet.
-    awaiting_fence: Option<String>,
+    pending: VecDeque<pty_terminal_test_client::DecodedMilestone>,
     /// Marker names whose matching rendered anchors have arrived.
     completed: VecDeque<String>,
-    /// Monotonic completion count used for silent flow-control waits.
-    completion_count: usize,
+    fence_decoder: RenderFenceDecoder,
 }
 
 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()
+    fn pop_completed(&mut self) -> Option<String> {
+        self.completed.pop_front()
     }
 }
 
 impl vte::Perform for MilestoneTracker {
     fn print(&mut self, character: char) {
-        // `print` is called only for rendered characters, not for bytes inside
-        // OSC metadata. With stop-and-wait flow control, the anchor can only
-        // belong to the single marker awaiting a fence.
-        if character == MILESTONE_HYPERTEXT
-            && let Some(name) = self.awaiting_fence.take()
+        if let Some(id) = self.fence_decoder.advance(character)
+            && let Some(index) = self.pending.iter().position(|marker| marker.id == id)
         {
-            self.completed.push_back(name);
-            self.completion_count += 1;
+            // A later rendered fence proves that any older pending frames were
+            // coalesced. Drop them together with the matching marker.
+            let marker = self.pending.drain(..=index).next_back().unwrap();
+            self.completed.push_back(marker.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) {
-            assert!(
-                self.awaiting_fence.is_none(),
-                "milestone protocol violation: marker '{name}' arrived before the rendered fence for '{}'",
-                self.awaiting_fence.as_deref().unwrap()
-            );
-            self.awaiting_fence = Some(name);
+        if let Some(marker) = pty_terminal_test_client::decode_milestone_from_osc8_params(params) {
+            if let Some(index) = self.pending.iter().position(|pending| pending.id == marker.id) {
+                self.pending.remove(index);
+            }
+            self.pending.push_back(marker);
         }
     }
 }
 
+#[derive(Default)]
+struct RenderFenceDecoder {
+    active: bool,
+    id: u32,
+    digits: u8,
+}
+
+impl RenderFenceDecoder {
+    fn advance(&mut self, character: char) -> Option<u32> {
+        if pty_terminal_test_client::is_milestone_render_boundary(character) {
+            if self.active && self.digits == pty_terminal_test_client::MILESTONE_RENDER_DIGITS {
+                let id = self.id;
+                // The closing boundary can also be the opening boundary of a
+                // following fence if the previous close was coalesced away.
+                self.id = 0;
+                self.digits = 0;
+                return Some(id);
+            }
+            self.active = true;
+            self.id = 0;
+            self.digits = 0;
+            return None;
+        }
+
+        let Some(digit) = pty_terminal_test_client::decode_milestone_render_digit(character) else {
+            self.active = false;
+            return None;
+        };
+        if !self.active || self.digits == pty_terminal_test_client::MILESTONE_RENDER_DIGITS {
+            self.active = false;
+            return None;
+        }
+        self.id = (self.id << 2) | u32::from(digit);
+        self.digits += 1;
+        None
+    }
+}
+
 /// A test-oriented terminal that provides milestone-based synchronization.
 ///
 /// Wraps a PTY terminal, splitting it into a [`PtyWriter`] for sending input
@@ -139,15 +163,19 @@ impl Reader {
     #[must_use]
     pub fn screen_contents(&self) -> String {
         let mut contents = self.pty.screen_contents();
-        contents.retain(|ch| ch != MILESTONE_HYPERTEXT);
+        contents.retain(|character| {
+            !pty_terminal_test_client::is_milestone_render_character(character)
+        });
         contents
     }
 
     /// Returns the screen contents with inline ANSI SGR escape codes preserved.
     /// Useful for snapshot tests that need to assert colour or style attributes.
     #[must_use]
     pub fn screen_contents_formatted(&self) -> Vec<u8> {
-        self.pty.screen_contents_formatted()
+        pty_terminal_test_client::strip_milestone_render_characters(
+            &self.pty.screen_contents_formatted(),
+        )
     }
 
     /// Reads from the PTY until a milestone with the given name is encountered.
@@ -171,35 +199,17 @@ impl Reader {
         let mut buf = [0u8; 4096];
 
         loop {
-            if self.milestone_tracker.take_completed(name) {
-                return self.screen_contents();
+            while let Some(completed) = self.milestone_tracker.pop_completed() {
+                if completed == name {
+                    return self.screen_contents();
+                }
             }
 
             let n = self.read(&mut buf).expect("PTY read failed");
             assert!(n > 0, "EOF reached before milestone '{name}'");
         }
     }
 
-    /// Waits until the next milestone marker and its rendered fence arrive.
-    ///
-    /// Unlike [`Self::expect_milestone`], this does not consume the completed
-    /// milestone. It is intended for stop-and-wait flow control when a test
-    /// sends several logical input events but only snapshots the final state.
-    ///
-    /// # Panics
-    ///
-    /// Panics if the child process exits before another milestone completes,
-    /// or if a PTY read fails.
-    pub fn wait_for_next_milestone(&mut self) {
-        let completion_count = self.milestone_tracker.completion_count;
-        let mut buf = [0u8; 4096];
-
-        while self.milestone_tracker.completion_count == completion_count {
-            let n = self.read(&mut buf).expect("PTY read failed");
-            assert!(n > 0, "EOF reached before the next milestone");
-        }
-    }
-
     /// Reads all remaining PTY output until the child exits, then returns the exit status.
     ///
     /// # Errors
@@ -220,16 +230,18 @@ impl Reader {
 mod tests {
     use super::*;
 
-    fn marker_without_fence(name: &str) -> Vec<u8> {
+    fn marker_without_fence(name: &str) -> (Vec<u8>, 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)
+            .windows(pty_terminal_test_client::MILESTONE_RENDER_FENCE_START.len())
+            .position(|window| window == pty_terminal_test_client::MILESTONE_RENDER_FENCE_START)
             .unwrap();
-        marker.drain(index..index + pty_terminal_test_client::MILESTONE_RENDER_FENCE.len());
-        marker
+        let fence = marker
+            .drain(index..index + pty_terminal_test_client::MILESTONE_RENDER_FENCE_LEN)
+            .collect();
+        (marker, fence)
     }
 
     fn advance(parser: &mut vte::Parser, tracker: &mut MilestoneTracker, bytes: &[u8]) {
@@ -243,12 +255,13 @@ mod tests {
 
         // 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"));
+        let (marker, fence) = marker_without_fence("target");
+        advance(&mut parser, &mut tracker, &marker);
         advance(&mut parser, &mut tracker, b"rendered output");
-        assert!(!tracker.take_completed("target"));
+        assert!(tracker.pop_completed().is_none());
 
-        advance(&mut parser, &mut tracker, pty_terminal_test_client::MILESTONE_RENDER_FENCE);
-        assert!(tracker.take_completed("target"));
+        advance(&mut parser, &mut tracker, &fence);
+        assert_eq!(tracker.pop_completed().as_deref(), Some("target"));
     }
 
     #[test]
@@ -260,7 +273,11 @@ mod tests {
             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}");
+            assert_eq!(
+                tracker.pop_completed().as_deref(),
+                Some("target"),
+                "failed at split {split}"
+            );
         }
     }
 
@@ -270,19 +287,65 @@ mod tests {
         let mut tracker = MilestoneTracker::default();
 
         advance(&mut parser, &mut tracker, &pty_terminal_test_client::encoded_milestone("first"));
-        assert!(tracker.take_completed("first"));
+        assert_eq!(tracker.pop_completed().as_deref(), Some("first"));
         advance(&mut parser, &mut tracker, &pty_terminal_test_client::encoded_milestone("second"));
-        assert!(tracker.take_completed("second"));
+        assert_eq!(tracker.pop_completed().as_deref(), Some("second"));
+    }
+
+    #[test]
+    fn coalesced_markers_complete_the_latest_state() {
+        let mut parser = vte::Parser::new();
+        let mut tracker = MilestoneTracker::default();
+        let (mut markers, _) = marker_without_fence("first");
+        let (second_marker, second_fence) = marker_without_fence("second");
+        markers.extend(second_marker);
+
+        advance(&mut parser, &mut tracker, &markers);
+        advance(&mut parser, &mut tracker, &second_fence);
+        assert_eq!(tracker.pop_completed().as_deref(), Some("second"));
+        assert!(tracker.pop_completed().is_none());
+        assert!(tracker.pending.is_empty());
     }
 
     #[test]
-    #[should_panic(expected = "milestone protocol violation")]
-    fn second_marker_before_fence_is_a_protocol_violation() {
+    fn surviving_fences_complete_their_own_markers() {
         let mut parser = vte::Parser::new();
         let mut tracker = MilestoneTracker::default();
-        let mut markers = marker_without_fence("first");
-        markers.extend(marker_without_fence("second"));
+        let (mut markers, first_fence) = marker_without_fence("first");
+        let (second_marker, second_fence) = marker_without_fence("second");
+        markers.extend(second_marker);
 
         advance(&mut parser, &mut tracker, &markers);
+        advance(&mut parser, &mut tracker, &first_fence);
+        advance(&mut parser, &mut tracker, &second_fence);
+        assert_eq!(tracker.pop_completed().as_deref(), Some("first"));
+        assert_eq!(tracker.pop_completed().as_deref(), Some("second"));
+    }
+
+    #[test]
+    fn a_missing_close_does_not_consume_the_next_fence() {
+        let mut parser = vte::Parser::new();
+        let mut tracker = MilestoneTracker::default();
+        let (mut markers, mut first_fence) = marker_without_fence("first");
+        let (second_marker, second_fence) = marker_without_fence("second");
+        markers.extend(second_marker);
+        first_fence.truncate(
+            first_fence.len() - pty_terminal_test_client::MILESTONE_RENDER_FENCE_START.len(),
+        );
+
+        advance(&mut parser, &mut tracker, &markers);
+        advance(&mut parser, &mut tracker, &first_fence);
+        advance(&mut parser, &mut tracker, &second_fence);
+        assert_eq!(tracker.pop_completed().as_deref(), Some("first"));
+        assert_eq!(tracker.pop_completed().as_deref(), Some("second"));
+    }
+
+    #[test]
+    fn screen_stripping_removes_reserved_protocol_characters() {
+        let standalone = "before\u{2060}\u{2061}after";
+        assert_eq!(
+            pty_terminal_test_client::strip_milestone_render_characters(standalone.as_bytes()),
+            b"beforeafter"
+        );
     }
 }