diff --git a/hive-forge/src/verbs/labels.rs b/hive-forge/src/verbs/labels.rs index 77f6bcab..41439111 100644 --- a/hive-forge/src/verbs/labels.rs +++ b/hive-forge/src/verbs/labels.rs @@ -69,22 +69,36 @@ pub fn run(client: &Client, args: Args) -> Result<()> { bail!("hive-forge labels remove: pass at least one label name"); } let all = repo_labels(client)?; - for label in &labels { - if let Some(id) = lookup_id(&all, label) { - let _ = client - .api() - .issue_remove_label( - owner, - name, - idx, - &id.to_string(), - DeleteLabelsOption { updated_at: None }, - ) - .send(); - } + // Same strict resolution as `add` — a typo used to be silently + // skipped here instead of refused. + let ids = resolve_ids(&all, &labels)?; + for id in ids { + client + .api() + .issue_remove_label( + owner, + name, + idx, + &id.to_string(), + DeleteLabelsOption { updated_at: None }, + ) + .send()?; } - let labels = client.api().issue_get_labels(owner, name, idx).send()?; - print_label_names(&labels); + let current = client.api().issue_get_labels(owner, name, idx).send()?; + // Forgejo can 200 a remove it silently didn't apply (e.g. + // insufficient permission), same as the assignee case in + // `assign.rs` — diff the result against the intent instead of + // trusting the status code. + let still_present = still_present_names(&labels, ¤t); + if !still_present.is_empty() { + bail!( + "label(s) still present on #{} after remove — the forge \ + rejected the change: {}", + args.number, + still_present.join(", ") + ); + } + print_label_names(¤t); } } Ok(()) @@ -162,6 +176,21 @@ fn lookup_id(all: &[Label], name: &str) -> Option { .and_then(|l| l.id) } +/// Which of `requested` names are still present in `current` — the +/// remove-verb's post-condition check. A non-empty result means the forge +/// didn't actually drop those labels even though the call returned 200. +fn still_present_names(requested: &[String], current: &[Label]) -> Vec { + requested + .iter() + .filter(|r| { + current + .iter() + .any(|l| l.name.as_deref() == Some(r.as_str())) + }) + .cloned() + .collect() +} + fn print_label_names(labels: &[Label]) { let names: Vec<&str> = labels.iter().filter_map(|l| l.name.as_deref()).collect(); let _ = print_json(&json!(names)); @@ -169,7 +198,7 @@ fn print_label_names(labels: &[Label]) { #[cfg(test)] mod tests { - use super::resolve_ids; + use super::{resolve_ids, still_present_names}; use forgejo_api::structs::Label; fn label(id: i64, name: &str) -> Label { @@ -206,4 +235,35 @@ mod tests { let err = resolve_ids(&[], &["anything".to_owned()]).unwrap_err(); assert!(err.to_string().contains("(none)")); } + + #[test] + fn unresolved_remove_name_is_refused_like_add() { + // `remove` resolves names through the same `resolve_ids` as `add`, + // so a typo is refused up front instead of silently doing nothing. + let all = vec![label(1, "area/ops")]; + let err = resolve_ids(&all, &["area:ops".to_owned()]).unwrap_err(); + assert!(err.to_string().contains("area:ops")); + } + + #[test] + fn still_present_control_all_removed_reports_none() { + // Happy path: none of the requested names remain, so the caller + // should not bail. + let requested = vec!["area/ops".to_owned(), "type/bug".to_owned()]; + let current = vec![label(3, "state/open")]; + assert!(still_present_names(&requested, ¤t).is_empty()); + } + + #[test] + fn still_present_names_reports_labels_the_forge_kept() { + // A remove the forge silently rejected — the label is still on the + // issue after the "successful" call. This is the case the old code + // (`let _ = …send();` with no re-check) let through as success. + let requested = vec!["area/ops".to_owned(), "type/bug".to_owned()]; + let current = vec![label(1, "area/ops")]; + assert_eq!( + still_present_names(&requested, ¤t), + vec!["area/ops".to_owned()] + ); + } }