refuse a dependency add that would close a cycle

hive-forge/src/verbs/dependency.rs

@@ -9,7 +9,7 @@
 //! action, which is a same-repo-only relationship (there is no
 //! cross-repo dependency support in this CLI, mirroring the UI).
 
-use std::collections::HashSet;
+use std::collections::{HashMap, HashSet, VecDeque};
 
 use anyhow::{Result, bail};
 use clap::{Args as ClapArgs, Subcommand};
@@ -97,6 +97,11 @@ fn apply_and_verify(
     deps: &[u64],
     adding: bool,
 ) -> Result<()> {
+    if adding {
+        for dep in deps {
+            refuse_cycle(client, owner, name, number, *dep)?;
+        }
+    }
     let mut call_errors = Vec::new();
     for dep in deps {
         let meta = dep_meta(owner, name, *dep)?;
@@ -185,6 +190,72 @@ fn apply_and_verify(
     Ok(())
 }
 
+/// Refuse an add of `dep` as a blocker of `number` when `dep` already
+/// depends on `number`, directly or through other issues.
+///
+/// Forgejo answers a direct reverse edge with a bare `500` (its handler
+/// maps every model error, `ErrCircularDependency` included, to
+/// `Internal Server Error`) and accepts longer cycles without complaint,
+/// so the check walks `dependency list` client-side.
+fn refuse_cycle(client: &Client, owner: &str, name: &str, number: u64, dep: u64) -> Result<()> {
+    let path = find_dependency_path(dep, number, |n| {
+        let deps = dependency_summaries(client, owner, name, n)?;
+        Ok(deps
+            .iter()
+            .filter_map(|d| d.get("number").and_then(Value::as_u64))
+            .collect())
+    })?;
+    let Some(path) = path else { return Ok(()) };
+    let how = match path.as_slice() {
+        [_, _] => "directly".to_owned(),
+        [_, via @ .., _] => format!(
+            "via {}",
+            via.iter()
+                .map(|n| format!("#{n}"))
+                .collect::<Vec<_>>()
+                .join(" -> ")
+        ),
+        _ => String::new(),
+    };
+    bail!(
+        "hive-forge dependency add: #{dep} already depends on #{number} ({how}), so adding \
+         #{dep} as a dependency of #{number} would create a cycle; remove that edge first"
+    );
+}
+
+/// Breadth-first walk of the "depends on" relation from `from`, using
+/// `deps_of` to list an issue's dependencies. Returns the shortest chain
+/// `from, …, to` when `to` is reachable, else `None`.
+fn find_dependency_path(
+    from: u64,
+    to: u64,
+    mut deps_of: impl FnMut(u64) -> Result<Vec<u64>>,
+) -> Result<Option<Vec<u64>>> {
+    let mut parent: HashMap<u64, u64> = HashMap::new();
+    let mut seen = HashSet::from([from]);
+    let mut queue = VecDeque::from([from]);
+    while let Some(cur) = queue.pop_front() {
+        for next in deps_of(cur)? {
+            if !seen.insert(next) {
+                continue;
+            }
+            parent.insert(next, cur);
+            if next == to {
+                let mut path = vec![to];
+                let mut at = to;
+                while let Some(&p) = parent.get(&at) {
+                    path.push(p);
+                    at = p;
+                }
+                path.reverse();
+                return Ok(Some(path));
+            }
+            queue.push_back(next);
+        }
+    }
+    Ok(None)
+}
+
 /// Build the `IssueMeta` body the create/remove endpoints want.
 ///
 /// `owner`/`repo` are filled in with the *same* repo the request URL
@@ -221,6 +292,36 @@ mod tests {
         assert_eq!(m.index, Some(3950));
     }
 
+    fn graph(edges: &[(u64, u64)]) -> impl FnMut(u64) -> Result<Vec<u64>> + '_ {
+        move |n| {
+            Ok(edges
+                .iter()
+                .filter(|(a, _)| *a == n)
+                .map(|(_, b)| *b)
+                .collect())
+        }
+    }
+
+    #[test]
+    fn path_finds_direct_transitive_and_absent() {
+        let edges = [(1, 2), (2, 3), (4, 5)];
+        assert_eq!(
+            find_dependency_path(1, 2, graph(&edges)).unwrap(),
+            Some(vec![1, 2])
+        );
+        assert_eq!(
+            find_dependency_path(1, 3, graph(&edges)).unwrap(),
+            Some(vec![1, 2, 3])
+        );
+        assert_eq!(find_dependency_path(1, 5, graph(&edges)).unwrap(), None);
+    }
+
+    #[test]
+    fn path_walk_terminates_on_an_existing_cycle() {
+        let edges = [(1, 2), (2, 1)];
+        assert_eq!(find_dependency_path(1, 9, graph(&edges)).unwrap(), None);
+    }
+
     /// Out-of-range must be an error, not a wrapped negative index that
     /// the forge would then look up as some other issue.
     #[test]

hive-forge/tests/dependency_cycle.rs

@@ -0,0 +1,151 @@
+//! `dependency add` against a mock forge: an edge that would close a cycle
+//! must be refused with a message naming the existing edge, before any
+//! POST reaches the forge.
+
+use std::io::{BufRead as _, BufReader, Read as _, Write as _};
+use std::net::{TcpListener, TcpStream};
+use std::path::PathBuf;
+use std::process::Command;
+use std::sync::{Arc, Mutex};
+
+/// Existing edges `(issue, depends_on)` served by the mock.
+const EDGES: &[(u64, u64)] = &[(20, 10), (30, 20)];
+
+fn scratch_state_dir(tag: &str) -> PathBuf {
+    let ts = std::time::SystemTime::now()
+        .duration_since(std::time::UNIX_EPOCH)
+        .map_or(0, |d| d.as_nanos());
+    let dir = std::env::temp_dir().join(format!("hive-forge-dep-cycle-test-{tag}-{ts}"));
+    std::fs::create_dir_all(&dir).expect("create scratch state dir");
+    std::fs::write(dir.join("forge-token"), "test-token\n").expect("seed forge-token");
+    dir
+}
+
+/// `GET .../issues/<n>/dependencies` lists `n`'s dependencies; `POST`
+/// answers 500 like the real forge does for a cycle and is recorded.
+fn serve(mut stream: TcpStream, posts: &Mutex<u32>) {
+    let mut reader = BufReader::new(stream.try_clone().expect("clone stream"));
+    let mut request_line = String::new();
+    reader.read_line(&mut request_line).expect("request line");
+    let mut content_length = 0usize;
+    loop {
+        let mut line = String::new();
+        if reader.read_line(&mut line).expect("read header") == 0 || line == "\r\n" {
+            break;
+        }
+        if let Some((name, value)) = line.split_once(':')
+            && name.eq_ignore_ascii_case("content-length")
+        {
+            content_length = value.trim().parse().expect("numeric content-length");
+        }
+    }
+    let mut raw = vec![0u8; content_length];
+    reader.read_exact(&mut raw).expect("read body");
+    let response = if request_line.starts_with("POST") {
+        *posts.lock().expect("lock") += 1;
+        "HTTP/1.1 500 Internal Server Error\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"
+            .to_owned()
+    } else {
+        let n: u64 = request_line
+            .split("/issues/")
+            .nth(1)
+            .and_then(|r| r.split(['/', '?', ' ']).next())
+            .and_then(|r| r.parse().ok())
+            .expect("issue number in path");
+        let deps: Vec<_> = EDGES
+            .iter()
+            .filter(|(a, _)| *a == n)
+            .map(|(_, b)| serde_json::json!({ "number": b, "title": "t", "state": "open" }))
+            .collect();
+        let body = if request_line.contains("/dependencies") {
+            serde_json::Value::Array(deps).to_string()
+        } else {
+            serde_json::json!({ "number": n, "title": "t", "state": "open" }).to_string()
+        };
+        format!(
+            "HTTP/1.1 200 OK\r\nContent-Type: application/json\r\nContent-Length: {}\r\nConnection: close\r\n\r\n{body}",
+            body.len()
+        )
+    };
+    stream
+        .write_all(response.as_bytes())
+        .expect("write response");
+}
+
+/// Run `hive-forge -r o/r issue dependency <number> add <dep>`; returns
+/// `(success, stderr, POST count)`.
+fn add(tag: &str, number: &str, dep: &str) -> (bool, String, u32) {
+    let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock forge");
+    let addr = listener.local_addr().expect("mock forge addr");
+    let posts = Arc::new(Mutex::new(0));
+    let served = Arc::clone(&posts);
+    std::thread::spawn(move || {
+        for stream in listener.incoming() {
+            serve(stream.expect("accept"), &served);
+        }
+    });
+    let out = Command::new(env!("CARGO_BIN_EXE_hive-forge"))
+        .args(["-r", "o/r", "issue", "dependency", number, "add", dep])
+        .env("HIVE_FORGE_URL", format!("http://{addr}"))
+        .env("HYPERHIVE_STATE_DIR", scratch_state_dir(tag))
+        .env_remove("HIVE_FORGE_TOKEN_FILE")
+        .env_remove("HIVE_LABEL")
+        .output()
+        .expect("run hive-forge");
+    let count = *posts.lock().expect("lock");
+    (
+        out.status.success(),
+        String::from_utf8_lossy(&out.stderr).into_owned(),
+        count,
+    )
+}
+
+#[test]
+fn direct_reverse_edge_is_refused_before_any_post() {
+    // Issue 20 depends on issue 10; making 10 depend on 20 closes a cycle.
+    let (ok, stderr, posts) = add("direct", "10", "20");
+    assert!(!ok);
+    assert!(
+        stderr.contains(&format!(
+            "{} already depends on {} (directly)",
+            tag(20),
+            tag(10)
+        )),
+        "{stderr}"
+    );
+    assert!(stderr.contains("would create a cycle"), "{stderr}");
+    assert!(!stderr.contains("500"), "{stderr}");
+    assert_eq!(posts, 0);
+}
+
+#[test]
+fn transitive_cycle_is_refused_before_any_post() {
+    // Issue 30 depends on 20, which depends on 10; adding 30 under 10 closes a cycle via 20.
+    let (ok, stderr, posts) = add("transitive", "10", "30");
+    assert!(!ok);
+    assert!(
+        stderr.contains(&format!(
+            "{} already depends on {} (via {})",
+            tag(30),
+            tag(10),
+            tag(20)
+        )),
+        "{stderr}"
+    );
+    assert_eq!(posts, 0);
+}
+
+#[test]
+fn non_cyclic_add_reaches_the_forge() {
+    // 10 depends on nothing, so making 40 depend on 10 is fine; the mock
+    // answers the POST with 500, which proves the request was sent.
+    let (ok, stderr, posts) = add("control", "40", "10");
+    assert_eq!(posts, 1, "{stderr}");
+    assert!(!stderr.contains("cycle"), "{stderr}");
+    assert!(!ok);
+}
+
+/// How the CLI writes an issue number in its messages.
+fn tag(n: u64) -> String {
+    format!("{}{n}", '#')
+}