From e30c9e549d84f59440a37350b89ac687b9721855 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Fri, 30 Oct 2026 15:39:24 -0400 Subject: [PATCH 1/3] Show selected edge labels on CLI transition arrows --- .../src/commands/run/petri_stream.rs | 33 +++-- .../src/commands/run/run_progress/event.rs | 1 + .../commands/run/run_progress/info_display.rs | 16 +-- .../src/commands/run/run_progress/mod.rs | 9 +- .../src/commands/run/run_progress/petri.rs | 3 +- .../src/commands/run/run_progress/tests.rs | 136 ++++++++++++++++++ 6 files changed, 178 insertions(+), 20 deletions(-) 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 f3079d40b..2e70dba49 100644 --- a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs +++ b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs @@ -163,15 +163,32 @@ impl<'a> PetriItem<'a> { self.value().pointer(pointer)?.as_str() } - /// The condition the applied route matched, as written: the edge a - /// `route.applied` record names is a key into the subject node's - /// `meta.edges`, whose entry carries the edge's `condition` when it - /// has one. - pub(crate) fn matched_condition(self) -> Option<&'a str> { + /// Metadata for the exact edge named by `route.applied`, not another + /// edge to the same target or the stage's preferred-label signal. + fn selected_edge(self) -> Option<&'a Value> { let edge = self.body()?.get("edge")?.as_u64()?; - self.node()? - .pointer(&format!("/meta/edges/{edge}/condition"))? + self.node()?.pointer(&format!("/meta/edges/{edge}")) + } + + pub(crate) fn selected_edge_label(self) -> Option<&'a str> { + self.selected_edge()? + .get("label")? .as_str() + .filter(|label| !label.is_empty()) + } + + /// The condition the applied route matched, as written. + pub(crate) fn matched_condition(self) -> Option<&'a str> { + self.selected_edge()?.get("condition")?.as_str() + } +} + +/// Put an edge label on the arrow in both progress and pretty events. +/// Escape controls so a workflow label cannot create extra terminal lines. +pub(crate) fn route_arrow(label: Option<&str>) -> String { + match label.filter(|label| !label.is_empty()) { + Some(label) => format!("── {} ──▶", label.escape_debug()), + None => "→".to_string(), } } @@ -427,7 +444,7 @@ pub(crate) fn format_pretty( Some(format!( "{ts} {} {} {} {}{}{}", styles.dim.apply_to(view.node_name().unwrap_or("?")), - styles.dim.apply_to("\u{2192}"), + styles.dim.apply_to(route_arrow(view.selected_edge_label())), target, styles.dim.apply_to(&transition), styles.dim.apply_to(detail), diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs index 24cfa271f..d5f9d173a 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs @@ -140,6 +140,7 @@ pub(super) enum ProgressEvent { LoopRestart { from_node: String, to_node: String, + label: Option, }, RunNotice { level: RunNoticeLevel, diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs index 420db28a1..d9c9c43af 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs @@ -4,6 +4,7 @@ use fabro_types::RunNoticeLevel; use super::renderer::ProgressRenderer; use super::styles; +use crate::commands::run::petri_stream; use crate::shared::{format_duration_ms, tilde_path}; pub(super) struct InfoDisplay { @@ -72,16 +73,11 @@ impl InfoDisplay { return; } - let detail = if let Some(condition) = condition { - format!(" [{condition}]") - } else if let Some(label) = label { - format!(" \"{label}\"") - } else { - String::new() - }; + let detail = condition.map_or_else(String::new, |condition| format!(" [{condition}]")); + let arrow = petri_stream::route_arrow(label); Self::insert_info_line( renderer, - &format!("\u{2192} {from_node} \u{2192} {to_node}{detail}"), + &format!("\u{2192} {from_node} {arrow} {to_node}{detail}"), ); } @@ -90,14 +86,16 @@ impl InfoDisplay { renderer: &ProgressRenderer, from_node: &str, to_node: &str, + label: Option<&str>, ) { if !self.verbose { return; } + let arrow = petri_stream::route_arrow(label); Self::insert_info_line( renderer, - &format!("\u{21ba} {from_node} \u{2192} {to_node} (loop restart)"), + &format!("\u{21ba} {from_node} {arrow} {to_node} (loop restart)"), ); } 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 ee02a3e6f..aa188ab23 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 @@ -318,8 +318,13 @@ impl ProgressUI { condition.as_deref(), ); } - ProgressEvent::LoopRestart { from_node, to_node } => { - self.info.on_loop_restart(renderer, &from_node, &to_node); + ProgressEvent::LoopRestart { + from_node, + to_node, + label, + } => { + self.info + .on_loop_restart(renderer, &from_node, &to_node, label.as_deref()); } ProgressEvent::RunNotice { level, diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs index 1ae1a5510..b84b4b7d8 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs @@ -156,12 +156,13 @@ pub(super) fn progress_events( vec![ProgressEvent::LoopRestart { from_node: node_id, to_node: to_node.to_string(), + label: view.selected_edge_label().map(str::to_owned), }] } else { vec![ProgressEvent::EdgeSelected { from_node: node_id, to_node: to_node.to_string(), - label: None, + label: view.selected_edge_label().map(str::to_owned), condition: derived .get("transition") .and_then(Value::as_str) 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 index 38068927e..7a174bfce 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs @@ -284,3 +284,139 @@ fn base_header_uses_only_the_commit_when_source_branch_is_unknown_or_ambiguous() } } } + +fn applied_route(from: &str, to: &str, label: &Value, back: bool) -> RunStreamItem { + RunStreamItem { + run_id: fixtures::RUN_1, + stream_seq: 1, + kind: RunStreamItemKind::Petri, + id: "execution 0/19/0".to_string(), + recorded_at: 0, + item: json!({ + "origin": "core", + "context": {"invocation": 0, "execution": 0}, + "subject": {"node": {"name": from, "meta": {"edges": { + "0": {"to": to, "label": "Not selected"}, + "7": {"to": to, "label": label} + }}}, "firing": 2}, + "record": {"seq": 19, "body": {"event": "route.applied", + "kind": "edge", "firing": 2, "group": 0, "edge": 7}}, + "derived": {"target": {"name": to}, "transition": "Continue", "back": back} + }), + } +} + +fn route_progress(item: &RunStreamItem, verbose: bool) -> String { + 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(item); + output.text() +} + +#[test] +fn selected_route_labels_render_on_arrows_in_progress_and_pretty_events() { + let mut transcript = String::new(); + for (from, to, label, back) in [ + ( + "Run tests", + "Open pull request", + json!("Tests passed"), + false, + ), + ( + "Review changes", + "Revise implementation", + json!("Changes requested"), + false, + ), + ("Approval", "Deploy", json!("Approved"), false), + ("Verify fix", "Investigate", json!("Still failing"), true), + ("work", "a", json!("yes"), false), + ("start", "work", Value::Null, false), + ("verify", "work", Value::Null, true), + ] { + let item = applied_route(from, to, &label, back); + let before = petri_stream::raw_line(&item).unwrap(); + transcript.push_str(&route_progress(&item, true)); + transcript.push_str( + &petri_stream::format_pretty(&item, &Styles::new(false), &mut PrettyState::default()) + .unwrap(), + ); + transcript.push('\n'); + assert_eq!(route_progress(&item, false), ""); + assert_eq!(petri_stream::raw_line(&item).unwrap(), before); + } + insta::assert_snapshot!(transcript, @r#" + → Run tests ── Tests passed ──▶ Open pull request + 00:00:00 Run tests ── Tests passed ──▶ Open pull request continue + → Review changes ── Changes requested ──▶ Revise implementation + 00:00:00 Review changes ── Changes requested ──▶ Revise implementation continue + → Approval ── Approved ──▶ Deploy + 00:00:00 Approval ── Approved ──▶ Deploy continue + ↺ Verify fix ── Still failing ──▶ Investigate (loop restart) + 00:00:00 Verify fix ── Still failing ──▶ Investigate continue (loop) + → work ── yes ──▶ a + 00:00:00 work ── yes ──▶ a continue + → start → work + 00:00:00 start → work continue + ↺ verify → work (loop restart) + 00:00:00 verify → work continue (loop) + "#); +} + +#[test] +fn selected_route_labels_coexist_with_transition_and_condition_details() { + let mut item = applied_route("verify", "finish", &json!("Tests passed"), false); + item.item["subject"]["node"]["meta"]["edges"]["7"]["condition"] = json!("outcome=succeeded"); + item.item["derived"]["transition"] = json!("Restart"); + let pretty = + petri_stream::format_pretty(&item, &Styles::new(false), &mut PrettyState::default()) + .unwrap(); + insta::assert_snapshot!(format!("{}{pretty}", route_progress(&item, true)), @r#" + → verify ── Tests passed ──▶ finish [restart] + 00:00:00 verify ── Tests passed ──▶ finish restart when outcome=succeeded + "#); +} + +#[test] +fn unavailable_selected_route_labels_preserve_unlabeled_output() { + let baseline = applied_route("work", "next", &Value::Null, false); + let mut cases = vec![ + applied_route("work", "next", &json!(""), false), + applied_route("work", "next", &json!(42), false), + ]; + let mut missing_edge = baseline.clone(); + missing_edge.item["record"]["body"]["edge"] = json!(99); + cases.push(missing_edge); + let mut missing_metadata = baseline.clone(); + missing_metadata.item["subject"]["node"]["meta"] = json!({}); + cases.push(missing_metadata); + let mut no_edge = baseline.clone(); + no_edge.item["record"]["body"] = json!({"event": "route.applied", "kind": "jump"}); + cases.push(no_edge); + for item in cases { + assert_eq!(route_progress(&item, true), route_progress(&baseline, true)); + assert_eq!( + petri_stream::format_pretty(&item, &Styles::new(false), &mut PrettyState::default()), + petri_stream::format_pretty( + &baseline, + &Styles::new(false), + &mut PrettyState::default() + ), + ); + } +} + +#[test] +fn selected_route_labels_escape_controls_and_preserve_unicode() { + let item = applied_route( + "review", + "revise", + &json!("Réessayer \"tests\"\n\u{1b}[31m"), + false, + ); + insta::assert_snapshot!(route_progress(&item, true), @r#" + → review ── Réessayer \"tests\"\n\u{1b}[31m ──▶ revise + "#); +} From fe13561ca524cdbfc6e585acf9dff9d0d9b7e783 Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Mon, 2 Nov 2026 13:28:54 -0500 Subject: [PATCH 2/3] Fix PID readiness race in remote workflow tests --- lib/apps/fabro-cli/src/commands/run/remote_workflow.rs | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs index 8bec413d7..a1742ce55 100644 --- a/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs +++ b/lib/apps/fabro-cli/src/commands/run/remote_workflow.rs @@ -752,7 +752,10 @@ mod tests { #[tokio::test] async fn remote_workflow_dropped_waiter_cancels_child_before_checkout_cleanup() { - let (fake, git) = fake_git("printf '%s' $$ > pid; exec /bin/sleep 60"); + // Publish the PID only after writing it: file existence is the readiness + // signal. + let (fake, git) = + fake_git("printf '%s' $$ > pid.tmp; /bin/mv pid.tmp pid; exec /bin/sleep 60"); let checkout = tempfile::tempdir().unwrap(); let path = checkout.path().to_path_buf(); let worker = tokio::spawn(async move { @@ -1154,7 +1157,8 @@ mod tests { #[tokio::test] async fn remote_workflow_timeout_and_cancel_reap_owned_children() { for timeout in [true, false] { - let (root, mut git) = fake_git("printf '%s' $$ > pid; exec /bin/sleep 60"); + let (root, mut git) = + fake_git("printf '%s' $$ > pid.tmp; /bin/mv pid.tmp pid; exec /bin/sleep 60"); git.timeout = Duration::from_secs(2); let cancel = CancellationToken::new(); let trigger = async { From f6001562112636a5ad37d0559ec1ad5af35cd71c Mon Sep 17 00:00:00 2001 From: Scott Werner Date: Wed, 4 Nov 2026 11:30:14 -0500 Subject: [PATCH 3/3] Render compact edge choices beneath CLI decision stages --- Cargo.lock | 28 +++ lib/apps/fabro-cli/Cargo.toml | 1 + .../src/commands/run/petri_stream.rs | 32 +-- .../src/commands/run/run_progress/event.rs | 9 +- .../commands/run/run_progress/info_display.rs | 9 +- .../src/commands/run/run_progress/mod.rs | 33 ++- .../src/commands/run/run_progress/petri.rs | 14 ++ .../src/commands/run/run_progress/renderer.rs | 32 ++- .../run/run_progress/stage_display.rs | 45 ++++ .../src/commands/run/run_progress/styles.rs | 1 + .../src/commands/run/run_progress/tests.rs | 210 ++++++++++++++++-- 11 files changed, 371 insertions(+), 43 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index e7668185a..c406f16bf 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -182,6 +182,12 @@ version = "0.5.1" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "7d902e3d592a523def97af8f317b08ce16b7ab854c1985a0c671e6f15cebc236" +[[package]] +name = "arrayvec" +version = "0.7.8" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "d3fb67a6e08acf24fdeccbac2cb6ac4305825bd1f117462e0e6f2f193345ad56" + [[package]] name = "assert-json-diff" version = "2.0.2" @@ -3981,6 +3987,7 @@ dependencies = [ "portable-atomic", "unicode-width 0.2.2", "unit-prefix", + "vt100", "web-time", ] @@ -8240,6 +8247,27 @@ version = "0.8.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "5c3082ca00d5a5ef149bb8b555a72ae84c9c59f7250f013ac822ac2e49b19c64" +[[package]] +name = "vt100" +version = "0.16.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "054ff75fb8fa83e609e685106df4faeffdf3a735d3c74ebce97ec557d5d36fd9" +dependencies = [ + "itoa", + "unicode-width 0.2.2", + "vte", +] + +[[package]] +name = "vte" +version = "0.15.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "a5924018406ce0063cd67f8e008104968b74b563ee1b85dde3ed1f7cb87d3dbd" +dependencies = [ + "arrayvec", + "memchr", +] + [[package]] name = "wait-timeout" version = "0.2.1" diff --git a/lib/apps/fabro-cli/Cargo.toml b/lib/apps/fabro-cli/Cargo.toml index 89f7cb644..a4d405092 100644 --- a/lib/apps/fabro-cli/Cargo.toml +++ b/lib/apps/fabro-cli/Cargo.toml @@ -103,6 +103,7 @@ fabro-build-support = { path = "../../foundation/build-support" } chrono = { workspace = true } [dev-dependencies] +indicatif = { workspace = true, features = ["in_memory"] } shlex = "1" ulid.workspace = true regex.workspace = true 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 2e70dba49..a7053c081 100644 --- a/lib/apps/fabro-cli/src/commands/run/petri_stream.rs +++ b/lib/apps/fabro-cli/src/commands/run/petri_stream.rs @@ -183,15 +183,6 @@ impl<'a> PetriItem<'a> { } } -/// Put an edge label on the arrow in both progress and pretty events. -/// Escape controls so a workflow label cannot create extra terminal lines. -pub(crate) fn route_arrow(label: Option<&str>) -> String { - match label.filter(|label| !label.is_empty()) { - Some(label) => format!("── {} ──▶", label.escape_debug()), - None => "→".to_string(), - } -} - /// Whether the item is the platform record of the run's terminal lifecycle /// transition, which ends the attached stream. pub(crate) fn is_terminal_lifecycle(item: &RunStreamItem) -> bool { @@ -435,6 +426,22 @@ pub(crate) fn format_pretty( .and_then(Value::as_bool) .unwrap_or(false); let detail = if back { " (loop)" } else { "" }; + let edge_label = view.selected_edge_label(); + let arrow = if edge_label.is_some() && back { + "↺" + } else { + "→" + }; + let route_detail = if let Some(label) = edge_label { + let transition = if transition == "continue" || transition.is_empty() { + String::new() + } else { + format!(" {transition}") + }; + format!(" [{}]{transition}", label.escape_debug()) + } else { + format!(" {transition}{detail}") + }; // The condition the edge matched, when it has one. let condition = view .matched_condition() @@ -442,12 +449,11 @@ pub(crate) fn format_pretty( // Petri records the route after the next visit started, so the // line names both ends of the edge. Some(format!( - "{ts} {} {} {} {}{}{}", + "{ts} {} {} {}{}{}", styles.dim.apply_to(view.node_name().unwrap_or("?")), - styles.dim.apply_to(route_arrow(view.selected_edge_label())), + styles.dim.apply_to(arrow), target, - styles.dim.apply_to(&transition), - styles.dim.apply_to(detail), + styles.dim.apply_to(&route_detail), styles.dim.apply_to(&condition), )) } diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs index d5f9d173a..45d964cee 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/event.rs @@ -30,9 +30,10 @@ pub(super) enum ProgressEvent { base_sha: Option, }, StageStarted { - node_id: String, - name: String, - script: Option, + node_id: String, + name: String, + script: Option, + route_key: Option, }, StageCompleted { node_id: String, @@ -132,12 +133,14 @@ pub(super) enum ProgressEvent { turns_used: u64, }, EdgeSelected { + route_key: Option, from_node: String, to_node: String, label: Option, condition: Option, }, LoopRestart { + route_key: Option, from_node: String, to_node: String, label: Option, diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs index d9c9c43af..db58e3810 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/info_display.rs @@ -4,7 +4,6 @@ use fabro_types::RunNoticeLevel; use super::renderer::ProgressRenderer; use super::styles; -use crate::commands::run::petri_stream; use crate::shared::{format_duration_ms, tilde_path}; pub(super) struct InfoDisplay { @@ -74,10 +73,10 @@ impl InfoDisplay { } let detail = condition.map_or_else(String::new, |condition| format!(" [{condition}]")); - let arrow = petri_stream::route_arrow(label); + let label = label.map_or_else(String::new, |label| format!(" [{}]", label.escape_debug())); Self::insert_info_line( renderer, - &format!("\u{2192} {from_node} {arrow} {to_node}{detail}"), + &format!("\u{2192} {from_node} → {to_node}{label}{detail}"), ); } @@ -92,10 +91,10 @@ impl InfoDisplay { return; } - let arrow = petri_stream::route_arrow(label); + let label = label.map_or_else(String::new, |label| format!(" [{}]", label.escape_debug())); Self::insert_info_line( renderer, - &format!("\u{21ba} {from_node} {arrow} {to_node} (loop restart)"), + &format!("\u{21ba} {from_node} → {to_node}{label} (loop restart)"), ); } 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 aa188ab23..c8e0b0168 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 @@ -112,9 +112,15 @@ impl ProgressUI { node_id, name, script, + route_key, } => { - self.stage - .on_stage_started(renderer, &node_id, &name, script.as_deref()); + self.stage.on_stage_started( + renderer, + &node_id, + &name, + script.as_deref(), + route_key, + ); } ProgressEvent::StageCompleted { node_id, @@ -305,11 +311,24 @@ impl ProgressUI { ); } ProgressEvent::EdgeSelected { + route_key, from_node, to_node, label, condition, } => { + let message = label.as_ref().map(|label| { + let detail = condition + .as_ref() + .map_or_else(String::new, |value| format!(" [{value}]")); + format!("└─ {}{detail}", label.escape_debug()) + }); + if self + .stage + .on_route(renderer, route_key.as_deref(), message.as_deref()) + { + return; + } self.info.on_edge_selected( renderer, &from_node, @@ -319,10 +338,20 @@ impl ProgressUI { ); } ProgressEvent::LoopRestart { + route_key, from_node, to_node, label, } => { + let message = label + .as_ref() + .map(|label| format!("↺ {}", label.escape_debug())); + if self + .stage + .on_route(renderer, route_key.as_deref(), message.as_deref()) + { + return; + } self.info .on_loop_restart(renderer, &from_node, &to_node, label.as_deref()); } diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs index b84b4b7d8..72619a00f 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/petri.rs @@ -41,6 +41,18 @@ pub(super) fn progress_events( node_id, name: label, script: None, + route_key: view + .node() + .and_then(|node| node.pointer("/meta/edges")) + .and_then(Value::as_object) + .filter(|edges| { + edges.values().any(|edge| { + edge.get("label") + .and_then(Value::as_str) + .is_some_and(|label| !label.is_empty()) + }) + }) + .and_then(|_| view.stage_key()), }] } else if view.node_kind() == Some("parallel.branch") { vec![ProgressEvent::ParallelBranchStarted { branch: node_id }] @@ -154,12 +166,14 @@ pub(super) fn progress_events( .unwrap_or(false); if back { vec![ProgressEvent::LoopRestart { + route_key: view.stage_key(), from_node: node_id, to_node: to_node.to_string(), label: view.selected_edge_label().map(str::to_owned), }] } else { vec![ProgressEvent::EdgeSelected { + route_key: view.stage_key(), from_node: node_id, to_node: to_node.to_string(), label: view.selected_edge_label().map(str::to_owned), diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/renderer.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/renderer.rs index 4e4bc6654..33213b17e 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/renderer.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/renderer.rs @@ -4,6 +4,7 @@ terminal progress lines" )] +use std::cell::Cell; use std::io::Write; use std::sync::Mutex; @@ -18,26 +19,29 @@ enum RendererInner { } pub(super) struct ProgressRenderer { - inner: RendererInner, - styles: Styles, + inner: RendererInner, + styles: Styles, + printed_lines: Cell, } impl ProgressRenderer { pub(super) fn new_tty() -> Self { Self { - inner: RendererInner::Tty { + inner: RendererInner::Tty { multi: MultiProgress::new(), }, - styles: Styles::new(console::colors_enabled_stderr()), + styles: Styles::new(console::colors_enabled_stderr()), + printed_lines: Cell::new(0), } } pub(super) fn new_plain(out: Box, colors: bool) -> Self { Self { - inner: RendererInner::Plain { + inner: RendererInner::Plain { out: Mutex::new(out), }, - styles: Styles::new(colors), + styles: Styles::new(colors), + printed_lines: Cell::new(0), } } @@ -59,6 +63,22 @@ impl ProgressRenderer { if let RendererInner::Plain { out } = &self.inner { let mut out = out.lock().expect("plain renderer lock poisoned"); let _ = writeln!(out, "{}{message}", " ".repeat(indent)); + self.printed_lines.set(self.printed_lines.get() + 1); + } + } + + pub(super) fn printed_lines(&self) -> usize { + self.printed_lines.get() + } + + #[cfg(test)] + pub(super) fn with_draw_target(target: ProgressDrawTarget) -> Self { + Self { + inner: RendererInner::Tty { + multi: MultiProgress::with_draw_target(target), + }, + styles: Styles::new(false), + printed_lines: Cell::new(0), } } 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 f57fa3968..59f5593f0 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 @@ -33,6 +33,7 @@ pub(super) struct ToolCallEntry { #[derive(Debug)] pub(super) struct ActiveStage { + route_key: Option, pub(super) display_name: String, pub(super) has_model: bool, pub(super) spinner: ProgressBar, @@ -52,6 +53,9 @@ impl ActiveStage { } pub(super) struct StageDisplay { + /// Keep the completed visit's last visible line until its route arrives. + /// Keys include the execution and firing, so revisits cannot steal a route. + route_anchors: HashMap, verbose: bool, pub(super) active_stages: HashMap, pub(super) stage_counts: HashMap, @@ -63,6 +67,7 @@ pub(super) struct StageDisplay { impl StageDisplay { pub(super) fn new(verbose: bool) -> Self { Self { + route_anchors: HashMap::new(), verbose, active_stages: HashMap::new(), stage_counts: HashMap::new(), @@ -73,6 +78,7 @@ impl StageDisplay { } pub(super) fn finish(&mut self) { + self.route_anchors.clear(); for (_node_id, stage) in self.active_stages.drain() { if let Some(bar) = stage.compaction_bar { bar.finish_and_clear(); @@ -97,6 +103,7 @@ impl StageDisplay { node_id: &str, name: &str, script: Option<&str>, + route_key: Option, ) { self.stage_counts.insert(node_id.to_string(), (0, 0)); let display_name = match script { @@ -121,6 +128,7 @@ impl StageDisplay { bar.enable_steady_tick(Duration::from_millis(100)); } self.active_stages.insert(node_id.to_string(), ActiveStage { + route_key, display_name, has_model: false, spinner: bar, @@ -675,6 +683,12 @@ impl StageDisplay { return; }; + let anchor = if self.verbose && stage.route_key.is_some() { + Some(stage.last_bar().clone()) + } else { + None + }; + if let Some(bar) = stage.compaction_bar { bar.finish_and_clear(); } @@ -698,6 +712,37 @@ impl StageDisplay { } else { Self::print_plain_stage_completion(renderer, name, glyph, prefix); } + if let (Some(key), Some(anchor)) = (stage.route_key, anchor) { + self.route_anchors + .insert(key, (anchor, renderer.printed_lines())); + } + } + + /// A TTY can insert beneath an earlier visit even after other stages start. + /// A plain transcript can only abbreviate when that visit is still the + /// preceding line; otherwise the caller prints a self-contained route. + pub(super) fn on_route( + &mut self, + renderer: &ProgressRenderer, + key: Option<&str>, + message: Option<&str>, + ) -> bool { + let Some((anchor, position)) = key.and_then(|key| self.route_anchors.remove(key)) else { + return false; + }; + let Some(message) = message else { + return false; + }; + if renderer.is_tty() { + let bar = renderer.insert_after(&anchor); + bar.set_style(styles::style_route()); + bar.finish_with_message(message.to_string()); + } else if renderer.printed_lines() == position { + renderer.print_line(8, message); + } else { + return false; + } + true } fn print_plain_stage_completion( diff --git a/lib/apps/fabro-cli/src/commands/run/run_progress/styles.rs b/lib/apps/fabro-cli/src/commands/run/run_progress/styles.rs index 27f389170..ab9883fc7 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/styles.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/styles.rs @@ -32,6 +32,7 @@ cached_style!(style_tool_done, " {wide_msg} {prefix:.dim}"); cached_style!(style_subagent_info, " {wide_msg}"); cached_style!(style_branch_done, " {wide_msg} {prefix:.dim}"); cached_style!(style_static_dim, " {wide_msg:.dim}"); +cached_style!(style_route, " {wide_msg:.dim}"); cached_style!(style_empty, " "); pub(super) fn green_check(styles: &Styles) -> String { 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 index 7a174bfce..6a1e7b013 100644 --- a/lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs +++ b/lib/apps/fabro-cli/src/commands/run/run_progress/tests.rs @@ -10,6 +10,7 @@ use fabro_types::{ DirtyStatus, ForkSourceRef, GitContext, GitRunTarget, RunStreamItemKind, fixtures, test_support, }; use fabro_util::terminal::Styles; +use indicatif::{InMemoryTerm, ProgressDrawTarget}; use serde_json::{Value, json}; use super::*; @@ -310,12 +311,30 @@ fn route_progress(item: &RunStreamItem, verbose: bool) -> String { 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(&route_visit(item, "visit.started", None)); + ui.handle_stream_item(&route_visit(item, "visit.completed", None)); + output.0.lock().unwrap().clear(); ui.handle_stream_item(item); output.text() } +fn route_visit(route: &RunStreamItem, event: &str, label: Option<&str>) -> RunStreamItem { + let mut item = visit( + 0, + event, + json!({"outcome": {"status": "success"}, "executed": true}), + ); + item.item["subject"] = route.item["subject"].clone(); + item.item["subject"]["node"]["meta"]["kind"] = json!("agent"); + item.item["subject"]["visit"] = json!(1); + if let Some(label) = label { + item.item["subject"]["node"]["meta"]["label"] = json!(label); + } + item +} + #[test] -fn selected_route_labels_render_on_arrows_in_progress_and_pretty_events() { +fn selected_route_labels_render_beneath_stages_and_in_pretty_events() { let mut transcript = String::new(); for (from, to, label, back) in [ ( @@ -348,16 +367,16 @@ fn selected_route_labels_render_on_arrows_in_progress_and_pretty_events() { assert_eq!(petri_stream::raw_line(&item).unwrap(), before); } insta::assert_snapshot!(transcript, @r#" - → Run tests ── Tests passed ──▶ Open pull request - 00:00:00 Run tests ── Tests passed ──▶ Open pull request continue - → Review changes ── Changes requested ──▶ Revise implementation - 00:00:00 Review changes ── Changes requested ──▶ Revise implementation continue - → Approval ── Approved ──▶ Deploy - 00:00:00 Approval ── Approved ──▶ Deploy continue - ↺ Verify fix ── Still failing ──▶ Investigate (loop restart) - 00:00:00 Verify fix ── Still failing ──▶ Investigate continue (loop) - → work ── yes ──▶ a - 00:00:00 work ── yes ──▶ a continue + └─ Tests passed + 00:00:00 Run tests → Open pull request [Tests passed] + └─ Changes requested + 00:00:00 Review changes → Revise implementation [Changes requested] + └─ Approved + 00:00:00 Approval → Deploy [Approved] + ↺ Still failing + 00:00:00 Verify fix ↺ Investigate [Still failing] + └─ yes + 00:00:00 work → a [yes] → start → work 00:00:00 start → work continue ↺ verify → work (loop restart) @@ -374,8 +393,8 @@ fn selected_route_labels_coexist_with_transition_and_condition_details() { petri_stream::format_pretty(&item, &Styles::new(false), &mut PrettyState::default()) .unwrap(); insta::assert_snapshot!(format!("{}{pretty}", route_progress(&item, true)), @r#" - → verify ── Tests passed ──▶ finish [restart] - 00:00:00 verify ── Tests passed ──▶ finish restart when outcome=succeeded + └─ Tests passed [restart] + 00:00:00 verify → finish [Tests passed] restart when outcome=succeeded "#); } @@ -417,6 +436,169 @@ fn selected_route_labels_escape_controls_and_preserve_unicode() { false, ); insta::assert_snapshot!(route_progress(&item, true), @r#" - → review ── Réessayer \"tests\"\n\u{1b}[31m ──▶ revise + └─ Réessayer \"tests\"\n\u{1b}[31m "#); } + +fn review_route(back: bool) -> RunStreamItem { + applied_route( + "review_images", + if back { "render_images" } else { "render_html" }, + &json!(if back { + "Regenerate images" + } else { + "Keep images" + }), + back, + ) +} + +fn target_route(source: &RunStreamItem) -> RunStreamItem { + let target = source.item["derived"]["target"]["name"].as_str().unwrap(); + let mut route = applied_route(target, "exit", &Value::Null, false); + route.item["subject"]["firing"] = json!(3); + route.item["subject"]["node"]["meta"]["edges"] = json!({}); + route +} + +#[test] +fn decision_lines_follow_display_labels_or_node_names_in_plain_progress() { + let mut transcript = String::new(); + for back in [false, true] { + for labels in [true, false] { + let output = Output::default(); + let renderer = ProgressRenderer::new_plain(Box::new(output.clone()), false); + let mut ui = ProgressUI::with_renderer(renderer, true); + let source = review_route(back); + let target = target_route(&source); + let source_label = labels.then_some("Review images"); + let target_label = labels.then_some(if back { + "Render 12 images" + } else { + "Render HTML reading" + }); + ui.handle_stream_item(&route_visit(&source, "visit.started", source_label)); + ui.handle_stream_item(&route_visit(&source, "visit.completed", source_label)); + // The route follows the target's start in Petri's stream. + ui.handle_stream_item(&route_visit(&target, "visit.started", target_label)); + ui.handle_stream_item(&source); + ui.handle_stream_item(&route_visit(&target, "visit.completed", target_label)); + ui.finish(); + transcript.push_str(&output.text()); + } + } + insta::assert_snapshot!(transcript, @r#" + ✓ Review images 0ms + └─ Keep images + ✓ Render HTML reading 0ms + ✓ review_images 0ms + └─ Keep images + ✓ render_html 0ms + ✓ Review images 0ms + ↺ Regenerate images + ✓ Render 12 images 0ms + ✓ review_images 0ms + ↺ Regenerate images + ✓ render_images 0ms + "#); +} + +#[test] +fn tty_decision_stays_with_source_when_another_stage_has_already_finished() { + for back in [false, true] { + let term = InMemoryTerm::new(24, 100); + let renderer = ProgressRenderer::with_draw_target(ProgressDrawTarget::term_like(Box::new( + term.clone(), + ))); + let mut ui = ProgressUI::with_renderer(renderer, true); + let source = review_route(back); + let target = target_route(&source); + ui.handle_stream_item(&route_visit( + &source, + "visit.started", + Some("Review images"), + )); + ui.handle_stream_item(&route_visit( + &source, + "visit.completed", + Some("Review images"), + )); + ui.handle_stream_item(&route_visit(&target, "visit.started", Some("Next stage"))); + ui.handle_stream_item(&route_visit(&target, "visit.completed", Some("Next stage"))); + ui.handle_stream_item(&source); + ui.finish(); + let screen = term.contents(); + let decision = if back { + "↺ Regenerate images" + } else { + "└─ Keep images" + }; + let lines: Vec<_> = screen + .lines() + .filter(|line| !line.trim().is_empty()) + .collect(); + assert_eq!(lines.len(), 3, "{screen}"); + assert!(lines[0].contains("✓ Review images"), "{screen}"); + assert_eq!(lines[1], format!(" {decision}")); + assert!(lines[2].contains("✓ Next stage"), "{screen}"); + assert!(!screen.contains("review_images")); + } +} + +#[test] +fn detached_or_interleaved_plain_routes_keep_context() { + for with_source in [false, true] { + let output = Output::default(); + let renderer = ProgressRenderer::new_plain(Box::new(output.clone()), false); + let mut ui = ProgressUI::with_renderer(renderer, true); + let source = review_route(false); + if with_source { + ui.handle_stream_item(&route_visit( + &source, + "visit.started", + Some("Review images"), + )); + ui.handle_stream_item(&route_visit( + &source, + "visit.completed", + Some("Review images"), + )); + // An append-only transcript cannot move the decision above this line. + ui.renderer.print_line(4, "Other activity"); + } + output.0.lock().unwrap().clear(); + ui.handle_stream_item(&source); + insta::allow_duplicates! { + insta::assert_snapshot!(output.text(), @" → review_images → render_html [Keep images]"); + } + } +} + +#[test] +fn tty_self_loop_decisions_attach_to_the_correct_visit() { + let term = InMemoryTerm::new(24, 100); + let renderer = + ProgressRenderer::with_draw_target(ProgressDrawTarget::term_like(Box::new(term.clone()))); + let mut ui = ProgressUI::with_renderer(renderer, true); + let first = applied_route("review", "review", &json!("Regenerate images"), true); + let mut second = applied_route("review", "exit", &json!("Keep images"), false); + second.item["subject"]["firing"] = json!(3); + for (route, name) in [(&first, "First review"), (&second, "Second review")] { + ui.handle_stream_item(&route_visit(route, "visit.started", Some(name))); + ui.handle_stream_item(&route_visit(route, "visit.completed", Some(name))); + } + // Both completed visits share a node name; routes arrive in reverse order. + ui.handle_stream_item(&second); + ui.handle_stream_item(&first); + ui.finish(); + let screen = term.contents(); + let lines: Vec<_> = screen + .lines() + .filter(|line| !line.trim().is_empty()) + .collect(); + assert_eq!(lines.len(), 4, "{screen}"); + assert!(lines[0].contains("✓ First review"), "{screen}"); + assert_eq!(lines[1], " ↺ Regenerate images"); + assert!(lines[2].contains("✓ Second review"), "{screen}"); + assert_eq!(lines[3], " └─ Keep images"); +}