From ff4fd93f81c53c6d81c5f7456f4028945beec65f Mon Sep 17 00:00:00 2001 From: damocles Date: Mon, 26 Oct 2026 17:03:59 +0100 Subject: [PATCH] hive-forge: pr assign-reviewer --remove fails when the reviewer is still listed afterwards --- hive-forge/src/verbs/pr_assign_reviewer.rs | 21 ++++ hive-forge/tests/pr_assign_reviewer.rs | 106 +++++++++++++++++++++ 2 files changed, 127 insertions(+) create mode 100644 hive-forge/tests/pr_assign_reviewer.rs diff --git a/hive-forge/src/verbs/pr_assign_reviewer.rs b/hive-forge/src/verbs/pr_assign_reviewer.rs index b36c8760..a3a70eb6 100644 --- a/hive-forge/src/verbs/pr_assign_reviewer.rs +++ b/hive-forge/src/verbs/pr_assign_reviewer.rs @@ -46,6 +46,27 @@ pub fn run(client: &Client, args: Args) -> Result<()> { .api() .repo_delete_pull_review_requests(owner, name, idx, body) .send()?; + // The forge answers success even when there was no pending request + // to remove (the reviewer already submitted a review), so read the + // list back instead of claiming a withdrawal that did not happen. + let pull = client + .api() + .repo_get_pull_request(owner, name, idx) + .send()?; + let still_listed = pull + .requested_reviewers + .as_deref() + .unwrap_or_default() + .iter() + .any(|u| u.login.as_deref() == Some(args.user.as_str())); + if still_listed { + anyhow::bail!( + "{} is still listed as a requested reviewer on #{} after the withdraw call \ + (the forge keeps a reviewer who already reviewed in that list)", + args.user, + args.number + ); + } println!( "review request withdrawn: {} on #{}", args.user, args.number diff --git a/hive-forge/tests/pr_assign_reviewer.rs b/hive-forge/tests/pr_assign_reviewer.rs new file mode 100644 index 00000000..cac3f19a --- /dev/null +++ b/hive-forge/tests/pr_assign_reviewer.rs @@ -0,0 +1,106 @@ +//! `pr assign-reviewer --remove` against a mock forge: the withdraw call +//! answers success either way, so the verb must read the requested list back +//! and refuse to claim a withdrawal the forge did not perform. + +use std::io::{BufRead as _, BufReader, Read as _, Write as _}; +use std::net::{TcpListener, TcpStream}; +use std::path::PathBuf; +use std::process::Command; + +/// A unique scratch `HYPERHIVE_STATE_DIR` holding a `forge-token` file, +/// same manual-tmpdir pattern as `pr_edit.rs`. +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-assign-reviewer-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 +} + +/// Answer one request: `DELETE` succeeds with an empty body, `GET` returns +/// PR #7 listing `requested` as its requested reviewers. +fn serve(mut stream: TcpStream, requested: &[&str]) { + let mut reader = BufReader::new(stream.try_clone().expect("clone stream")); + let mut request_line = String::new(); + reader + .read_line(&mut request_line) + .expect("read 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("DELETE") { + "HTTP/1.1 204 No Content\r\nConnection: close\r\n\r\n".to_owned() + } else { + let reviewers: Vec<_> = requested + .iter() + .map(|login| serde_json::json!({ "login": login })) + .collect(); + let body = serde_json::json!({ + "number": 7, + "title": "t", + "state": "open", + "requested_reviewers": reviewers, + }) + .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 pr assign-reviewer 7 argus --remove` against a +/// forge whose PR lists `requested` afterwards. Returns `(success, stdout, stderr)`. +fn remove_argus(tag: &str, requested: &'static [&'static str]) -> (bool, String, String) { + let listener = TcpListener::bind("127.0.0.1:0").expect("bind mock forge"); + let addr = listener.local_addr().expect("mock forge addr"); + std::thread::spawn(move || { + for stream in listener.incoming() { + serve(stream.expect("accept"), requested); + } + }); + let out = Command::new(env!("CARGO_BIN_EXE_hive-forge")) + .args(["-r", "o/r", "pr", "assign-reviewer", "7", "argus", "--remove"]) + .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"); + ( + out.status.success(), + String::from_utf8_lossy(&out.stdout).into_owned(), + String::from_utf8_lossy(&out.stderr).into_owned(), + ) +} + +#[test] +fn withdrawal_that_left_the_reviewer_listed_is_reported_as_failed() { + let (ok, stdout, stderr) = remove_argus("still-listed", &["argus"]); + assert!(!ok, "{stdout}"); + assert!(!stdout.contains("withdrawn"), "{stdout}"); + assert!(stderr.contains("still listed"), "{stderr}"); +} + +#[test] +fn withdrawal_that_removed_the_reviewer_is_reported_as_done() { + let (ok, stdout, stderr) = remove_argus("removed", &["review-classifier"]); + assert!(ok, "{stderr}"); + assert!(stdout.contains("review request withdrawn: argus on #7"), "{stdout}"); +}