From 4a5c89e5baf67a36828b8b18ca891c54993791aa Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Thu, 22 Oct 2026 15:18:52 -0400 Subject: [PATCH] Distinguish skipped stages in CLI progress --- lib/apps/fabro-cli/src/commands/run/mod.rs | 1 + .../src/commands/run/petri_stream.rs | 28 ++- .../src/commands/run/run_progress/mod.rs | 2 + .../run/run_progress/stage_display.rs | 41 ++-- .../src/commands/run/run_progress/tests.rs | 205 ++++++++++++++++++ .../src/commands/run/stage_presentation.rs | 28 +++ 6 files changed, 277 insertions(+), 28 deletions(-) create mode 100644 lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs create mode 100644 lib/apps/fabro-cli/src/commands/run/stage_presentation.rs diff --git a/lib/apps/fabro-cli/src/commands/run/mod.rs b/lib/apps/fabro-cli/src/commands/run/mod.rs index c527a8adf..b15125b94 100644 --- a/lib/apps/fabro-cli/src/commands/run/mod.rs +++ b/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)] diff --git a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs index 2748f5779..f3079d40b 100644 --- a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs +++ b/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), )) diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/mod.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/mod.rs index 85436006d..d57f59b70 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/mod.rs +++ b/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; diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/stage_display.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/stage_display.rs index b00a35820..f57fa3968 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/stage_display.rs +++ b/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::().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, diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs new file mode 100644 index 000000000..3a33ef351 --- /dev/null +++ b/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>>); + +impl Write for Output { + fn write(&mut self, buf: &[u8]) -> io::Result { + 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" + ); +} diff --git a/lib/apps/fabro-cli/src/commands/run/stage_presentation.rs b/lib/apps/fabro-cli/src/commands/run/stage_presentation.rs new file mode 100644 index 000000000..98266da18 --- /dev/null +++ b/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, +} + +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 } + } +}