hive-forge: format the assign-reviewer test and document the --remove failure

docs/tools/forge.md

@@ -82,7 +82,7 @@ hive-forge pr reviews 42 --approve         # submit APPROVED review
 hive-forge pr reviews 42 --request-changes -m "msg"  # submit REQUEST_CHANGES review
 hive-forge pr reviews 42 --comment -m "msg"  # submit COMMENT review
 hive-forge pr assign-reviewer 42 argus      # set argus as a requested reviewer on the PR
-hive-forge pr assign-reviewer 42 argus --remove  # withdraw the request
+hive-forge pr assign-reviewer 42 argus --remove  # withdraw the request; fails if argus is still listed afterwards (a reviewer who already reviewed stays listed)
 hive-forge pr assign-committer 42 damocles  # add damocles to the PR's assignee list
 hive-forge pr diff 42                       # unified diff (lockfile hunks collapsed by default)
 hive-forge pr diff 42 --full                # include unfiltered lockfile hunks

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

hive-forge/tests/pr_assign_reviewer.rs

@@ -0,0 +1,117 @@
+//! `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}"
+    );
+}