From cdbfa953a25192b89f15ea53375bc60639fa93d1 Mon Sep 17 00:00:00 2001 From: Amolith Date: Wed, 25 Feb 2026 20:10:21 +0000 Subject: [PATCH] Add cycle detection to dep add 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. --- src/cmd/dep.rs | 5 ++- src/db.rs | 38 +++++++++++++++++++++ tests/cli_dep.rs | 89 ++++++++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 131 insertions(+), 1 deletion(-) diff --git a/src/cmd/dep.rs b/src/cmd/dep.rs index 6b5d86341bb31fb466d8256b64f264bab5a06abb..f1dd2a22ed868da1ffffb6f3a65a44b5158f1ca1 100644 --- a/src/cmd/dep.rs +++ b/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], diff --git a/src/db.rs b/src/db.rs index 02778295386a02824d1966a138318e185a514d9c..59ea9ada8b361084e547557977cdbd6f77c86ae2 100644 --- a/src/db.rs +++ b/src/db.rs @@ -132,6 +132,44 @@ pub fn load_blockers(conn: &Connection, task_id: &str) -> Result> { 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 { + 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 = stmt + .query_map([¤t], |r| r.get(0))? + .collect::>()?; + + 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 { let task = conn.query_row( diff --git a/tests/cli_dep.rs b/tests/cli_dep.rs index 0bdad01875d6f4443faf090fe2ea8bd9e9bc645a..20f115f37d7b5a106dceb43fbad343633567d6c0 100644 --- a/tests/cli_dep.rs +++ b/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); +}