hive-forge: labels remove refuses unknown names and verifies removal landed

labels <n> remove used to silently skip an unknown label name (lookup_id
returning None just did nothing) and discard each remove call's Result,
so a typo or a forge-rejected removal (403, insufficient permission)
both exited 0 with the label untouched.

Resolve names through the same resolve_ids labels add already uses, so
an unknown name is refused before anything is removed. After removing,
re-fetch and diff against what was requested — following assign.rs's
same before/after check — and bail! naming any label still present.
This commit is contained in:
atlas 2026-09-24 12:58:01 +02:00 • committed by mara
commit 6ac402ed2d

View file

@ -69,9 +69,11 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
bail!("hive-forge labels remove: pass at least one label name"); bail!("hive-forge labels remove: pass at least one label name");
} }
let all = repo_labels(client)?; let all = repo_labels(client)?;
for label in &labels { // Same strict resolution as `add` — a typo used to be silently
if let Some(id) = lookup_id(&all, label) { // skipped here instead of refused.
let _ = client let ids = resolve_ids(&all, &labels)?;
for id in ids {
client
.api() .api()
.issue_remove_label( .issue_remove_label(
owner, owner,
@ -80,11 +82,23 @@ pub fn run(client: &Client, args: Args) -> Result<()> {
&id.to_string(), &id.to_string(),
DeleteLabelsOption { updated_at: None }, DeleteLabelsOption { updated_at: None },
) )
.send(); .send()?;
} }
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, &current);
if !still_present.is_empty() {
bail!(
"label(s) still present on #{} after remove — the forge \
rejected the change: {}",
args.number,
still_present.join(", ")
);
} }
let labels = client.api().issue_get_labels(owner, name, idx).send()?; print_label_names(&current);
print_label_names(&labels);
} }
} }
Ok(()) Ok(())
@ -162,6 +176,21 @@ fn lookup_id(all: &[Label], name: &str) -> Option<i64> {
.and_then(|l| l.id) .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<String> {
requested
.iter()
.filter(|r| {
current
.iter()
.any(|l| l.name.as_deref() == Some(r.as_str()))
})
.cloned()
.collect()
}
fn print_label_names(labels: &[Label]) { fn print_label_names(labels: &[Label]) {
let names: Vec<&str> = labels.iter().filter_map(|l| l.name.as_deref()).collect(); let names: Vec<&str> = labels.iter().filter_map(|l| l.name.as_deref()).collect();
let _ = print_json(&json!(names)); let _ = print_json(&json!(names));
@ -169,7 +198,7 @@ fn print_label_names(labels: &[Label]) {
#[cfg(test)] #[cfg(test)]
mod tests { mod tests {
use super::resolve_ids; use super::{resolve_ids, still_present_names};
use forgejo_api::structs::Label; use forgejo_api::structs::Label;
fn label(id: i64, name: &str) -> Label { fn label(id: i64, name: &str) -> Label {
@ -206,4 +235,35 @@ mod tests {
let err = resolve_ids(&[], &["anything".to_owned()]).unwrap_err(); let err = resolve_ids(&[], &["anything".to_owned()]).unwrap_err();
assert!(err.to_string().contains("(none)")); 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, &current).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, &current),
vec!["area/ops".to_owned()]
);
}
} }