hive-forge dependency: don't drop the original errors if the verification read-back also fails

hive-forge/src/verbs/dependency.rs

@@ -9,9 +9,12 @@
 //! 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 anyhow::{Result, bail};
 use clap::{Args as ClapArgs, Subcommand};
 use forgejo_api::structs::IssueMeta;
+use serde_json::Value;
 
 use crate::client::{Client, index};
 use crate::verbs::{dependency_summaries, print_json};
@@ -50,29 +53,121 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
             if deps.is_empty() {
                 bail!("hive-forge dependency add: pass at least one issue/PR number");
             }
-            for dep in &deps {
-                client
-                    .api()
-                    .issue_create_issue_dependencies(owner, name, idx, dep_meta(owner, name, *dep)?)
-                    .send()?;
-            }
+            apply_and_verify(client, owner, name, idx, args.number, &deps, true)?;
         }
         Action::Remove { deps } => {
             if deps.is_empty() {
                 bail!("hive-forge dependency remove: pass at least one issue/PR number");
             }
-            for dep in &deps {
-                client
-                    .api()
-                    .issue_remove_issue_dependencies(owner, name, idx, dep_meta(owner, name, *dep)?)
-                    .send()?;
-            }
+            apply_and_verify(client, owner, name, idx, args.number, &deps, false)?;
         }
     }
     let deps = dependency_summaries(client, owner, name, args.number)?;
     print_json(&serde_json::json!(deps))
 }
 
+/// Apply an add/remove edit for each of `deps` against `number`, then
+/// verify by reading the dependency list back rather than trusting the
+/// HTTP status this verb family returns.
+///
+/// Measured: the status code lies in both directions on this endpoint
+/// pair. `create` can 500 on a call that actually wrote the
+/// edge (an intermittent server-side double-processing, not a client
+/// retry — ruled out separately). `remove` can answer `201 Created` on a
+/// call that actually deleted the edge, which the typed client's
+/// endpoint spec maps to an error since 201 isn't the expected status for
+/// a deletion. So a `send()` error here only means "maybe" — the
+/// authoritative signal is whether the edge is actually there afterward.
+///
+/// Calls that errored are only re-classified as real failures if the
+/// read-back does NOT show the expected converged state (edge present
+/// after an add, absent after a remove) — a call that errored *and*
+/// didn't converge is a genuine failure and still surfaces.
+fn apply_and_verify(
+    client: &Client,
+    owner: &str,
+    name: &str,
+    idx: i64,
+    number: u64,
+    deps: &[u64],
+    adding: bool,
+) -> Result<()> {
+    let mut call_errors = Vec::new();
+    for dep in deps {
+        let meta = dep_meta(owner, name, *dep)?;
+        let res = if adding {
+            client
+                .api()
+                .issue_create_issue_dependencies(owner, name, idx, meta)
+                .send()
+        } else {
+            client
+                .api()
+                .issue_remove_issue_dependencies(owner, name, idx, meta)
+                .send()
+        };
+        if let Err(e) = res {
+            call_errors.push((*dep, e));
+        }
+    }
+    if call_errors.is_empty() {
+        return Ok(());
+    }
+    let after = match dependency_summaries(client, owner, name, number) {
+        Ok(deps) => deps,
+        Err(read_err) => {
+            // The read-back itself failed, so none of the per-dep errors
+            // above could be re-classified — surface all of them rather
+            // than dropping them behind the read-back's own error.
+            let originals: Vec<String> = call_errors
+                .iter()
+                .map(|(dep, e)| format!("#{dep}: {e:#}"))
+                .collect();
+            let verb = if adding { "add" } else { "remove" };
+            bail!(
+                "hive-forge dependency {verb}: {} call(s) reported an error, and the \
+                 read-back to check whether they actually landed also failed ({read_err:#}). \
+                 Original error(s):\n{}",
+                call_errors.len(),
+                originals.join("\n")
+            );
+        }
+    };
+    let present: HashSet<i64> = after
+        .iter()
+        .filter_map(|d| d.get("number").and_then(Value::as_i64))
+        .collect();
+    let mut real_failures = Vec::new();
+    for (dep, e) in call_errors {
+        let still_present = present.contains(&index(dep).unwrap_or(-1));
+        // Converged iff present after an add, absent after a remove.
+        if still_present == adding {
+            // The write landed despite the error, but the error itself is
+            // still real signal: it means something *after* the write
+            // failed server-side (a timeline entry, a notification, a
+            // cycle check — we don't know which). Converging proves the
+            // effect is present, not that the operation fully succeeded,
+            // so don't let a clean exit make that anomaly unobservable.
+            eprintln!(
+                "hive-forge: warning: dependency on #{dep} converged despite a reported \
+                 error (server-side issue after the write, not a real failure): {e:#}"
+            );
+        } else {
+            real_failures.push(format!("#{dep}: {e:#}"));
+        }
+    }
+    if !real_failures.is_empty() {
+        let verb = if adding { "add" } else { "remove" };
+        bail!(
+            "hive-forge dependency {verb}: {} call(s) genuinely failed (read-back doesn't \
+             show the expected state):\n{}",
+            real_failures.len(),
+            real_failures.join("\n")
+        );
+    }
+    Ok(())
+}
+
 /// Build the `IssueMeta` body the create/remove endpoints want.
 ///
 /// `owner`/`repo` are filled in with the *same* repo the request URL