Merge pull request #1065 from fabro-sh/codex/skipped-stage-display

Distinguish skipped stages in CLI progress

lib/apps/fabro-cli/src/commands/run/mod.rs

@@ -33,6 +33,7 @@ pub(crate) mod run_progress;
 pub(crate) mod runner;
 mod selection;
 pub(crate) mod ssh;
+mod stage_presentation;
 pub(crate) mod start;
 pub(crate) mod steer;
 #[cfg(test)]

lib/apps/fabro-cli/src/commands/run/petri_stream.rs

@@ -25,12 +25,13 @@ use std::time::Duration;
 use anyhow::{Context as _, Result};
 use chrono::{DateTime, TimeZone as _, Utc};
 use fabro_redact::redact_jsonl_line;
-use fabro_types::{RunId, RunStreamItem, RunStreamItemKind};
+use fabro_types::{RunId, RunStreamItem, RunStreamItemKind, StageOutcome};
 use fabro_util::terminal::Styles;
 use serde_json::Value;
 use tokio::time;
 use tracing::debug;
 
+use super::stage_presentation::StagePresentation;
 use crate::args::EventsArgs;
 use crate::server_client;
 use crate::shared::{format_duration_ms, format_usd_micros};
@@ -338,22 +339,29 @@ pub(crate) fn format_pretty(
             let status = outcome.get("status").and_then(Value::as_str).unwrap_or("?");
             let duration = format_duration_ms(elapsed.unwrap_or(0));
             if !executed || status == "skipped" {
+                let presentation = StagePresentation::new(StageOutcome::Skipped, styles);
                 return Some(format!(
                     "{ts} {} {}  {}",
-                    styles.dim.apply_to("\u{2298}"),
+                    presentation.glyph,
                     styles.bold.apply_to(label),
-                    styles.dim.apply_to("skipped"),
+                    presentation.label.unwrap_or_default(),
                 ));
             }
             match status {
                 "success" | "partial_success" => {
+                    let outcome_kind = if status == "partial_success" {
+                        StageOutcome::PartiallySucceeded
+                    } else {
+                        StageOutcome::Succeeded
+                    };
+                    let presentation = StagePresentation::new(outcome_kind, styles);
                     let mut line = format!(
                         "{ts} {} {}",
-                        styles.green.apply_to("\u{2713}"),
+                        presentation.glyph,
                         styles.bold.apply_to(label),
                     );
-                    if status == "partial_success" {
-                        let _ = write!(line, "  {}", styles.yellow.apply_to("partial"));
+                    if let Some(label) = presentation.label {
+                        let _ = write!(line, "  {label}");
                     }
                     let _ = write!(line, "  {duration}");
                     if let Some(usage) = usage_summary(outcome, styles) {
@@ -362,13 +370,19 @@ pub(crate) fn format_pretty(
                     Some(line)
                 }
                 other => {
+                    let presentation = StagePresentation::new(
+                        StageOutcome::Failed {
+                            retry_requested: false,
+                        },
+                        styles,
+                    );
                     let error = outcome
                         .pointer("/failure/message")
                         .and_then(Value::as_str)
                         .unwrap_or(other);
                     Some(format!(
                         "{ts} {} {}  {}",
-                        styles.red.apply_to("\u{2717}"),
+                        presentation.glyph,
                         styles.bold.apply_to(label),
                         styles.red.apply_to(error),
                     ))

lib/apps/fabro-cli/src/commands/run/run_progress/mod.rs

@@ -11,6 +11,8 @@ mod petri;
 mod renderer;
 mod stage_display;
 mod styles;
+#[cfg(test)]
+mod tests;
 
 use event::ProgressEvent;
 use info_display::InfoDisplay;

lib/apps/fabro-cli/src/commands/run/run_progress/stage_display.rs

@@ -9,6 +9,7 @@ use indicatif::ProgressBar;
 use super::event::ProgressUsage;
 use super::renderer::ProgressRenderer;
 use super::styles;
+use crate::commands::run::stage_presentation::StagePresentation;
 use crate::shared::{format_duration_ms, format_tokens_human};
 
 const MAX_TOOL_CALLS: usize = 5;
@@ -138,14 +139,11 @@ impl StageDisplay {
         status: &str,
         usage: Option<&ProgressUsage>,
     ) {
-        let succeeded = status.parse::<StageOutcome>().map_or_else(
-            |_| matches!(status, "succeeded" | "partially_succeeded"),
-            |status| {
-                matches!(
-                    status,
-                    StageOutcome::Succeeded | StageOutcome::PartiallySucceeded
-                )
-            },
+        let presentation = StagePresentation::new(
+            status.parse().unwrap_or(StageOutcome::Failed {
+                retry_requested: false,
+            }),
+            renderer.styles(),
         );
         let cost_str = usage
             .and_then(ProgressUsage::display_cost)
@@ -171,13 +169,14 @@ impl StageDisplay {
         } else {
             String::new()
         };
-        let prefix = format!("{cost_str}{}{stats_str}", format_duration_ms(duration_ms));
-        let glyph = if succeeded {
-            styles::green_check(renderer.styles())
-        } else {
-            styles::red_cross(renderer.styles())
-        };
-        self.finish_stage(renderer, node_id, name, &glyph, &prefix);
+        let label = presentation
+            .label
+            .map_or_else(String::new, |label| format!("{label}  "));
+        let prefix = format!(
+            "{label}{cost_str}{}{stats_str}",
+            format_duration_ms(duration_ms)
+        );
+        self.finish_stage(renderer, node_id, name, &presentation.glyph, &prefix);
     }
 
     pub(super) fn on_stage_failed(
@@ -187,13 +186,13 @@ impl StageDisplay {
         name: &str,
         error: &str,
     ) {
-        self.finish_stage(
-            renderer,
-            node_id,
-            name,
-            &styles::red_cross(renderer.styles()),
-            "",
+        let presentation = StagePresentation::new(
+            StageOutcome::Failed {
+                retry_requested: false,
+            },
+            renderer.styles(),
         );
+        self.finish_stage(renderer, node_id, name, &presentation.glyph, "");
         let summary = styles::last_line_truncated(error, 120);
         Self::insert_global_info_line(
             renderer,

lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs

@@ -0,0 +1,205 @@
+#![expect(
+    clippy::disallowed_types,
+    reason = "capture the synchronous progress renderer's output in memory"
+)]
+
+use std::io::{self, Write};
+use std::sync::{Arc, Mutex};
+
+use fabro_types::{RunStreamItemKind, fixtures};
+use fabro_util::terminal::Styles;
+use serde_json::{Value, json};
+
+use super::*;
+use crate::commands::run::petri_stream::{self, PrettyState};
+
+#[derive(Clone, Default)]
+struct Output(Arc<Mutex<Vec<u8>>>);
+
+impl Write for Output {
+    fn write(&mut self, buf: &[u8]) -> io::Result<usize> {
+        self.0.lock().unwrap().extend_from_slice(buf);
+        Ok(buf.len())
+    }
+
+    fn flush(&mut self) -> io::Result<()> {
+        Ok(())
+    }
+}
+
+impl Output {
+    fn text(&self) -> String {
+        String::from_utf8(self.0.lock().unwrap().clone()).unwrap()
+    }
+}
+
+fn visit(seq: u64, event: &str, derived: Value) -> RunStreamItem {
+    let mut derived = derived;
+    derived["event"] = json!(event);
+    RunStreamItem {
+        run_id:      fixtures::RUN_1,
+        stream_seq:  seq,
+        kind:        RunStreamItemKind::Petri,
+        id:          seq.to_string(),
+        recorded_at: 1_789_706_579_000 + seq * 1000,
+        item:        json!({
+            "origin": "derived",
+            "context": {"invocation": 0, "execution": 0},
+            "subject": {
+                "node": {"id": 2, "name": "work", "kind": "attractor/x",
+                    "meta": {"label": "work", "kind": "agent"}},
+                "firing": 2, "visit": 1, "attempt": 1,
+                "generation": 0, "branch": {"role": "none"}
+            },
+            "derived": derived
+        }),
+    }
+}
+
+fn completed(status: &str, executed: bool) -> RunStreamItem {
+    visit(
+        4,
+        "visit.completed",
+        json!({
+            "outcome": {"status": status},
+            "executed": executed,
+            "attempts": 1
+        }),
+    )
+}
+
+#[test]
+fn progress_and_events_present_stage_outcomes_consistently() {
+    let mut cases = Vec::new();
+    for (status, executed) in [
+        ("skipped", true),
+        ("success", false),
+        ("success", true),
+        ("partial_success", true),
+        ("failure", true),
+        ("cancelled", true),
+        ("unrecognized", true),
+    ] {
+        let started = visit(1, "visit.started", json!({}));
+        let completed = completed(status, executed);
+        let raw_before = petri_stream::raw_line(&completed).unwrap();
+        let mut progress = Vec::new();
+        for verbose in [false, true] {
+            let output = Output::default();
+            let renderer = ProgressRenderer::new_plain(Box::new(output.clone()), false);
+            let mut ui = ProgressUI::with_renderer(renderer, verbose);
+            ui.handle_stream_item(&started);
+            ui.handle_stream_item(&completed);
+            assert!(ui.stage.active_stages.is_empty());
+            ui.finish();
+            progress.push(output.text());
+        }
+        let mut state = PrettyState::default();
+        let styles = Styles::new(false);
+        petri_stream::format_pretty(&started, &styles, &mut state);
+        let pretty = petri_stream::format_pretty(&completed, &styles, &mut state).unwrap();
+        // Presentation must not change the machine-readable event or turn a
+        // stage outcome into the run's terminal result.
+        assert_eq!(petri_stream::raw_line(&completed).unwrap(), raw_before);
+        assert_eq!(petri_stream::exit_code_of(&completed), None);
+        cases.push(json!({
+            "status": status, "executed": executed,
+            "progress": progress[0], "verbose_progress": progress[1], "events": pretty
+        }));
+    }
+    insta::assert_snapshot!(serde_json::to_string_pretty(&cases).unwrap(), @r#"
+    [
+      {
+        "status": "skipped",
+        "executed": true,
+        "progress": "    ⊘ work  skipped  3s\n",
+        "verbose_progress": "    ⊘ work  skipped  3s\n",
+        "events": "04:43:03 ⊘ work  skipped"
+      },
+      {
+        "status": "success",
+        "executed": false,
+        "progress": "    ⊘ work  skipped  3s\n",
+        "verbose_progress": "    ⊘ work  skipped  3s\n",
+        "events": "04:43:03 ⊘ work  skipped"
+      },
+      {
+        "status": "success",
+        "executed": true,
+        "progress": "    ✓ work  3s\n",
+        "verbose_progress": "    ✓ work  3s\n",
+        "events": "04:43:03 ✓ work  3s"
+      },
+      {
+        "status": "partial_success",
+        "executed": true,
+        "progress": "    ✓ work  partial  3s\n",
+        "verbose_progress": "    ✓ work  partial  3s\n",
+        "events": "04:43:03 ✓ work  partial  3s"
+      },
+      {
+        "status": "failure",
+        "executed": true,
+        "progress": "    ✗ work\n    Error: failure\n",
+        "verbose_progress": "    ✗ work\n    Error: failure\n",
+        "events": "04:43:03 ✗ work  failure"
+      },
+      {
+        "status": "cancelled",
+        "executed": true,
+        "progress": "    ✗ work  3s\n",
+        "verbose_progress": "    ✗ work  3s\n",
+        "events": "04:43:03 ✗ work  cancelled"
+      },
+      {
+        "status": "unrecognized",
+        "executed": true,
+        "progress": "    ✗ work\n    Error: unrecognized\n",
+        "verbose_progress": "    ✗ work\n    Error: unrecognized\n",
+        "events": "04:43:03 ✗ work  unrecognized"
+      }
+    ]
+    "#);
+}
+
+#[test]
+fn skipped_stage_finishes_terminal_progress_with_a_neutral_label() {
+    for (status, executed) in [("skipped", true), ("success", false)] {
+        for verbose in [false, true] {
+            let renderer = ProgressRenderer::new_tty();
+            renderer.hide();
+            let mut ui = ProgressUI::with_renderer(renderer, verbose);
+            ui.handle_stream_item(&visit(1, "visit.started", json!({})));
+            let bar = ui.stage.active_stages["work"].spinner.clone();
+            ui.handle_stream_item(&completed(status, executed));
+            assert_eq!(console::strip_ansi_codes(&bar.message()), "⊘ work");
+            assert_eq!(console::strip_ansi_codes(&bar.prefix()), "skipped  3s");
+            assert!(bar.is_finished());
+            assert!(ui.stage.active_stages.is_empty());
+            ui.finish();
+        }
+    }
+}
+
+#[test]
+fn skipped_stage_is_dim_in_both_renderers_and_visible_in_plain_replay_without_a_start() {
+    let completed = completed("skipped", true);
+    let output = Output::default();
+    let renderer = ProgressRenderer::new_plain(Box::new(output.clone()), true);
+    let mut ui = ProgressUI::with_renderer(renderer, false);
+    ui.handle_stream_item(&completed);
+    ui.finish();
+    let progress = output.text();
+    let pretty =
+        petri_stream::format_pretty(&completed, &Styles::new(true), &mut PrettyState::default())
+            .unwrap();
+    for text in [&progress, &pretty] {
+        assert!(text.contains("\u{1b}[2m⊘\u{1b}[0m"), "{text:?}");
+        assert!(text.contains("\u{1b}[2mskipped\u{1b}[0m"), "{text:?}");
+        assert!(!text.contains('✗'), "{text:?}");
+    }
+    assert_eq!(
+        console::strip_ansi_codes(&progress),
+        "    ⊘ work  skipped  0ms\n"
+    );
+}

lib/apps/fabro-cli/src/commands/run/stage_presentation.rs

@@ -0,0 +1,28 @@
+//! Terminal styling for an already interpreted stage outcome. Decoding engine
+//! events and deciding run success remain the callers' responsibilities.
+
+use fabro_types::StageOutcome;
+use fabro_util::terminal::Styles;
+
+pub(super) struct StagePresentation {
+    pub(super) glyph: String,
+    pub(super) label: Option<String>,
+}
+
+impl StagePresentation {
+    pub(super) fn new(outcome: StageOutcome, styles: &Styles) -> Self {
+        let (glyph, label) = match outcome {
+            StageOutcome::Succeeded => (styles.green.apply_to("\u{2713}").to_string(), None),
+            StageOutcome::PartiallySucceeded => (
+                styles.green.apply_to("\u{2713}").to_string(),
+                Some(styles.yellow.apply_to("partial").to_string()),
+            ),
+            StageOutcome::Skipped => (
+                styles.dim.apply_to("\u{2298}").to_string(),
+                Some(styles.dim.apply_to("skipped").to_string()),
+            ),
+            StageOutcome::Failed { .. } => (styles.red.apply_to("\u{2717}").to_string(), None),
+        };
+        Self { glyph, label }
+    }
+}