Add cycle detection to dep add

Amolith created 1 week ago

Prevent circular dependencies in the blocker graph by walking the existing
edges from the proposed blocker back toward the task. Reject with a clear
error if a cycle would be created.

Change summary

src/cmd/dep.rs   |  5 ++
src/db.rs        | 38 +++++++++++++++++++++
tests/cli_dep.rs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++++
3 files changed, 131 insertions(+), 1 deletion(-)

Detailed changes

src/cmd/dep.rs

@@ -1,4 +1,4 @@
-use anyhow::Result;
+use anyhow::{bail, Result};
 use std::path::Path;
 
 use crate::cli::DepAction;
@@ -9,6 +9,9 @@ pub fn run(root: &Path, action: &DepAction, json: bool) -> Result<()> {
 
     match action {
         DepAction::Add { child, parent } => {
+            if db::would_cycle(&conn, parent, child)? {
+                bail!("adding dependency would create a cycle: {child} → {parent} → … → {child}");
+            }
             conn.execute(
                 "INSERT OR IGNORE INTO blockers (task_id, blocker_id) VALUES (?1, ?2)",
                 [child, parent],

src/db.rs

@@ -132,6 +132,44 @@ pub fn load_blockers(conn: &Connection, task_id: &str) -> Result<Vec<String>> {
     Ok(blockers)
 }
 
+/// Check whether `from` can reach `to` by following blocker edges.
+///
+/// Returns `true` if there is a path from `from` to `to` in the blocker
+/// graph (i.e. adding an edge `to → from` would create a cycle).
+/// Uses a visited-set so it terminates even if the graph already contains
+/// a cycle from bad data.
+pub fn would_cycle(conn: &Connection, from: &str, to: &str) -> Result<bool> {
+    use std::collections::{HashSet, VecDeque};
+
+    if from == to {
+        return Ok(true);
+    }
+
+    let mut visited = HashSet::new();
+    let mut queue = VecDeque::new();
+    queue.push_back(from.to_string());
+    visited.insert(from.to_string());
+
+    let mut stmt = conn.prepare("SELECT blocker_id FROM blockers WHERE task_id = ?1")?;
+
+    while let Some(current) = queue.pop_front() {
+        let neighbors: Vec<String> = stmt
+            .query_map([&current], |r| r.get(0))?
+            .collect::<rusqlite::Result<_>>()?;
+
+        for neighbor in neighbors {
+            if neighbor == to {
+                return Ok(true);
+            }
+            if visited.insert(neighbor.clone()) {
+                queue.push_back(neighbor);
+            }
+        }
+    }
+
+    Ok(false)
+}
+
 /// Load a full task with labels and blockers.
 pub fn load_task_detail(conn: &Connection, id: &str) -> Result<TaskDetail> {
     let task = conn.query_row(

tests/cli_dep.rs

@@ -90,3 +90,92 @@ fn dep_tree_shows_children() {
         .stdout(predicate::str::contains(".1"))
         .stdout(predicate::str::contains(".2"));
 }
+
+#[test]
+fn dep_add_rejects_self_cycle() {
+    let tmp = init_tmp();
+    let a = create_task(&tmp, "Self-referential");
+
+    td().args(["dep", "add", &a, &a])
+        .current_dir(&tmp)
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("cycle"));
+}
+
+#[test]
+fn dep_add_rejects_direct_cycle() {
+    let tmp = init_tmp();
+    let a = create_task(&tmp, "Task A");
+    let b = create_task(&tmp, "Task B");
+
+    // A blocked by B
+    td().args(["dep", "add", &a, &b])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+
+    // B blocked by A would create A → B → A
+    td().args(["dep", "add", &b, &a])
+        .current_dir(&tmp)
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("cycle"));
+}
+
+#[test]
+fn dep_add_rejects_transitive_cycle() {
+    let tmp = init_tmp();
+    let a = create_task(&tmp, "Task A");
+    let b = create_task(&tmp, "Task B");
+    let c = create_task(&tmp, "Task C");
+
+    // A blocked by B, B blocked by C
+    td().args(["dep", "add", &a, &b])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+    td().args(["dep", "add", &b, &c])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+
+    // C blocked by A would create A → B → C → A
+    td().args(["dep", "add", &c, &a])
+        .current_dir(&tmp)
+        .assert()
+        .failure()
+        .stderr(predicate::str::contains("cycle"));
+}
+
+#[test]
+fn dep_add_allows_diamond_without_cycle() {
+    let tmp = init_tmp();
+    let a = create_task(&tmp, "Task A");
+    let b = create_task(&tmp, "Task B");
+    let c = create_task(&tmp, "Task C");
+    let d = create_task(&tmp, "Task D");
+
+    // Diamond: D blocked by B and C, both blocked by A
+    td().args(["dep", "add", &d, &b])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+    td().args(["dep", "add", &d, &c])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+    td().args(["dep", "add", &b, &a])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+    td().args(["dep", "add", &c, &a])
+        .current_dir(&tmp)
+        .assert()
+        .success();
+
+    // Verify all edges exist — no false cycle detection
+    let t = get_task_json(&tmp, &d);
+    let blockers = t["blockers"].as_array().unwrap();
+    assert_eq!(blockers.len(), 2);
+}