From 6d40671f28de0a032e79e873259e5a71759c1db7 Mon Sep 17 00:00:00 2001 From: Elijah Voigt Date: Sat, 28 Feb 2026 21:29:02 -0800 Subject: [PATCH] fix(nbd): fix graph orientation to show goals at root, prerequisites as leaves MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Roots are now tickets with no dependents (nobody depends on them — top-level goals), and the ASCII tree traverses dependency edges so prerequisites appear indented beneath the goal that needs them. JSON edges are now {from: dependent, to: dependency} rather than {from: blocker, to: blocked}. All graph-related unit and integration tests updated to match the new semantics. Closes #668150 Co-Authored-By: Claude Sonnet 4.6 --- nbd/.nbd/tickets/668150.md | 2 +- nbd/src/display.rs | 23 ++++++------- nbd/src/graph.rs | 62 ++++++++++++++++++----------------- nbd/src/tests.rs | 66 +++++++++++++++++++++----------------- nbd/tests/integration.rs | 47 ++++++++++++++++----------- 5 files changed, 109 insertions(+), 91 deletions(-) diff --git a/nbd/.nbd/tickets/668150.md b/nbd/.nbd/tickets/668150.md index 902f33f..84dc51e 100644 --- a/nbd/.nbd/tickets/668150.md +++ b/nbd/.nbd/tickets/668150.md @@ -1,7 +1,7 @@ +++ title = "Fix graph orientation: show goals at root, prerequisites as leaves" priority = 7 -status = "todo" +status = "done" ticket_type = "bug" dependencies = [] +++ diff --git a/nbd/src/display.rs b/nbd/src/display.rs index 6d9a2d2..dce4477 100644 --- a/nbd/src/display.rs +++ b/nbd/src/display.rs @@ -348,16 +348,17 @@ pub fn print_diff(old: &Ticket, new: &Ticket) { /// Format the full dependency forest as an ASCII tree string. /// -/// Roots (tickets with no in-graph dependencies) appear at column 0, sorted -/// by priority descending. Each root's dependents (tickets that list it as a -/// dependency) are indented below using box-drawing characters: +/// Roots (tickets that no other ticket depends on — top-level goals) appear +/// at column 0, sorted by priority descending. Each root's dependencies +/// (prerequisites it needs completed first) are indented below using +/// box-drawing characters: /// /// ```text /// a3f9c2 [todo] Fix login bug /// ├── b7d41e [in_progress] Add rate limiting /// │ └── c9e823 [todo] Write tests /// └── d1f302 [done] Update docs -/// e4a781 [todo] New feature (no deps) +/// e4a781 [todo] New feature (no prereqs) /// ``` /// /// Nodes that appear in multiple subtrees are marked `*` on subsequent @@ -378,8 +379,8 @@ pub fn print_graph(graph: &TicketGraph<'_>) { /// Format the subtree rooted at `root_id` as an ASCII tree string. /// -/// Renders `root_id` and every ticket that transitively depends on it -/// (via dependent edges), using the same box-drawing format as +/// Renders `root_id` and every ticket it transitively depends on +/// (via dependency edges), using the same box-drawing format as /// [`format_graph`]. Returns an empty string when `root_id` is not in the /// graph. pub fn format_subtree(graph: &TicketGraph<'_>, root_id: &str) -> String { @@ -398,7 +399,7 @@ pub fn print_subtree(graph: &TicketGraph<'_>, root_id: &str) { // ── Internal graph helpers ──────────────────────────────────────────────────── -/// Recursively render a single node and its dependents into `out`. +/// Recursively render a single node and its dependencies into `out`. /// /// Parameters: /// - `prefix` — the indentation printed before `connector` on this node's line. @@ -429,11 +430,11 @@ fn render_node( // Extract the node data we need, cloning so we can drop the borrow before // recursing (the recursive call needs a mutable `visited`). - let (status_s, title, dependents) = match graph.get_node(id) { + let (status_s, title, dependencies) = match graph.get_node(id) { Some(node) => { let s = status_str(&node.ticket.status); let t = node.ticket.title.clone(); - let d: Vec = node.dependents.iter().map(|s| s.to_string()).collect(); + let d: Vec = node.dependencies.iter().map(|s| s.to_string()).collect(); (s, t, d) } None => return, @@ -444,8 +445,8 @@ fn render_node( &format!("{prefix}{connector}{id} [{status_s}] {title}"), ); - let n = dependents.len(); - for (i, dep_id) in dependents.iter().enumerate() { + let n = dependencies.len(); + for (i, dep_id) in dependencies.iter().enumerate() { let is_last = i == n - 1; // The child's connector on its own line. let child_connector = if is_last { "└── " } else { "├── " }; diff --git a/nbd/src/graph.rs b/nbd/src/graph.rs index d3828e1..8f32488 100644 --- a/nbd/src/graph.rs +++ b/nbd/src/graph.rs @@ -10,12 +10,12 @@ //! - A is a **dependency** of B (A must be done before B) //! - B is a **dependent** of A (B is waiting on A) //! -//! The ASCII tree is rendered with **roots** (tickets with no in-graph -//! dependencies) at the top and their **dependents** indented below — showing -//! "what is blocked by this ticket?". +//! The ASCII tree is rendered with **roots** (tickets that no other ticket +//! depends on — the top-level goals) at the top and their **dependencies** +//! indented below — showing "what does this ticket need to be done first?". //! -//! JSON edges use `{"from": , "to": }` — i.e. the blocking -//! ticket is `from` and the waiting ticket is `to`. +//! JSON edges use `{"from": , "to": }` — i.e. the +//! ticket that is waiting is `from` and the prerequisite ticket is `to`. use std::collections::{HashMap, HashSet}; @@ -48,18 +48,19 @@ pub struct GraphNode<'a> { /// /// Only tickets present in the graph are listed; dangling references in /// [`Ticket::dependencies`] are silently ignored. + /// + /// These are the visual "children" in the ASCII dependency tree — the + /// prerequisites that must be completed before this ticket. pub dependencies: Vec<&'a str>, /// IDs of tickets that list this ticket as a dependency (reverse edges). - /// - /// These are the visual "children" in the ASCII dependency tree. pub dependents: Vec<&'a str>, } /// A directed dependency graph built from a flat list of tickets. /// -/// Build with [`TicketGraph::build`]. Roots (entry points) are returned by +/// Build with [`TicketGraph::build`]. Roots (top-level goals) are returned by /// [`TicketGraph::roots`]. Use [`TicketGraph::subtree`] to extract the IDs -/// reachable from a specific ticket via its dependents, and +/// reachable from a specific ticket via its dependencies, and /// [`TicketGraph::to_json_value`] for machine-readable output. pub struct TicketGraph<'a> { /// Nodes keyed by ticket ID. @@ -128,17 +129,18 @@ impl<'a> TicketGraph<'a> { TicketGraph { nodes, ids } } - /// Return tickets with no in-graph dependencies, sorted by priority descending. + /// Return tickets that no other ticket depends on, sorted by priority descending. /// - /// These are the entry points for the ASCII dependency tree renderer. - /// A ticket is a root when its `dependencies` list (after filtering to - /// in-graph tickets only) is empty. + /// These are the top-level goals for the ASCII dependency tree renderer. + /// A ticket is a root when its `dependents` list is empty — nothing in the + /// graph is waiting on it, so it represents an end goal rather than a + /// prerequisite. pub fn roots(&self) -> Vec<&'a Ticket> { let mut roots: Vec<&'a Ticket> = self .ids .iter() .filter_map(|id| self.nodes.get(id)) - .filter(|node| node.dependencies.is_empty()) + .filter(|node| node.dependents.is_empty()) .map(|node| node.ticket) .collect(); roots.sort_by(|a, b| b.priority.cmp(&a.priority)); @@ -150,13 +152,13 @@ impl<'a> TicketGraph<'a> { self.nodes.get(id) } - /// Return all ticket IDs reachable from `root_id` via dependent edges, + /// Return all ticket IDs reachable from `root_id` via dependency edges, /// in depth-first order, including `root_id` itself. /// - /// "Reachable via dependents" means: `root_id`, plus every ticket that - /// depends on `root_id`, plus every ticket that depends on those, and so - /// on. This answers "what tickets are blocked (directly or transitively) - /// by `root_id`?". + /// "Reachable via dependencies" means: `root_id`, plus every ticket that + /// `root_id` depends on, plus every ticket those depend on, and so on. + /// This answers "what tickets does `root_id` need (directly or + /// transitively)?". /// /// Cycles are handled by a visited set — each ID appears at most once. /// Returns an empty `Vec` when `root_id` is not in the graph. @@ -174,8 +176,8 @@ impl<'a> TicketGraph<'a> { /// Each node includes `id`, `title`, `status`, `priority`, and /// `dependencies` (only in-graph dependency IDs). /// - /// Each edge is `{"from": , "to": }` — meaning - /// the ticket identified by `from` must be completed before `to` can start. + /// Each edge is `{"from": , "to": }` — meaning + /// the ticket identified by `from` depends on (must wait for) `to`. pub fn to_json_value(&self) -> serde_json::Value { let nodes: Vec = self .ids @@ -192,14 +194,14 @@ impl<'a> TicketGraph<'a> { }) .collect(); - // Edges point from the blocking ticket to the blocked ticket. + // Edges point from the dependent (waiting) ticket to the dependency (prerequisite). let edges: Vec = self .ids .iter() .filter_map(|id| self.nodes.get(id)) .flat_map(|node| { let from = node.ticket.id.as_str(); - node.dependents + node.dependencies .iter() .map(move |&to| serde_json::json!({ "from": from, "to": to })) }) @@ -211,7 +213,7 @@ impl<'a> TicketGraph<'a> { /// Serialise the subtree rooted at `root_id` as a JSON object. /// /// Same structure as [`to_json_value`] but limited to nodes and edges - /// within the subtree reachable from `root_id` via dependent edges. + /// within the subtree reachable from `root_id` via dependency edges. /// Returns an empty `{"nodes":[],"edges":[]}` object when `root_id` is /// not in the graph. pub fn to_subtree_json_value(&self, root_id: &str) -> serde_json::Value { @@ -246,7 +248,7 @@ impl<'a> TicketGraph<'a> { .filter_map(|id| self.nodes.get(id)) .flat_map(|node| { let from = node.ticket.id.as_str(); - node.dependents + node.dependencies .iter() .copied() .filter(|&to| reachable.contains(to)) @@ -260,10 +262,10 @@ impl<'a> TicketGraph<'a> { // ── Private helpers ─────────────────────────────────────────────────────────── -/// Depth-first traversal following `dependents` edges. +/// Depth-first traversal following `dependencies` edges. /// -/// Visits `id` and recursively visits each of its dependents (tickets that -/// depend on it). A `visited` set prevents revisiting nodes, making the +/// Visits `id` and recursively visits each of its dependencies (tickets that +/// `id` depends on). A `visited` set prevents revisiting nodes, making the /// function safe even when the data contains dependency cycles. fn dfs_subtree<'a>( nodes: &HashMap<&'a str, GraphNode<'a>>, @@ -277,8 +279,8 @@ fn dfs_subtree<'a>( result.push(id); if let Some(node) = nodes.get(id) { // Clone the list to avoid holding an immutable borrow while we recurse. - let dependents: Vec<&'a str> = node.dependents.clone(); - for dep_id in dependents { + let dependencies: Vec<&'a str> = node.dependencies.clone(); + for dep_id in dependencies { dfs_subtree(nodes, dep_id, visited, result); } } diff --git a/nbd/src/tests.rs b/nbd/src/tests.rs index 6612418..8de8823 100644 --- a/nbd/src/tests.rs +++ b/nbd/src/tests.rs @@ -1068,7 +1068,7 @@ mod graph { assert!(ids.contains(&"bbbbbb")); } - /// When B depends on A, only A is a root. + /// When B depends on A, only B is a root (B is the goal; A is a prerequisite). #[test] fn roots_with_chain() { let a = make_ticket("aaaaaa", &[]); @@ -1077,7 +1077,7 @@ mod graph { let graph = TicketGraph::build(&tickets); let roots = graph.roots(); assert_eq!(roots.len(), 1); - assert_eq!(roots[0].id, "aaaaaa"); + assert_eq!(roots[0].id, "bbbbbb"); } /// Roots are sorted by priority descending. @@ -1095,7 +1095,7 @@ mod graph { } /// `subtree` on a linear chain A → B → C (B depends on A, C depends on B) - /// returns all three IDs when starting from A. + /// returns all three IDs when starting from C (the top-level goal). #[test] fn subtree_linear_chain() { let a = make_ticket("aaaaaa", &[]); @@ -1104,14 +1104,14 @@ mod graph { let tickets = vec![a, b, c]; let graph = TicketGraph::build(&tickets); - let sub = graph.subtree("aaaaaa"); + let sub = graph.subtree("cccccc"); assert_eq!(sub.len(), 3, "subtree should include all three tickets"); assert!(sub.contains(&"aaaaaa")); assert!(sub.contains(&"bbbbbb")); assert!(sub.contains(&"cccccc")); } - /// `subtree` on a leaf node (no dependents) returns just that ID. + /// `subtree` on a leaf node (no dependencies) returns just that ID. #[test] fn subtree_leaf() { let a = make_ticket("aaaaaa", &[]); @@ -1119,8 +1119,8 @@ mod graph { let tickets = vec![a, b]; let graph = TicketGraph::build(&tickets); - let sub = graph.subtree("bbbbbb"); - assert_eq!(sub, vec!["bbbbbb"]); + let sub = graph.subtree("aaaaaa"); + assert_eq!(sub, vec!["aaaaaa"]); } /// `subtree` on an unknown ID returns an empty vec. @@ -1169,9 +1169,9 @@ mod graph { assert!(node_ids.contains(&"bbbbbb")); assert!(node_ids.contains(&"cccccc")); - // Every edge should have "from" (blocker) = "aaaaaa". + // Every edge should have "to" (dependency/prerequisite) = "aaaaaa". for edge in edges { - assert_eq!(edge["from"].as_str().unwrap(), "aaaaaa"); + assert_eq!(edge["to"].as_str().unwrap(), "aaaaaa"); } } @@ -1189,16 +1189,20 @@ mod graph { fn to_subtree_json_value_scoped() { let a = make_ticket("aaaaaa", &[]); let b = make_ticket("bbbbbb", &["aaaaaa"]); // depends on a - let c = make_ticket("cccccc", &[]); // unrelated root + let c = make_ticket("cccccc", &[]); // unrelated let tickets = vec![a, b, c]; let graph = TicketGraph::build(&tickets); - let json = graph.to_subtree_json_value("aaaaaa"); + // Starting from bbbbbb (the goal): subtree includes bbbbbb + its dependency aaaaaa. + let json = graph.to_subtree_json_value("bbbbbb"); let nodes = json["nodes"].as_array().unwrap(); let node_ids: Vec<&str> = nodes.iter().map(|n| n["id"].as_str().unwrap()).collect(); - assert!(node_ids.contains(&"aaaaaa"), "root should be included"); - assert!(node_ids.contains(&"bbbbbb"), "dependent should be included"); + assert!(node_ids.contains(&"bbbbbb"), "root should be included"); + assert!( + node_ids.contains(&"aaaaaa"), + "dependency should be included" + ); assert!( !node_ids.contains(&"cccccc"), "unrelated ticket should be excluded" @@ -1254,8 +1258,8 @@ mod display_graph { assert!(!out.contains("└──"), "should have no branch connectors"); } - /// A two-ticket chain (B depends on A) renders A at the top level and B - /// indented below it with `└──`. + /// A two-ticket chain (B depends on A) renders B at the top level (the goal) + /// and A indented below it with `└──` (the prerequisite). #[test] fn two_ticket_chain() { let a = make_ticket("aaaaaa", &[]); @@ -1264,26 +1268,27 @@ mod display_graph { let graph = TicketGraph::build(&tickets); let out = format_graph(&graph); - // A should appear before B. + // B should appear before A (B is the goal, A is the prerequisite). let pos_a = out.find("aaaaaa").expect("aaaaaa should appear"); let pos_b = out.find("bbbbbb").expect("bbbbbb should appear"); assert!( - pos_a < pos_b, - "root (aaaaaa) should appear before dependent (bbbbbb)" + pos_b < pos_a, + "goal (bbbbbb) should appear before prerequisite (aaaaaa)" ); - // B's line should use the └── connector. + // A's line should use the └── connector. assert!(out.contains("└──"), "last (only) child should use └──"); assert!(!out.contains("├──"), "only child should not use ├──"); } - /// When a root has two dependents, the first uses `├──` and the last `└──`. + /// When a goal depends on two prerequisites, the first uses `├──` and the last `└──`. #[test] - fn branching_parent() { - let root = make_ticket("aaaaaa", &[]); - let b = make_ticket("bbbbbb", &["aaaaaa"]); - let c = make_ticket("cccccc", &["aaaaaa"]); - let tickets = vec![root, b, c]; + fn branching_goal() { + let a = make_ticket("aaaaaa", &[]); + let b = make_ticket("bbbbbb", &[]); + // cccccc depends on both aaaaaa and bbbbbb, so it renders with two children. + let c = make_ticket("cccccc", &["aaaaaa", "bbbbbb"]); + let tickets = vec![a, b, c]; let graph = TicketGraph::build(&tickets); let out = format_graph(&graph); @@ -1291,7 +1296,7 @@ mod display_graph { assert!(out.contains("└──"), "last child should use └──"); } - /// `format_subtree` for a root only includes that root and its dependents, + /// `format_subtree` for a goal includes that goal and its dependencies, /// not unrelated tickets. #[test] fn subtree_excludes_unrelated() { @@ -1301,9 +1306,10 @@ mod display_graph { let tickets = vec![a, b, c]; let graph = TicketGraph::build(&tickets); - let out = format_subtree(&graph, "aaaaaa"); - assert!(out.contains("aaaaaa"), "root should be present"); - assert!(out.contains("bbbbbb"), "dependent should be present"); + // Starting from bbbbbb (the goal): renders bbbbbb and its dependency aaaaaa. + let out = format_subtree(&graph, "bbbbbb"); + assert!(out.contains("bbbbbb"), "goal should be present"); + assert!(out.contains("aaaaaa"), "dependency should be present"); assert!(!out.contains("cccccc"), "unrelated ticket should be absent"); } @@ -1329,7 +1335,7 @@ mod display_graph { let tickets = vec![a, b]; let graph = TicketGraph::build(&tickets); // format_subtree drives rendering from "aaaaaa"; when it tries to - // revisit "aaaaaa" via bbbbbb's dependent edge, it should mark with *. + // revisit "aaaaaa" via bbbbbb's dependency edge, it should mark with *. let out = format_subtree(&graph, "aaaaaa"); assert!( out.contains(" *"), diff --git a/nbd/tests/integration.rs b/nbd/tests/integration.rs index f3356e7..7be819e 100644 --- a/nbd/tests/integration.rs +++ b/nbd/tests/integration.rs @@ -1757,8 +1757,8 @@ fn graph_all_no_deps() { ); } -/// When B depends on A, `nbd graph` shows A at the top level and B indented -/// below it with `└──`. +/// When B depends on A, `nbd graph` shows B at the top level (the goal) and A +/// indented below it with `└──` (the prerequisite). #[test] fn graph_chain() { let env = TestEnv::new(); @@ -1769,21 +1769,24 @@ fn graph_chain() { assert!(output.status.success()); let stdout = String::from_utf8(output.stdout).unwrap(); - // A should appear before B. + // B should appear before A (B is the goal, A is the prerequisite). let pos_a = stdout.find(&id_a).expect("id_a not found"); let pos_b = stdout.find(&id_b).expect("id_b not found"); - assert!(pos_a < pos_b, "root (A) should appear before dependent (B)"); + assert!( + pos_b < pos_a, + "goal (B) should appear before prerequisite (A)" + ); - // B's line should use `└──`. - let line_b = stdout.lines().find(|l| l.contains(&id_b)).unwrap(); + // A's line should use `└──`. + let line_a = stdout.lines().find(|l| l.contains(&id_a)).unwrap(); assert!( - line_b.contains("└──"), - "child should use └── connector: {line_b}" + line_a.contains("└──"), + "prerequisite should use └── connector: {line_a}" ); } -/// `nbd graph ` for ticket A shows A and its dependents but not unrelated -/// tickets. +/// `nbd graph ` for ticket B (which depends on A) shows B and its +/// dependency A, but not unrelated tickets. #[test] fn graph_single_ticket_subtree() { let env = TestEnv::new(); @@ -1791,12 +1794,12 @@ fn graph_single_ticket_subtree() { let id_b = env.create(&["--title", "Beta", "--deps", &id_a]); let id_c = env.create(&["--title", "Gamma"]); // unrelated - let output = env.run(&["graph", &id_a]); + let output = env.run(&["graph", &id_b]); assert!(output.status.success()); let stdout = String::from_utf8(output.stdout).unwrap(); - assert!(stdout.contains(&id_a), "root should be in subtree"); - assert!(stdout.contains(&id_b), "dependent should be in subtree"); + assert!(stdout.contains(&id_b), "goal should be in subtree"); + assert!(stdout.contains(&id_a), "dependency should be in subtree"); assert!( !stdout.contains(&id_c), "unrelated ticket should be absent: {stdout}" @@ -1832,12 +1835,17 @@ fn graph_json_output() { node_ids.contains(&id_b.as_str()), "nodes should include id_b" ); - assert_eq!(edges.len(), 1, "should have exactly one edge (A blocks B)"); - assert_eq!(edges[0]["from"].as_str().unwrap(), id_a); - assert_eq!(edges[0]["to"].as_str().unwrap(), id_b); + assert_eq!( + edges.len(), + 1, + "should have exactly one edge (B depends on A)" + ); + assert_eq!(edges[0]["from"].as_str().unwrap(), id_b); + assert_eq!(edges[0]["to"].as_str().unwrap(), id_a); } -/// `nbd graph --json` returns only nodes and edges reachable from . +/// `nbd graph --json` returns only nodes and edges reachable from +/// via dependency edges (the goal and its transitive prerequisites). #[test] fn graph_json_subtree() { let env = TestEnv::new(); @@ -1845,15 +1853,16 @@ fn graph_json_subtree() { let id_b = env.create(&["--title", "Beta", "--deps", &id_a]); let id_c = env.create(&["--title", "Gamma"]); // unrelated - let output = env.run(&["graph", &id_a, "--json"]); + // Starting from id_b (the goal): subtree includes id_b and its dependency id_a. + let output = env.run(&["graph", &id_b, "--json"]); assert!(output.status.success()); let stdout = String::from_utf8(output.stdout).unwrap(); let parsed: serde_json::Value = serde_json::from_str(&stdout).unwrap(); let nodes = parsed["nodes"].as_array().unwrap(); let node_ids: Vec<&str> = nodes.iter().map(|n| n["id"].as_str().unwrap()).collect(); - assert!(node_ids.contains(&id_a.as_str())); assert!(node_ids.contains(&id_b.as_str())); + assert!(node_ids.contains(&id_a.as_str())); assert!( !node_ids.contains(&id_c.as_str()), "unrelated ticket should be excluded from JSON subtree"