From 347b8119b2156c097ed832a9dc6f0224cfe1ba15 Mon Sep 17 00:00:00 2001 From: atlas Date: Tue, 27 Oct 2026 14:00:49 +0100 Subject: [PATCH] refuse a dependency add that would close a cycle --- hive-forge/src/verbs/dependency.rs | 103 +++++++++++++++++- hive-forge/tests/dependency_cycle.rs | 151 +++++++++++++++++++++++++++ 2 files changed, 253 insertions(+), 1 deletion(-) create mode 100644 hive-forge/tests/dependency_cycle.rs diff --git a/hive-forge/src/verbs/dependency.rs b/hive-forge/src/verbs/dependency.rs index 97d759b7..ff2d7ece 100644 --- a/hive-forge/src/verbs/dependency.rs +++ b/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::>() + .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>, +) -> Result>> { + let mut parent: HashMap = 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> + '_ { + 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] diff --git a/hive-forge/tests/dependency_cycle.rs b/hive-forge/tests/dependency_cycle.rs new file mode 100644 index 00000000..5ace7537 --- /dev/null +++ b/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//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) { + 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 add `; 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}", '#') +}