From 7668454baee25b1b18bc1f76afce0a01931042d3 Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 16 Aug 2026 18:44:21 +0200 Subject: [PATCH 1/3] hive-forge: verify dependency add/remove by read-back instead of trusting HTTP status --- hive-forge/src/verbs/dependency.rs | 91 ++++++++++++++++++++++++++---- 1 file changed, 79 insertions(+), 12 deletions(-) diff --git a/hive-forge/src/verbs/dependency.rs b/hive-forge/src/verbs/dependency.rs index 318f29e2..41d94619 100644 --- a/hive-forge/src/verbs/dependency.rs +++ b/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,93 @@ 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 = dependency_summaries(client, owner, name, number)?; + let present: HashSet = after + .iter() + .filter_map(|d| d.get("number").and_then(Value::as_i64)) + .collect(); + let real_failures: Vec = call_errors + .into_iter() + .filter(|(dep, _)| { + let still_present = present.contains(&index(*dep).unwrap_or(-1)); + // Converged iff present after an add, absent after a remove. + // A real failure is anything that didn't converge. + still_present != adding + }) + .map(|(dep, e)| format!("#{dep}: {e:#}")) + .collect(); + 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 From eefd971bd838fd32621801609b9fb306c426cc97 Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 16 Aug 2026 18:48:39 +0200 Subject: [PATCH 2/3] hive-forge dependency: surface a converged-despite-error case instead of swallowing it --- hive-forge/src/verbs/dependency.rs | 29 +++++++++++++++++++---------- 1 file changed, 19 insertions(+), 10 deletions(-) diff --git a/hive-forge/src/verbs/dependency.rs b/hive-forge/src/verbs/dependency.rs index 41d94619..6fb4a349 100644 --- a/hive-forge/src/verbs/dependency.rs +++ b/hive-forge/src/verbs/dependency.rs @@ -118,16 +118,25 @@ fn apply_and_verify( .iter() .filter_map(|d| d.get("number").and_then(Value::as_i64)) .collect(); - let real_failures: Vec = call_errors - .into_iter() - .filter(|(dep, _)| { - let still_present = present.contains(&index(*dep).unwrap_or(-1)); - // Converged iff present after an add, absent after a remove. - // A real failure is anything that didn't converge. - still_present != adding - }) - .map(|(dep, e)| format!("#{dep}: {e:#}")) - .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!( From 4f8b78a4d4176895d57ff18a5431db7c494a2c25 Mon Sep 17 00:00:00 2001 From: damocles Date: Sun, 16 Aug 2026 18:51:46 +0200 Subject: [PATCH 3/3] hive-forge dependency: don't drop the original errors if the verification read-back also fails --- hive-forge/src/verbs/dependency.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/hive-forge/src/verbs/dependency.rs b/hive-forge/src/verbs/dependency.rs index 6fb4a349..64a9d943 100644 --- a/hive-forge/src/verbs/dependency.rs +++ b/hive-forge/src/verbs/dependency.rs @@ -113,7 +113,26 @@ fn apply_and_verify( if call_errors.is_empty() { return Ok(()); } - let after = dependency_summaries(client, owner, name, number)?; + 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 = 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 = after .iter() .filter_map(|d| d.get("number").and_then(Value::as_i64))