swarm: merge a re-create into the stored agent permissions instead of overwriting them

docs/process/conventions.md

@@ -314,10 +314,12 @@ none. swarm-controller logs every refused `PUT` or `DELETE` on that route at `wa
 **Internet flag** — the record also carries `unrestricted_internet`, a
 boolean that isn't a `Permission`. An absent value means on, so every agent
 that existed before the field keeps the open access it had. `POST /api/agents`
-always writes the record with an explicit value, `false` unless the request
+writes `false` for an agent that has no record, unless the request names a
-names it. A `PUT` that leaves the field out and a `DELETE` (`swarmctl agent
+value. Over an existing record (a re-create) it keeps the stored list and flag,
-permissions clear`) keep the stored value; a `DELETE` on an agent that has one
+an absent flag included, and a value in the request wins. A `PUT` that leaves
-rewrites the record to just that value. Read it with
+the field out and a `DELETE` (`swarmctl agent permissions clear`) also keep the
+stored value; a `DELETE` on an agent that has one rewrites the record to just
+that value. Read it with
 `swarm_queue_client::agent_permissions::unrestricted`. Nothing acts on it yet.
 
 **How a change applies** —

swarm-controller-api/src/agents.rs

@@ -76,13 +76,15 @@ pub struct CreateAgentRequest {
     /// /api/agents/{name}/inference` takes them. Absent as `limits`.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub inference: Option<InferenceSettings>,
-    /// The agent's permissions, stored before the job is queued. Left out, the
+    /// The agent's permissions, stored before the job is queued. Left out, an
-    /// agent holds the default set.
+    /// existing record keeps its list and otherwise the agent holds the default
+    /// set.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     #[cfg_attr(feature = "schema", schema(value_type = Option<Vec<String>>))]
     pub permissions: Option<Vec<Permission>>,
-    /// Whether the agent's traffic leaves the host unfiltered. Left out, the
+    /// Whether the agent's traffic leaves the host unfiltered. Left out, an
-    /// agent is created with `false`.
+    /// agent with no stored value is created with `false`, and one that has a
+    /// record (a re-create) keeps its value.
     #[serde(default, skip_serializing_if = "Option::is_none")]
     pub unrestricted_internet: Option<bool>,
     /// The archetype to build the agent from, one `GET /api/archetypes`

swarm-controller/src/agent_permissions.rs

@@ -240,7 +240,8 @@ pub(crate) async fn delete_agent_permissions(
 }
 
 /// Delete a destroyed agent's key. A name is free again once its agent is
-/// destroyed, so it starts with no record.
+/// destroyed, and a create keeps the list and flag of a record it finds, so a
+/// key left here would carry over to the next agent of that name.
 ///
 /// Like the credential revoke it runs beside, a failure is logged and does
 /// not stop the destroy; declaring the agent destroyed again re-runs it.
@@ -260,8 +261,8 @@ pub(crate) async fn clear_on_destroy(state: &AppState, hive: &str, agent: &str)
             hive,
             agent,
             error = %format!("{e:#}"),
-            "clearing this agent's permissions FAILED; it was destroyed anyway, so a read \
+            "clearing this agent's permissions FAILED; it was destroyed anyway, so a new \
-             of that name finds the old record until a create writes a new one. \
+             agent of the same name created later keeps this record's list and flag. \
              Declaring the agent destroyed again re-runs this."
         ),
     }
@@ -291,9 +292,9 @@ pub(crate) async fn get_permission_catalogue() -> Json<Catalogue> {
     })
 }
 
-/// The value `POST /api/agents` stores, or why it is refused. `perms` left
+/// The value `POST /api/agents` asks [`store`] to merge in, or why it is
-/// out keeps the default set; `unrestricted_internet` left out is `false`, so
+/// refused. A field the request leaves out stays unset here: [`store`] keeps
-/// every new agent has an explicit value and only older records read as on.
+/// the stored value, and gives an agent with no stored flag `false`.
 /// The agent is not declared on its hive yet, so the "declared by a hive"
 /// check of [`put_agent_permissions`] does not apply.
 pub(crate) fn at_create(
@@ -305,14 +306,37 @@ pub(crate) fn at_create(
     }
     Ok(AgentPermissions {
         permissions: perms,
-        unrestricted_internet: Some(unrestricted_internet.unwrap_or(false)),
+        unrestricted_internet,
     })
 }
 
+/// What a create leaves in the key: the request's fields win, and a field it
+/// leaves out keeps the stored value. Only an agent with no record at all
+/// (`held` false) gets `unrestricted_internet: false`, so re-creating an
+/// existing agent keeps its list and its flag, a legacy record's absent flag
+/// included.
+fn merged_at_create(
+    value: &AgentPermissions,
+    current: &AgentPermissions,
+    held: bool,
+) -> AgentPermissions {
+    AgentPermissions {
+        permissions: value
+            .permissions
+            .clone()
+            .or_else(|| current.permissions.clone()),
+        unrestricted_internet: value
+            .unrestricted_internet
+            .or(current.unrestricted_internet)
+            .or_else(|| (!held).then_some(false)),
+    }
+}
+
 /// What [`store`] replaced, for [`restore`]; `None` when it wrote nothing.
 pub(crate) type Replaced = Option<crate::schedules::Replaced<AgentPermissions>>;
 
-/// Replace `agent`'s stored permissions with `value`.
+/// Merge `value` into `agent`'s stored permissions, as [`merged_at_create`]
+/// says.
 pub(crate) async fn store(
     client: &async_nats::Client,
     agent: &str,
@@ -323,17 +347,18 @@ pub(crate) async fn store(
         let (current, revision, undecodable) = read_or_default::<AgentPermissions>(&store, agent)
             .await
             .map_err(|e| queue_problem(&e))?;
-        if current == *value && !undecodable {
-            tracing::info!(%agent, ?value, "set agent permissions at create");
-            return Ok(None);
-        }
         // A deleted key reads as present at its marker's revision, so ask
-        // whether it holds a value before keeping one to put back.
+        // whether it holds a value.
         let held = store
             .get(agent)
             .await
             .map_err(|e| queue_problem(&anyhow::Error::new(e)))?
             .is_some();
+        let value = &merged_at_create(value, &current, held);
+        if current == *value && !undecodable {
+            tracing::info!(%agent, ?value, "set agent permissions at create");
+            return Ok(None);
+        }
         if let Some(written) = write_revision(&store, agent, value, revision)
             .await
             .map_err(|e| queue_problem(&e))?
@@ -697,7 +722,10 @@ mod tests {
 
         // Control: an agent that never had the flag has nothing left after a
         // clear, and a `PUT` does not invent one.
-        crate::agent_permissions::store(&client, "atlas", &super::AgentPermissions::default())
+        swarm_queue_client::agent_permissions::open_or_create(&client)
+            .await
+            .expect("bucket")
+            .put("atlas", "{}".into())
             .await
             .expect("an older record");
         let _ = put(&state, "atlas", granted()).await.expect("set");
@@ -709,6 +737,60 @@ mod tests {
         assert_eq!(published(&client, "atlas").await, None);
     }
 
+    /// A create over an existing record merges into it: a list and a flag
+    /// that were set stay, and a record without a flag (an older agent)
+    /// stays without one. Control: with no record, the flag is `false`.
+    #[tokio::test]
+    async fn a_re_create_keeps_the_record_it_finds() {
+        let nats = TestNats::start();
+        let client = nats.client().await;
+        let state = crate::agent_limits::tests::declared(&client).await;
+        let create = |permissions, unrestricted_internet| {
+            crate::routes::agents::create_agent(
+                State(state.clone()),
+                axum::Json(swarm_controller_api::agents::CreateAgentRequest {
+                    name: "atlas".to_owned(),
+                    hive: "pr1ma".to_owned(),
+                    limits: None,
+                    inference: None,
+                    permissions,
+                    unrestricted_internet,
+                    archetype: None,
+                    agents_md: None,
+                }),
+            )
+        };
+
+        let on = Permissions {
+            unrestricted_internet: Some(true),
+            ..granted()
+        };
+        let _ = put(&state, "atlas", on).await.expect("set");
+        let _ = create(None, None).await.expect("re-created");
+        assert_eq!(
+            published(&client, "atlas").await.as_deref(),
+            Some(r#"{"permissions":["manage_root_agent"],"unrestricted_internet":true}"#)
+        );
+
+        // A value in the request wins over the stored one.
+        let _ = create(None, Some(false)).await.expect("re-created");
+        assert_eq!(
+            published(&client, "atlas").await.as_deref(),
+            Some(r#"{"permissions":["manage_root_agent"],"unrestricted_internet":false}"#)
+        );
+
+        // A record from before the flag existed stays without one.
+        let older = r#"{"permissions":["manage_root_agent"]}"#;
+        swarm_queue_client::agent_permissions::open_or_create(&client)
+            .await
+            .expect("bucket")
+            .put("atlas", older.into())
+            .await
+            .expect("an older record");
+        let _ = create(None, None).await.expect("re-created");
+        assert_eq!(published(&client, "atlas").await.as_deref(), Some(older));
+    }
+
     /// A name no hive declares, or a name that is not an identifier, is a 400
     /// and writes nothing.
     #[tokio::test]