swarm-controller: don't fold every 422 into "already exists" on user/repo create

Forgejo answers 422 for six different causes on admin user create
(ErrUserAlreadyExist, ErrEmailAlreadyUsed, ErrNameReserved,
ErrNameCharsNotAllowed, ErrEmailInvalid, ErrNamePatternNotAllowed) and
several on repo create, but is_already_exists() treated every one of
them as a conflict. A reserved or otherwise-refused name silently
folded to Done, so CreateForgeUser reported success with no user
created, and the graph's real failure only surfaced one node later as
a misleading AddRepoMember error.

ensure_agent_user and ensure_org_repo now trust a 409 unconditionally
(folds_into_success) but confirm a 422 with a follow-up user_get /
repo_get before folding it to success; an unconfirmed 422 fails with
forgejo's own message at error level. Webhook registration still uses
the old is_already_exists — it has no comparable follow-up read, so it
is out of scope here.

Closes #4678

swarm-controller/src/forge.rs

@@ -191,7 +191,11 @@ impl Client {
     }
 
     /// Create `name` inside [`CONFIG_ORG`]. Idempotent: an existing repo
-    /// (409/422) is folded into success.
+    /// (409, or a 422 confirmed by a follow-up `repo_get`) is folded into
+    /// success. A 422 that is *not* an existing repo — forgejo also uses
+    /// it for other repo-create validation failures — fails with
+    /// forgejo's own message rather than being folded silently (see
+    /// [`folds_into_success`]).
     async fn ensure_org_repo(&self, name: &str) -> Result<()> {
         match self
             .api
@@ -202,11 +206,17 @@ impl Client {
                 tracing::info!(%name, "swarm forge: created repo in {CONFIG_ORG}");
                 Ok(())
             }
-            Err(e) if is_already_exists(&e) => {
+            Err(e) => {
+                let existing = is_ambiguous_validation_failure(&e)
+                    && self.api.repo_get(CONFIG_ORG, name).await.is_ok();
+                if folds_into_success(&e, existing) {
                     tracing::debug!(%name, "swarm forge: repo already exists");
                     Ok(())
+                } else {
+                    tracing::error!(%name, error = %e, "swarm forge: forge refused to create repo");
+                    Err(e).with_context(|| format!("create repo {CONFIG_ORG}/{name}"))
+                }
             }
-            Err(e) => Err(e).with_context(|| format!("create repo {CONFIG_ORG}/{name}")),
         }
     }
 
@@ -362,25 +372,25 @@ impl Client {
     /// but not its mechanism: that function shells out to the local
     /// `forgejo admin` CLI, which assumes co-location with the forge host.
     /// This daemon has no such assumption — like every other call in this
-    /// file, it only ever talks to the forge over HTTP — so this goes
-    /// through `admin_create_user` instead.
+    /// file, it only ever talks to the forge over HTTP — so this goes through
+    /// `admin_create_user` instead.
     ///
     /// The password itself is a throwaway: 32 random bytes, generated once,
     /// never persisted anywhere, and never needed again (unlike
     /// `hive-c0re`'s CLI path, which can ask forgejo to `--random-password`
     /// on its own, the HTTP admin API requires a real value up front — see
-    /// [`crate::webhook::generate_hex_secret`], reused here rather than
-    /// duplicated for the same reason a webhook secret and this password
-    /// are both "32 random bytes nothing reads back").
+    /// [`crate::webhook::generate_hex_secret`], reused here for the same
+    /// reason a webhook secret and this password are both "32 random bytes
+    /// nothing reads back").
     ///
-    /// Idempotent: an existing user (409/422) is folded into success, same
-    /// as [`Self::ensure_org_repo`]. Deliberately does not attempt to align
-    /// the account's email or disable its own repo-creation rights the way
-    /// `hive-c0re`'s per-hive provisioning does (`ensure_user_email`,
-    /// `ensure_repo_creation_disabled`) — this account only exists so
-    /// `AddRepoMember` has something to add, and the agent's owning hive
-    /// still runs its own full provisioning pass once the agent actually
-    /// spawns there, which self-heals both of those.
+    /// Idempotent, same as [`Self::ensure_org_repo`]: 409 folds to success
+    /// unconditionally, 422 only after a follow-up `user_get` confirms the
+    /// user (see [`folds_into_success`] — not every 422 means it exists).
+    /// Deliberately does not align the account's email or disable its own
+    /// repo-creation rights the way `hive-c0re`'s per-hive provisioning does
+    /// (`ensure_user_email`, `ensure_repo_creation_disabled`) — this account
+    /// only exists so `AddRepoMember` has something to add, and the owning
+    /// hive's own provisioning pass self-heals both once the agent spawns.
     pub async fn ensure_agent_user(&self, agent: &str) -> Result<()> {
         let password = crate::webhook::generate_hex_secret()
             .context("generating a throwaway password for the agent's forge account")?;
@@ -405,11 +415,17 @@ impl Client {
                 tracing::info!(%agent, "swarm forge: created agent forge user");
                 Ok(())
             }
-            Err(e) if is_already_exists(&e) => {
+            Err(e) => {
+                let existing =
+                    is_ambiguous_validation_failure(&e) && self.api.user_get(agent).await.is_ok();
+                if folds_into_success(&e, existing) {
                     tracing::debug!(%agent, "swarm forge: agent forge user already exists");
                     Ok(())
+                } else {
+                    tracing::error!(%agent, error = %e, "swarm forge: forge refused to create agent user");
+                    Err(e).with_context(|| format!("create forge user {agent}"))
+                }
             }
-            Err(e) => Err(e).with_context(|| format!("create forge user {agent}")),
         }
     }
 
@@ -1048,6 +1064,55 @@ fn is_already_exists(e: &ForgejoError) -> bool {
     }
 }
 
+/// Whether a create-call failure is forgejo's unambiguous "already
+/// exists" signal — HTTP 409. Unlike a 422 (see
+/// [`is_ambiguous_validation_failure`]), nothing else produces a 409 on
+/// these create calls, so folding it into success needs no follow-up
+/// confirmation.
+fn is_confirmed_conflict(e: &ForgejoError) -> bool {
+    match e {
+        ForgejoError::ApiError(api) => {
+            matches!(api.error_kind(), ApiErrorKind::Other(s) if *s == StatusCode::CONFLICT)
+        }
+        ForgejoError::UnexpectedStatusCode(s) => *s == StatusCode::CONFLICT,
+        _ => false,
+    }
+}
+
+/// Whether a create-call failure is forgejo's 422 "validation failed" —
+/// ambiguous on its own. Forgejo folds several distinct causes into this
+/// one status; an admin user create alone answers 422 for
+/// `ErrUserAlreadyExist`, `ErrEmailAlreadyUsed`, `ErrNameReserved`,
+/// `ErrNameCharsNotAllowed`, `ErrEmailInvalid` and
+/// `ErrNamePatternNotAllowed` — only the first of those means the object
+/// exists. A 422 alone is never enough to fold into success; the caller
+/// must confirm with a follow-up read first (see [`folds_into_success`]).
+fn is_ambiguous_validation_failure(e: &ForgejoError) -> bool {
+    match e {
+        ForgejoError::ApiError(api) => matches!(api.error_kind(), ApiErrorKind::ValidationFailed),
+        ForgejoError::UnexpectedStatusCode(s) => *s == StatusCode::UNPROCESSABLE_ENTITY,
+        _ => false,
+    }
+}
+
+/// Whether a create-call failure `e` should be folded into success, given
+/// whether a follow-up read found the object already present.
+/// `existing` is only consulted for the ambiguous 422 case — a 409 is
+/// trusted on its own, and any other error always fails.
+fn folds_into_success(e: &ForgejoError, existing: bool) -> bool {
+    is_confirmed_conflict(e) || (is_ambiguous_validation_failure(e) && existing)
+}
+
+#[cfg(test)]
+fn validation_failed_error() -> ForgejoError {
+    ForgejoError::ApiError(ApiErrorKind::ValidationFailed.into())
+}
+
+#[cfg(test)]
+fn conflict_error() -> ForgejoError {
+    ForgejoError::ApiError(ApiErrorKind::Other(StatusCode::CONFLICT).into())
+}
+
 #[cfg(test)]
 mod tests {
     use super::*;
@@ -1110,4 +1175,40 @@ mod tests {
         let adj = HashMap::from([(1, vec![2]), (2, vec![1])]);
         assert_eq!(transitive_reach(1, &adj), 1);
     }
+
+    // The three cases from the `forge-422-fold` issue: a create call's
+    // 422 with the object confirmed absent must fail (case 1), the same
+    // 422 with the object confirmed present must fold to success (case
+    // 2), and a 409 must fold to success without needing a follow-up
+    // read at all (case 3).
+    #[test]
+    fn folds_into_success_case1_422_absent_fails() {
+        assert!(!folds_into_success(&validation_failed_error(), false));
+    }
+
+    #[test]
+    fn folds_into_success_case2_422_present_is_done() {
+        assert!(folds_into_success(&validation_failed_error(), true));
+    }
+
+    #[test]
+    fn folds_into_success_case3_conflict_is_done() {
+        // `existing` is never consulted for a 409 — pass `false` to prove
+        // that, not just `true`.
+        assert!(folds_into_success(&conflict_error(), false));
+    }
+
+    // Invert-proof: the old `is_already_exists` folded every 422 into
+    // success regardless of whether the object existed — exactly the
+    // defect `folds_into_success` fixes. Demonstrate the old logic
+    // getting case 1 wrong before trusting the new one.
+    #[test]
+    fn old_is_already_exists_wrongly_returns_true_for_an_absent_object() {
+        let e = validation_failed_error();
+        assert!(is_already_exists(&e), "old logic: false positive on 422");
+        assert!(
+            !folds_into_success(&e, false),
+            "new logic: correctly refuses to fold a 422 with no confirmed existence"
+        );
+    }
 }