swarm-controller: refuse a new agent name the forge would reject

Every agent becomes a Forgejo user of the same name, and nothing upstream
of `CreateForgeUser` knew what Forgejo refuses: `admin`, `api`, `foo-` or a
41-character name passed name validation and failed one node into
provisioning with Forgejo's 422.

`nix/reserved-names.nix` gains the 22 reserved usernames of Forgejo
v16.0.5 (`models/user/user.go:639-680`) that `[a-z0-9-]` can spell, and
the bare `-` (`models/repo/repo.go:67`). The dot and underscore entries are
left out, since our charset cannot produce them. The header's admission
rule grows a third class — a username the forge refuses — because that is
a failure behind the refusal.

The shape rules are not literals, so they live in
`hive_types::forge_username_violation`: no leading `-`, no `--`, no
trailing `-`, at most 40 characters. Beside `is_reserved_name`, not in
`Ident::parse`: an `Ident` is also a hive, label, account and subagent
name, and parsing runs on every read of an existing name.

`create_agent` used to WARN on a reserved name, deliberately: an operator
with agents already created under a colliding name would otherwise be
unable to re-run creation. That reason is kept, and narrowed to what it
protects. A name breaking either rule is now refused with a 400 naming the
rule when the name is NOT in the swarm roster, and still only warned
about when it is, so re-creating an existing agent keeps working. The
roster is read only for a rule-breaking name; when it cannot be read, a new
name and an existing one look alike, and this warns as before. The
hive-collision warning is unchanged.

hive-types/src/lib.rs

@@ -64,6 +64,35 @@ pub fn is_reserved_name(name: &str, reserved: &[&str]) -> bool {
     reserved.contains(&name)
 }
 
+/// Forgejo's cap on a username (`MaxSize(40)` on the admin create-user body,
+/// `modules/structs/admin_user.go:14` in v16.0.5) — tighter than
+/// [`Ident::MAX_LEN`], so an agent name can be a valid [`Ident`] and still
+/// never get a forge account.
+pub const FORGE_USERNAME_MAX_LEN: usize = 40;
+
+/// The Forgejo username shape rule an [`Ident`]-valid `name` breaks, or
+/// `None`. Forgejo v16.0.5 `modules/validation/helpers.go:97-108`.
+///
+/// The shape half of the forge's refusals; its reserved literals are in
+/// `nix/reserved-names.nix`. Creation sites only, for the reason given at
+/// [`is_reserved_name`] — and not inside [`Ident::parse`], because an
+/// `Ident` is also a hive, label, account and subagent name that never
+/// becomes a forge user.
+#[must_use]
+pub fn forge_username_violation(name: &str) -> Option<&'static str> {
+    if name.starts_with('-') {
+        Some("must not start with '-'")
+    } else if name.contains("--") {
+        Some("must not contain '--'")
+    } else if name.ends_with('-') {
+        Some("must not end with '-'")
+    } else if name.len() > FORGE_USERNAME_MAX_LEN {
+        Some("must be 40 characters or fewer")
+    } else {
+        None
+    }
+}
+
 /// A validated hive identifier: 1-63 chars of `[a-z0-9-]`.
 ///
 /// The single ident type for agent names, forge labels, and matrix / github
@@ -156,7 +185,10 @@ impl<'de> serde::Deserialize<'de> for Ident {
 
 #[cfg(test)]
 mod ident_tests {
-    use super::{Ident, is_reserved_name, parse_reserved_names};
+    use super::{
+        FORGE_USERNAME_MAX_LEN, Ident, forge_username_violation, is_reserved_name,
+        parse_reserved_names,
+    };
 
     #[test]
     fn accepts_canonical_shapes() {
@@ -240,6 +272,36 @@ mod ident_tests {
         }
     }
 
+    /// Each Forgejo shape rule is named on its own, so a refusal tells the
+    /// operator which character to change. The 63-char case is an `Ident`
+    /// that is still no forge username: the gap between the two caps.
+    #[test]
+    fn forge_shape_rules_are_each_refused() {
+        let over = "a".repeat(FORGE_USERNAME_MAX_LEN + 1);
+        let longest_ident = "a".repeat(Ident::MAX_LEN);
+        for (bad, rule) in [
+            ("-", "must not start with '-'"),
+            ("-agent", "must not start with '-'"),
+            ("my--agent", "must not contain '--'"),
+            ("agent-", "must not end with '-'"),
+            (over.as_str(), "must be 40 characters or fewer"),
+            (longest_ident.as_str(), "must be 40 characters or fewer"),
+        ] {
+            assert!(Ident::parse(bad).is_ok(), "{bad:?} must be a valid Ident");
+            assert_eq!(forge_username_violation(bad), Some(rule), "{bad:?}");
+        }
+    }
+
+    /// The accept arm: without it, a predicate refusing everything passes
+    /// the test above. 40 characters is the boundary, not past it.
+    #[test]
+    fn forge_shape_rules_accept_ordinary_names() {
+        let at_cap = "a".repeat(FORGE_USERNAME_MAX_LEN);
+        for ok in ["my-agent", "a", "v3", "a-b-c", at_cap.as_str()] {
+            assert_eq!(forge_username_violation(ok), None, "{ok:?}");
+        }
+    }
+
     #[test]
     fn round_trips_and_serde_validates() {
         let id = Ident::parse("damocles").unwrap();

nix/reserved-names.nix

@@ -22,9 +22,9 @@
 # here forbids it for both. That is the point: two lists is how they drift.
 #
 # ⚠️ Every entry must be a value some component actually PRODUCES as a message
-# `from`/`to`, or a component name the collector builds pipelines from — not a
+# `from`/`to`, a component name the collector builds pipelines from, or a
-# word that merely looked risky. A name in here that nothing emits is a refusal
+# username the forge refuses — not a word that merely looked risky. A name in
-# with no failure behind it.
+# here that nothing emits or refuses is a refusal with no failure behind it.
 #
 # ⚠️ Matched by EQUALITY. Words forbidden *inside* a hive name live in
 # `./reserved-hive-fragments.nix` — read its header before merging the two.
@@ -59,6 +59,37 @@
   # pushes into a pipeline that routes nowhere. Previously enforced only
   # against hive names, in `swarm-otel.nix`'s own `reservedOwners`.
   "swarm"
+
+  # ---- usernames the forge refuses -------------------------------------
+  # Every agent becomes a Forgejo user of the same name, and Forgejo answers
+  # these with a 422 one node into provisioning: `reservedUsernames` in
+  # Forgejo v16.0.5 `models/user/user.go:639-680`, and the bare `-` that
+  # `models/repo/repo.go:67` also refuses as a repo name. Forgejo's shape
+  # rules are not literals and live in `hive_types::forge_username_violation`.
+  # Its entries containing `.` or `_` are left out: `[a-z0-9-]` cannot spell them.
+  "-"
+  "admin"
+  "api"
+  "assets"
+  "attachments"
+  "avatar"
+  "avatars"
+  "captcha"
+  "explore"
+  "forgejo-actions"
+  "ghost"
+  "gitea-actions"
+  "issues"
+  "login"
+  "metrics"
+  "milestones"
+  "notifications"
+  "org"
+  "pulls"
+  "repo"
+  "repo-avatars"
+  "user"
+  "v2"
 ]
 # Deliberately absent, and it is a load-bearing omission:
 #

swarm-controller/src/main.rs

@@ -560,10 +560,12 @@ struct AppState {
     /// one are different facts.
     ///
     /// ⚠️ The two `/api/agents` verbs treat this differently on purpose.
-    /// **POST does not consult it**: creation only queues a job, and
+    /// **POST does not need it**: creation only queues a job, and
     /// whether a bridge exists is `run_swarm_node`'s concern (it holds its
     /// own clone from `spawn_jobq_worker`), so a request still queues
-    /// cleanly on a bridge-less host and fails loud once claimed. **GET
+    /// cleanly on a bridge-less host and fails loud once claimed. POST reads
+    /// it only for a name that breaks a naming rule, to tell a new agent
+    /// from an existing one, and warns when it cannot (`name_verdict`). **GET
     /// has nowhere to defer to** — there is no job, only an answer it
     /// either has or does not.
     auth: Option<Arc<auth::AuthBridge>>,
@@ -1199,7 +1201,7 @@ struct CreateAgentResponse {
     request_body = CreateAgentRequest,
     responses(
         (status = 200, description = "job chain queued", body = CreateAgentResponse),
-        (status = 400, description = "`name` or `hive` is not a valid identifier, or `hive` is not in this swarm (problem+json)", body = String),
+        (status = 400, description = "`name` or `hive` is not a valid identifier, `name` is new and reserved or refused by the forge, or `hive` is not in this swarm (problem+json)", body = String),
         (status = 500, description = "the job chain could not be queued (problem+json)", body = String),
     ),
     tag = "agents"
@@ -1222,43 +1224,24 @@ async fn create_agent(
     // `state.hives` is the swarm directory loaded from `SWARM_CONTROLLER_HIVES`
     // at startup and never mutated, so this is a scan of a handful of
     // entries against a value an operator just chose.
-    // The name is shaped like an identifier; these two ask whether it is
+    // The name is shaped like an identifier; the checks below ask whether it
-    // *available*. Both WARN rather than refuse: an operator with agents
+    // is *available*. An operator with agents already created under a
-    // already created under a colliding name would otherwise be unable to
+    // colliding name must still be able to re-run creation, so a name that
-    // re-run creation at all, so the refusal comes after a grace period,
+    // breaks a naming rule is refused only when it is new — see
-    // once the warning has had time to be seen.
+    // `name_verdict`. The hive collision further down still only warns, on
+    // its grace period, once the warning has had time to be seen.
     //
     // Collected rather than logged-and-dropped — see `CreateAgentResponse`.
     let mut warnings = Vec::new();
-    // The blacklist comes from nix via `HIVE_RESERVED_NAMES` — one file, read
+    let broken = broken_name_rules(
-    // by this daemon, by hive-c0re and by the swarm collector's own owner
+        &agent,
-    // assertion. An UNSET variable means this process was never told, which
+        hive_types::reserved_names_raw().as_deref(),
-    // is not the same as "no name is reserved": staying quiet there would be
+        &mut warnings,
-    // a check that reports clean because it could not run.
-    let raw = hive_types::reserved_names_raw();
-    match raw.as_deref().map(hive_types::parse_reserved_names) {
-        None => {
-            tracing::error!(
-                var = hive_types::RESERVED_NAMES_ENV,
-                "create_agent: reserved-name check could not run — variable not set"
     );
-            warnings.push(format!(
+    if !broken.is_empty() {
-                "the reserved-name check did not run: {} is unset, so {agent:?} was accepted \
+        let existing = in_roster(state.auth.as_deref(), &agent).await;
-                 without being checked against the protocol literals",
+        name_verdict(&agent, &broken, existing, &mut warnings)
-                hive_types::RESERVED_NAMES_ENV
+            .map_err(|detail| error_problem(axum::http::StatusCode::BAD_REQUEST, &detail))?;
-            ));
-        }
-        Some(reserved) if hive_types::is_reserved_name(&agent, &reserved) => {
-            // A protocol literal: the message layer already produces this
-            // name as a sender or recipient, so wakes from the component and
-            // messages from the agent become the same broker row.
-            tracing::warn!(agent = %agent, "create_agent: reserved name");
-            warnings.push(format!(
-                "agent name {agent:?} is a reserved protocol name — messages from this agent will \
-                 be indistinguishable from hyperhive's own; this will become an error"
-            ));
-        }
-        Some(_) => {}
     }
 
     let hive = hive_types::Ident::parse(&req.hive)
@@ -1312,6 +1295,107 @@ async fn create_agent(
     }))
 }
 
+/// Every naming rule `agent` breaks, each as a sentence naming the rule.
+///
+/// `reserved` is [`hive_types::reserved_names_raw`]: the list nix owns in
+/// `nix/reserved-names.nix`, read by this daemon, by hive-c0re and by the
+/// swarm collector's own owner assertion. `None` means this process was never
+/// told, which is not the same as "no name is reserved" — staying quiet there
+/// would be a check that reports clean because it could not run, so it goes
+/// onto `warnings` instead.
+fn broken_name_rules(
+    agent: &str,
+    reserved: Option<&str>,
+    warnings: &mut Vec<String>,
+) -> Vec<String> {
+    let mut broken = Vec::new();
+    match reserved.map(hive_types::parse_reserved_names) {
+        None => {
+            tracing::error!(
+                var = hive_types::RESERVED_NAMES_ENV,
+                "create_agent: reserved-name check could not run — variable not set"
+            );
+            warnings.push(format!(
+                "the reserved-name check did not run: {} is unset, so {agent:?} was accepted \
+                 without being checked against the reserved names",
+                hive_types::RESERVED_NAMES_ENV
+            ));
+        }
+        Some(reserved) if hive_types::is_reserved_name(agent, &reserved) => {
+            broken.push(format!(
+                "agent name {agent:?} is reserved (nix/reserved-names.nix): hyperhive's message \
+                 layer already uses it, or the forge refuses it as a username"
+            ));
+        }
+        Some(_) => {}
+    }
+    if let Some(rule) = hive_types::forge_username_violation(agent) {
+        broken.push(format!(
+            "agent name {agent:?} {rule}: the forge refuses it as a username"
+        ));
+    }
+    broken
+}
+
+/// Whether `agent` is already in the swarm roster, or why that could not be
+/// told. Asked only of a name that breaks a rule, so an ordinary creation
+/// still never waits on the bridge.
+async fn in_roster(auth: Option<&auth::AuthBridge>, agent: &str) -> Result<bool, String> {
+    let Some(auth) = auth else {
+        return Err("no identity bridge is configured".to_owned());
+    };
+    auth.list_agent_identities()
+        .await
+        .map(|roster| roster.iter().any(|name| name == agent))
+        .map_err(|e| format!("{e:#}"))
+}
+
+/// Refuse a NEW agent whose name breaks a rule; warn for one that exists.
+///
+/// Re-creating an agent under its own name is a migration path (mara, on
+/// the duplicate-name refusal), so an agent already in the roster keeps its
+/// name with a warning. When the roster cannot be read, a new name and an
+/// existing one look the same, and this warns rather than refuse: refusing
+/// would block that re-creation, and the forge still refuses the account one
+/// node in, as it did before.
+///
+/// # Errors
+/// Every rule `agent` breaks, as one caller-ready sentence, when `agent` is
+/// not in the roster.
+fn name_verdict(
+    agent: &str,
+    broken: &[String],
+    existing: Result<bool, String>,
+    warnings: &mut Vec<String>,
+) -> Result<(), String> {
+    match existing {
+        Ok(false) => Err(format!("{}; choose another name", broken.join("; "))),
+        Ok(true) => {
+            tracing::warn!(agent, "create_agent: existing agent breaks a naming rule");
+            warnings.extend(
+                broken
+                    .iter()
+                    .map(|rule| format!("{rule} — accepted only because {agent:?} already exists")),
+            );
+            Ok(())
+        }
+        Err(why) => {
+            tracing::warn!(
+                agent,
+                why,
+                "create_agent: naming rule broken, roster unreadable"
+            );
+            warnings.extend(broken.iter().map(|rule| {
+                format!(
+                    "{rule} — not refused, because whether {agent:?} already exists could not \
+                     be checked: {why}"
+                )
+            }));
+            Ok(())
+        }
+    }
+}
+
 /// The authority agent leaves are issued from, or `None` on a host that was
 /// given none.
 ///
@@ -2078,8 +2162,8 @@ mod tests {
             config_prs: None,
             swarm_name: None,
             // No bridge: these tests drive agent *creation*, which queues a
-            // job and never consults one. The roster read is the verb that
+            // job and consults one only for a rule-breaking name. The roster
-            // needs it, and it has its own test below.
+            // read is the verb that needs it, and it has its own test below.
             auth: None,
             forge: None,
         };
@@ -2191,6 +2275,113 @@ mod tests {
         assert!(queued > 0, "an accepted creation must queue work");
     }
 
+    /// Forgejo v16.0.5's reserved usernames our charset can spell, plus the
+    /// bare `-` — the forge section of `nix/reserved-names.nix`. Read from
+    /// the list nix exports, so dropping one from the file reds this.
+    const FORGE_REFUSED_LITERALS: [&str; 23] = [
+        "-",
+        "admin",
+        "api",
+        "assets",
+        "attachments",
+        "avatar",
+        "avatars",
+        "captcha",
+        "explore",
+        "forgejo-actions",
+        "ghost",
+        "gitea-actions",
+        "issues",
+        "login",
+        "metrics",
+        "milestones",
+        "notifications",
+        "org",
+        "pulls",
+        "repo",
+        "repo-avatars",
+        "user",
+        "v2",
+    ];
+
+    /// The list nix renders, panicking when absent for the reason
+    /// `hive-sh4re`'s drift test gives: a skip would read as green.
+    fn nix_reserved() -> String {
+        hive_types::reserved_names_raw()
+            .expect("HIVE_RESERVED_NAMES is exported by nix/checks.nix and nix/devshell.nix")
+    }
+
+    #[test]
+    fn a_new_agent_named_a_forge_refused_literal_is_refused() {
+        let raw = nix_reserved();
+        for name in FORGE_REFUSED_LITERALS {
+            let mut warnings = Vec::new();
+            let broken = super::broken_name_rules(name, Some(&raw), &mut warnings);
+            let err = super::name_verdict(name, &broken, Ok(false), &mut warnings).expect_err(name);
+            assert!(err.contains("nix/reserved-names.nix"), "{name:?}: {err}");
+        }
+    }
+
+    /// One case per shape rule, each named in the refusal. Checked against a
+    /// list without them, so the literal half cannot carry this test.
+    #[test]
+    fn a_new_agent_breaking_a_forge_shape_rule_is_refused() {
+        let over = "a".repeat(hive_types::FORGE_USERNAME_MAX_LEN + 1);
+        for (name, rule) in [
+            ("-agent", "must not start with '-'"),
+            ("my--agent", "must not contain '--'"),
+            ("agent-", "must not end with '-'"),
+            (over.as_str(), "must be 40 characters or fewer"),
+        ] {
+            let mut warnings = Vec::new();
+            let broken = super::broken_name_rules(name, Some("operator"), &mut warnings);
+            let err = super::name_verdict(name, &broken, Ok(false), &mut warnings).expect_err(name);
+            assert!(err.contains(rule), "{name:?}: {err}");
+        }
+    }
+
+    /// The control for both tests above, against the real list.
+    #[test]
+    fn an_ordinary_name_and_a_40_char_name_break_no_rule() {
+        let raw = nix_reserved();
+        let at_cap = "a".repeat(hive_types::FORGE_USERNAME_MAX_LEN);
+        for name in ["my-agent", at_cap.as_str()] {
+            let mut warnings = Vec::new();
+            let broken = super::broken_name_rules(name, Some(&raw), &mut warnings);
+            assert!(broken.is_empty(), "{name:?}: {broken:?}");
+            assert!(warnings.is_empty(), "{name:?}: {warnings:?}");
+        }
+    }
+
+    /// Re-creating an agent under its own name must keep working: a name
+    /// already in the roster is warned about, never refused.
+    #[test]
+    fn an_existing_agent_keeps_a_rule_breaking_name_with_a_warning() {
+        let mut warnings = Vec::new();
+        let broken = super::broken_name_rules("admin", Some("admin"), &mut warnings);
+        super::name_verdict("admin", &broken, Ok(true), &mut warnings)
+            .expect("an existing agent must not be refused");
+        assert_eq!(warnings.len(), 1, "{warnings:?}");
+        assert!(warnings[0].contains("already exists"), "{warnings:?}");
+    }
+
+    /// A roster that cannot be read cannot tell new from existing, so it
+    /// warns — and the warning says the check did not decide.
+    #[test]
+    fn an_unreadable_roster_warns_rather_than_refusing() {
+        let mut warnings = Vec::new();
+        let broken = super::broken_name_rules("agent-", Some("operator"), &mut warnings);
+        super::name_verdict(
+            "agent-",
+            &broken,
+            Err("bridge down".to_owned()),
+            &mut warnings,
+        )
+        .expect("an unreadable roster must not refuse");
+        assert_eq!(warnings.len(), 1, "{warnings:?}");
+        assert!(warnings[0].contains("bridge down"), "{warnings:?}");
+    }
+
     /// The backfill route's roster check, asserted by effect for the same
     /// reason its sibling above is: a refusal that queued first would still
     /// re-mint the agent's certificate, which every running agent on the