hive-forge: attach-issue/attach-comment fail on a missing download URL

print_url only printed when browser_download_url was Some, so a 2xx
upload response with no URL printed nothing and still exited 0 — a
caller doing url=$(hive-forge attach-issue …) got an empty string with
no signal anything went wrong.

require_url now errors, naming the attachment's id/name, when the
forge returns no download URL.

hive-forge/src/verbs/attach.rs

@@ -51,7 +51,7 @@ pub fn run_issue(client: &Client, args: IssueArgs) -> Result<()> {
         .api()
         .issue_create_issue_attachment(owner, name, index(args.number)?, &bytes, query)
         .send()?;
-    print_url(&resp);
+    println!("{}", require_url(&resp)?);
     Ok(())
 }
 
@@ -78,7 +78,7 @@ pub fn run_comment(client: &Client, args: CommentArgs) -> Result<()> {
         .api()
         .issue_create_issue_comment_attachment(owner, name, index(args.id)?, &bytes, query)
         .send()?;
-    print_url(&resp);
+    println!("{}", require_url(&resp)?);
     Ok(())
 }
 
@@ -93,8 +93,68 @@ fn file_name(path: &Path) -> Option<String> {
     path.file_name().map(|n| n.to_string_lossy().into_owned())
 }
 
-fn print_url(v: &Attachment) {
-    if let Some(url) = &v.browser_download_url {
-        println!("{url}");
+/// The uploaded attachment's browser download URL, hard-erroring if the
+/// forge answered 2xx with none. A caller piping the printed URL (e.g.
+/// `url=$(hive-forge attach-issue …)`) used to get an empty string and no
+/// signal when this was `None` — the upload had happened, but nothing said
+/// so, and nothing said it hadn't.
+fn require_url(v: &Attachment) -> Result<url::Url> {
+    v.browser_download_url.clone().ok_or_else(|| {
+        anyhow::anyhow!(
+            "hive-forge attach: upload succeeded but the forge returned no download URL \
+             (attachment {})",
+            attachment_ident(v)
+        )
+    })
+}
+
+/// Best-effort identifier for an attachment error message — id if present,
+/// else name, else a placeholder so the message never comes out empty.
+fn attachment_ident(v: &Attachment) -> String {
+    match (v.id, v.name.as_deref()) {
+        (Some(id), Some(n)) => format!("id={id} name={n}"),
+        (Some(id), None) => format!("id={id}"),
+        (None, Some(n)) => format!("name={n}"),
+        (None, None) => "<unknown>".to_owned(),
+    }
+}
+
+#[cfg(test)]
+mod tests {
+    use super::require_url;
+    use forgejo_api::structs::Attachment;
+
+    fn attachment(url: Option<&str>) -> Attachment {
+        Attachment {
+            browser_download_url: url.map(|u| u.parse().unwrap()),
+            created_at: None,
+            download_count: None,
+            id: Some(7),
+            name: Some("out.log".to_owned()),
+            size: None,
+            r#type: None,
+            uuid: None,
+        }
+    }
+
+    #[test]
+    fn control_present_url_is_returned() {
+        let a = attachment(Some("https://forge.example/a/1"));
+        assert_eq!(
+            require_url(&a).unwrap().as_str(),
+            "https://forge.example/a/1"
+        );
+    }
+
+    #[test]
+    fn missing_url_errors_naming_the_attachment() {
+        // A 2xx response with no browser_download_url used to print nothing
+        // and exit 0 — a caller doing `url=$(hive-forge attach-issue …)`
+        // got an empty string with no signal anything went wrong.
+        let a = attachment(None);
+        let err = require_url(&a).unwrap_err();
+        let msg = err.to_string();
+        assert!(msg.contains("id=7"), "missing attachment id: {msg}");
+        assert!(msg.contains("out.log"), "missing attachment name: {msg}");
     }
 }

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, &current);
+            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(&current);
         }
     }
     Ok(())
@@ -162,6 +176,21 @@ fn lookup_id(all: &[Label], name: &str) -> Option<i64> {
         .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]) {
     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, &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()]
+        );
+    }
 }