refactor(spaces): fix clippy, extract testable functions, improve test coverage

Fix all clippy warnings in space roles files: dangerous `as` casts,
`to_string()` on &str, format string inlining, items-after-statements,
needless borrows, large futures, semicolons outside blocks, and
let-else patterns.

Extract `compute_user_power_level` and `roles_satisfy_requirements` as
pure free functions so the core logic can be unit-tested without async
service dependencies. Update all tests in tests.rs and
integration_tests.rs to call the real extracted functions instead of
reimplementing the logic inline.

Add negative deserialization tests for RoleDefinition,
SpaceRoleMemberEventContent, and SpaceRoleRoomEventContent. Improve
doc comments on handle_* methods and add module-level documentation to
cache_tests.rs.

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

src/admin/space/roles.rs

@@ -1,3 +1,5 @@
+use std::fmt::Write;
+
 use clap::Subcommand;
 use conduwuit::{Err, Event, Result};
 use conduwuit_core::matrix::space_roles::{
@@ -134,13 +136,13 @@ async fn list(&self, space: OwnedRoomOrAliasId) -> Result {
 		return self.write_str("No roles defined in this space.").await;
 	}
 
-	let mut msg = format!("Roles in {}:\n```\n", space_id);
+	let mut msg = format!("Roles in {space_id}:\n```\n");
 	for (name, def) in &content.roles {
 		let pl = def
 			.power_level
 			.map(|p| format!(" (power_level: {p})"))
 			.unwrap_or_default();
-		msg.push_str(&format!("- {name}: {}{pl}\n", def.description));
+		let _ = writeln!(msg, "- {name}: {}{pl}", def.description);
 	}
 	msg.push_str("```");
 
@@ -332,7 +334,7 @@ async fn assign(
 
 	if !role_defs.roles.contains_key(&role_name) {
 		return self
-			.write_str(&format!("Error: Role '{}' does not exist in this space.", role_name))
+			.write_str(&format!("Error: Role '{role_name}' does not exist in this space."))
 			.await;
 	}
 
@@ -438,7 +440,7 @@ async fn require(
 
 	if !role_defs.roles.contains_key(&role_name) {
 		return self
-			.write_str(&format!("Error: Role '{}' does not exist in this space.", role_name))
+			.write_str(&format!("Error: Role '{role_name}' does not exist in this space."))
 			.await;
 	}
 

src/core/matrix/space_roles.rs

@@ -181,4 +181,22 @@ mod tests {
 		let content: SpaceRoleMemberEventContent = serde_json::from_str(json).unwrap();
 		assert_eq!(content.roles, vec!["nsfw"]);
 	}
+
+	#[test]
+	fn missing_description_fails() {
+		let json = r#"{"power_level":100}"#;
+		assert!(serde_json::from_str::<RoleDefinition>(json).is_err());
+	}
+
+	#[test]
+	fn wrong_type_for_roles_fails() {
+		let json = r#"{"roles":"not_an_array"}"#;
+		assert!(serde_json::from_str::<SpaceRoleMemberEventContent>(json).is_err());
+	}
+
+	#[test]
+	fn wrong_type_for_required_roles_fails() {
+		let json = r#"{"required_roles":42}"#;
+		assert!(serde_json::from_str::<SpaceRoleRoomEventContent>(json).is_err());
+	}
 }

src/service/rooms/roles/cache_tests.rs

@@ -1,4 +1,8 @@
-//! Tests for cache consistency of the space roles index structures.
+//! Cache consistency tests using a MockCache that mirrors the Service's
+//! cache structures. These tests validate the algorithm/logic but do NOT
+//! exercise the actual Service methods (which require async service
+//! dependencies). See `tests.rs` for tests that call the extracted pure
+//! logic functions directly.
 
 use std::collections::{BTreeMap, HashMap, HashSet};
 

src/service/rooms/roles/integration_tests.rs

@@ -2,6 +2,7 @@ use std::collections::{HashMap, HashSet};
 
 use ruma::{room_id, user_id};
 
+use super::{compute_user_power_level, roles_satisfy_requirements};
 use super::tests::{make_requirements, make_roles, make_user_roles};
 
 #[test]
@@ -9,13 +10,13 @@ fn scenario_user_gains_and_loses_access() {
 	let room_reqs = make_requirements(&["nsfw"]);
 
 	let no_roles: HashSet<String> = HashSet::new();
-	assert!(!room_reqs.iter().all(|r| no_roles.contains(r)));
+	assert!(!roles_satisfy_requirements(&room_reqs, &no_roles));
 
 	let with_nsfw = make_user_roles(&["nsfw"]);
-	assert!(room_reqs.iter().all(|r| with_nsfw.contains(r)));
+	assert!(roles_satisfy_requirements(&room_reqs, &with_nsfw));
 
 	let no_roles: HashSet<String> = HashSet::new();
-	assert!(!room_reqs.iter().all(|r| no_roles.contains(r)));
+	assert!(!roles_satisfy_requirements(&room_reqs, &no_roles));
 }
 
 #[test]
@@ -24,12 +25,12 @@ fn scenario_room_adds_requirement_existing_members_checked() {
 	let bob_roles = make_user_roles(&["vip", "nsfw"]);
 
 	let empty_reqs: HashSet<String> = HashSet::new();
-	assert!(empty_reqs.iter().all(|r| alice_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&empty_reqs, &alice_roles));
-	assert!(empty_reqs.iter().all(|r| bob_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&empty_reqs, &bob_roles));
 
 	let new_reqs = make_requirements(&["nsfw"]);
-	assert!(!new_reqs.iter().all(|r| alice_roles.contains(r)));
+	assert!(!roles_satisfy_requirements(&new_reqs, &alice_roles));
-	assert!(new_reqs.iter().all(|r| bob_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&new_reqs, &bob_roles));
 }
 
 #[test]
@@ -41,13 +42,13 @@ fn scenario_multiple_rooms_different_requirements() {
 	let vip_reqs = make_requirements(&["vip"]);
 	let both_reqs = make_requirements(&["nsfw", "vip"]);
 
-	assert!(nsfw_reqs.iter().all(|r| alice_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&nsfw_reqs, &alice_roles));
-	assert!(vip_reqs.iter().all(|r| alice_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&vip_reqs, &alice_roles));
-	assert!(both_reqs.iter().all(|r| alice_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&both_reqs, &alice_roles));
 
-	assert!(nsfw_reqs.iter().all(|r| bob_roles.contains(r)));
+	assert!(roles_satisfy_requirements(&nsfw_reqs, &bob_roles));
-	assert!(!vip_reqs.iter().all(|r| bob_roles.contains(r)));
+	assert!(!roles_satisfy_requirements(&vip_reqs, &bob_roles));
-	assert!(!both_reqs.iter().all(|r| bob_roles.contains(r)));
+	assert!(!roles_satisfy_requirements(&both_reqs, &bob_roles));
 }
 
 #[test]
@@ -59,22 +60,10 @@ fn scenario_power_level_cascading_highest_wins() {
 	]);
 
 	let admin_mod = make_user_roles(&["admin", "mod"]);
-	assert_eq!(
+	assert_eq!(compute_user_power_level(&roles, &admin_mod), Some(100));
-		admin_mod
-			.iter()
-			.filter_map(|r| roles.get(r)?.power_level)
-			.max(),
-		Some(100)
-	);
 
 	let helper = make_user_roles(&["helper"]);
-	assert_eq!(
+	assert_eq!(compute_user_power_level(&roles, &helper), Some(25));
-		helper
-			.iter()
-			.filter_map(|r| roles.get(r)?.power_level)
-			.max(),
-		Some(25)
-	);
 }
 
 #[test]
@@ -107,7 +96,7 @@ fn scenario_identify_auto_join_candidates() {
 
 	let qualifying: Vec<_> = room_reqs
 		.iter()
-		.filter(|(_, reqs)| reqs.iter().all(|r| alice_roles.contains(r)))
+		.filter(|(_, reqs)| roles_satisfy_requirements(reqs, &alice_roles))
 		.map(|(name, _)| name.clone())
 		.collect();
 
@@ -132,7 +121,7 @@ fn scenario_identify_kick_candidates_after_role_revocation() {
 
 	let kick_from: Vec<_> = rooms
 		.iter()
-		.filter(|(_, reqs)| !reqs.iter().all(|r| alice_roles_after.contains(r)))
+		.filter(|(_, reqs)| !roles_satisfy_requirements(reqs, &alice_roles_after))
 		.map(|(name, _)| name.clone())
 		.collect();
 

src/service/rooms/roles/mod.rs

@@ -150,7 +150,7 @@ impl crate::Service for Service {
 				| Ok(RoomType::Space) => {
 					debug!("Populating space roles cache for {room_id}");
 					self.populate_space(room_id).await;
-					space_count += 1;
+					space_count = space_count.saturating_add(1);
 				},
 				| _ => continue,
 			}
@@ -241,7 +241,7 @@ pub async fn populate_space(&self, space_id: &RoomId) {
 	}
 
 	// Check cache capacity — if over limit, clear and let spaces repopulate on demand
-	if self.roles.read().await.len() >= self.server.config.space_roles_cache_capacity as usize {
+	if self.roles.read().await.len() >= usize::try_from(self.server.config.space_roles_cache_capacity).unwrap_or(usize::MAX) {
 		self.roles.write().await.clear();
 		self.user_roles.write().await.clear();
 		self.room_requirements.write().await.clear();
@@ -376,8 +376,8 @@ pub async fn populate_space(&self, space_id: &RoomId) {
 		// Update forward index
 		{
 			let mut space_to_rooms = self.space_to_rooms.write().await;
-			space_to_rooms.insert(space_id.to_owned(), child_rooms.iter().cloned().collect());
+			space_to_rooms.insert(space_id.to_owned(), child_rooms.iter().cloned().collect())
-		}
+		};
 
 		// Single write lock for all children — clear stale entries first
 		{
@@ -398,6 +398,27 @@ pub async fn populate_space(&self, space_id: &RoomId) {
 	}
 }
 
+/// Compute the maximum power level from a user's assigned roles.
+#[must_use]
+pub fn compute_user_power_level<S: ::std::hash::BuildHasher>(
+	role_defs: &BTreeMap<String, RoleDefinition>,
+	assigned: &HashSet<String, S>,
+) -> Option<i64> {
+	assigned
+		.iter()
+		.filter_map(|role_name| role_defs.get(role_name)?.power_level)
+		.max()
+}
+
+/// Check if a set of assigned roles satisfies all requirements.
+#[must_use]
+pub fn roles_satisfy_requirements<S: ::std::hash::BuildHasher>(
+	required: &HashSet<String, S>,
+	assigned: &HashSet<String, S>,
+) -> bool {
+	required.iter().all(|r| assigned.contains(r))
+}
+
 /// Get a user's effective power level from Space roles.
 /// Returns None if user has no roles with power levels.
 #[implement(Service)]
@@ -406,18 +427,9 @@ pub async fn get_user_power_level(
 	space_id: &RoomId,
 	user_id: &UserId,
 ) -> Option<i64> {
-	let role_defs = {
+	let role_defs = { self.roles.read().await.get(space_id).cloned()? };
-		let guard = self.roles.read().await;
+	let user_assigned = { self.user_roles.read().await.get(space_id)?.get(user_id).cloned()? };
-		guard.get(space_id).cloned()?
+	compute_user_power_level(&role_defs, &user_assigned)
-	};
-	let user_assigned = {
-		let guard = self.user_roles.read().await;
-		guard.get(space_id)?.get(user_id).cloned()?
-	};
-	user_assigned
-		.iter()
-		.filter_map(|role_name| role_defs.get(role_name)?.power_level)
-		.max()
 }
 
 /// Check if a user has all required roles for a room.
@@ -451,7 +463,7 @@ pub async fn user_qualifies_for_room(
 		};
 		assigned.clone()
 	};
-	required.iter().all(|r| user_assigned.contains(r))
+	roles_satisfy_requirements(&required, &user_assigned)
 }
 
 /// Get the parent Spaces of a child room, if any.
@@ -681,8 +693,11 @@ pub async fn auto_join_qualifying_rooms(
 }
 
 /// Handle a state event change that may require enforcement.
-/// Spawns enforcement as a background task to avoid recursive Send issues
+///
-/// in the append_pdu path.
+/// Spawns a background task (gated by the enforcement semaphore) to
+/// repopulate the cache and trigger enforcement actions based on the
+/// event type. Deduplicated per-space to avoid redundant work during
+/// bulk operations.
 impl Service {
 	pub fn handle_state_event_change(
 		self: &Arc<Self>,
@@ -703,8 +718,8 @@ impl Service {
 				if pending.contains(&space_id) {
 					return;
 				}
-				pending.insert(space_id.clone());
+				pending.insert(space_id.clone())
-			}
+			};
 
 			let _permit = this.enforcement_semaphore.acquire().await;
 
@@ -732,7 +747,7 @@ impl Service {
 						.await;
 					for member in &space_members {
 						if let Err(e) =
-							this.kick_unqualified_from_rooms(&space_id, member).await
+							Box::pin(this.kick_unqualified_from_rooms(&space_id, member)).await
 						{
 							debug_warn!(
 								"Role definition revalidation kick failed for {member}: {e}"
@@ -744,14 +759,14 @@ impl Service {
 					// User's roles changed — auto-join/kick + PL sync
 					if let Ok(user_id) = UserId::parse(state_key.as_str()) {
 						if let Err(e) =
-							this.auto_join_qualifying_rooms(&space_id, &user_id).await
+							this.auto_join_qualifying_rooms(&space_id, user_id).await
 						{
 							debug_warn!(
 								"Space role auto-join failed for {user_id}: {e}"
 							);
 						}
 						if let Err(e) =
-							this.kick_unqualified_from_rooms(&space_id, &user_id).await
+							Box::pin(this.kick_unqualified_from_rooms(&space_id, user_id)).await
 						{
 							debug_warn!(
 								"Space role auto-kick failed for {user_id}: {e}"
@@ -776,7 +791,7 @@ impl Service {
 						let members: Vec<OwnedUserId> = this
 							.services
 							.state_cache
-							.room_members(&target_room)
+							.room_members(target_room)
 							.map(ToOwned::to_owned)
 							.collect()
 							.await;
@@ -784,13 +799,13 @@ impl Service {
 							if !this
 								.user_qualifies_for_room(
 									&space_id,
-									&target_room,
+									target_room,
 									member,
 								)
 								.await
 							{
-								if let Err(e) = this
+								if let Err(e) = Box::pin(this
-									.kick_unqualified_from_rooms(&space_id, member)
+									.kick_unqualified_from_rooms(&space_id, member))
 									.await
 								{
 									debug_warn!(
@@ -809,8 +824,12 @@ impl Service {
 		});
 	}
 
-	/// Handle a new m.space.child event — update index and auto-join qualifying
+	/// Handle a new `m.space.child` event — update index and auto-join
-	/// members, or remove child from index if `via` is empty.
+	/// qualifying members.
+	///
+	/// If the child event's `via` field is empty the child is removed from
+	/// both the forward and reverse indexes. Otherwise the child is added
+	/// and all qualifying space members are auto-joined.
 	pub fn handle_space_child_change(
 		self: &Arc<Self>,
 		space_id: OwnedRoomId,
@@ -961,7 +980,12 @@ impl Service {
 		});
 	}
 
-	/// Handle a user joining a Space — auto-join them to qualifying child rooms.
+	/// Handle a user joining a Space — auto-join them to qualifying child
+	/// rooms.
+	///
+	/// Spawns a background task that auto-joins the user into every child
+	/// room they qualify for, then synchronizes their power levels across
+	/// all child rooms.
 	pub fn handle_space_member_join(
 		self: &Arc<Self>,
 		space_id: OwnedRoomId,
@@ -987,7 +1011,7 @@ impl Service {
 			let child_rooms = this.get_child_rooms(&space_id).await;
 			for child_room_id in &child_rooms {
 				if let Err(e) =
-					this.sync_power_levels(&space_id, &child_room_id).await
+					this.sync_power_levels(&space_id, child_room_id).await
 				{
 					debug_warn!(
 						"Failed to sync PLs on join for {child_room_id}: {e}"
@@ -1059,19 +1083,16 @@ pub async fn kick_unqualified_from_rooms(
 		}
 
 		// Get existing member event content for the kick
-		let member_content = match self
+		let Ok(member_content) = self
 			.services
 			.state_accessor
 			.get_member(child_room_id, user_id)
 			.await
-		{
+		else {
-			| Ok(event) => event,
+			debug_warn!(
-			| Err(_) => {
+				"Could not get member event for {user_id} in {child_room_id}, skipping kick"
-				debug_warn!(
+			);
-					"Could not get member event for {user_id} in {child_room_id}, skipping kick"
+			continue;
-				);
-				continue;
-			},
 		};
 
 		let state_lock = self.services.state.mutex.lock(child_room_id).await;

src/service/rooms/roles/tests.rs

@@ -3,6 +3,8 @@ use std::collections::{BTreeMap, HashMap, HashSet};
 use conduwuit_core::matrix::space_roles::RoleDefinition;
 use ruma::{room_id, OwnedRoomId};
 
+use super::{compute_user_power_level, roles_satisfy_requirements};
+
 /// Helper to build a role definitions map.
 pub fn make_roles(entries: &[(&str, Option<i64>)]) -> BTreeMap<String, RoleDefinition> {
 	entries
@@ -31,11 +33,7 @@ pub fn make_requirements(roles: &[&str]) -> HashSet<String> {
 fn power_level_single_role() {
 	let roles = make_roles(&[("admin", Some(100)), ("mod", Some(50))]);
 	let user_assigned = make_user_roles(&["admin"]);
-	let max_pl = user_assigned
+	assert_eq!(compute_user_power_level(&roles, &user_assigned), Some(100));
-		.iter()
-		.filter_map(|r| roles.get(r)?.power_level)
-		.max();
-	assert_eq!(max_pl, Some(100));
 }
 
 #[test]
@@ -46,90 +44,70 @@ fn power_level_multiple_roles_takes_highest() {
 		("helper", Some(25)),
 	]);
 	let user_assigned = make_user_roles(&["mod", "helper"]);
-	let max_pl = user_assigned
+	assert_eq!(compute_user_power_level(&roles, &user_assigned), Some(50));
-		.iter()
-		.filter_map(|r| roles.get(r)?.power_level)
-		.max();
-	assert_eq!(max_pl, Some(50));
 }
 
 #[test]
 fn power_level_no_power_roles() {
 	let roles = make_roles(&[("nsfw", None), ("vip", None)]);
 	let user_assigned = make_user_roles(&["nsfw", "vip"]);
-	let max_pl = user_assigned
+	assert_eq!(compute_user_power_level(&roles, &user_assigned), None);
-		.iter()
-		.filter_map(|r| roles.get(r)?.power_level)
-		.max();
-	assert_eq!(max_pl, None);
 }
 
 #[test]
 fn power_level_mixed_roles() {
 	let roles = make_roles(&[("mod", Some(50)), ("nsfw", None)]);
 	let user_assigned = make_user_roles(&["mod", "nsfw"]);
-	let max_pl = user_assigned
+	assert_eq!(compute_user_power_level(&roles, &user_assigned), Some(50));
-		.iter()
-		.filter_map(|r| roles.get(r)?.power_level)
-		.max();
-	assert_eq!(max_pl, Some(50));
 }
 
 #[test]
 fn power_level_no_roles_assigned() {
 	let roles = make_roles(&[("admin", Some(100))]);
 	let user_assigned: HashSet<String> = HashSet::new();
-	let max_pl = user_assigned
+	assert_eq!(compute_user_power_level(&roles, &user_assigned), None);
-		.iter()
-		.filter_map(|r| roles.get(r)?.power_level)
-		.max();
-	assert_eq!(max_pl, None);
 }
 
 #[test]
 fn power_level_unknown_role_ignored() {
 	let roles = make_roles(&[("admin", Some(100))]);
 	let user_assigned = make_user_roles(&["nonexistent"]);
-	let max_pl = user_assigned
+	assert_eq!(compute_user_power_level(&roles, &user_assigned), None);
-		.iter()
-		.filter_map(|r| roles.get(r)?.power_level)
-		.max();
-	assert_eq!(max_pl, None);
 }
 
 #[test]
 fn qualifies_with_all_required_roles() {
 	let required = make_requirements(&["nsfw", "vip"]);
 	let user_assigned = make_user_roles(&["nsfw", "vip", "extra"]);
-	assert!(required.iter().all(|r| user_assigned.contains(r)));
+	assert!(roles_satisfy_requirements(&required, &user_assigned));
 }
 
 #[test]
 fn does_not_qualify_missing_one_role() {
 	let required = make_requirements(&["nsfw", "vip"]);
 	let user_assigned = make_user_roles(&["nsfw"]);
-	assert!(!required.iter().all(|r| user_assigned.contains(r)));
+	assert!(!roles_satisfy_requirements(&required, &user_assigned));
 }
 
 #[test]
 fn qualifies_with_no_requirements() {
 	let required: HashSet<String> = HashSet::new();
 	let user_assigned = make_user_roles(&["nsfw"]);
-	assert!(required.iter().all(|r| user_assigned.contains(r)));
+	assert!(roles_satisfy_requirements(&required, &user_assigned));
 }
 
 #[test]
 fn does_not_qualify_with_no_roles() {
 	let required = make_requirements(&["nsfw"]);
 	let user_assigned: HashSet<String> = HashSet::new();
-	assert!(!required.iter().all(|r| user_assigned.contains(r)));
+	assert!(!roles_satisfy_requirements(&required, &user_assigned));
 }
 
 #[test]
 fn qualifies_empty_requirements_empty_roles() {
 	let required: HashSet<String> = HashSet::new();
 	let user_assigned: HashSet<String> = HashSet::new();
-	assert!(required.iter().all(|r| user_assigned.contains(r)));
+	assert!(roles_satisfy_requirements(&required, &user_assigned));
 }
 
 #[test]

src/service/rooms/timeline/append.rs

@@ -379,7 +379,7 @@ where
 						roles.handle_state_event_change(
 							room_id.to_owned(),
 							event_type_str,
-							state_key.to_string(),
+							state_key.to_owned(),
 						);
 					}
 				},

src/service/rooms/timeline/build.rs

@@ -104,6 +104,9 @@ pub async fn build_and_append_pdu(
 	}
 	// Space permission cascading: reject power level changes that conflict
 	// with Space-granted levels (exempt the server user so sync_power_levels works)
+	type SpaceEnforcementData =
+		(ruma::OwnedRoomId, Vec<(OwnedUserId, HashSet<String>)>, BTreeMap<String, RoleDefinition>);
+
 	if self.services.roles.is_enabled()
 		&& *pdu.kind() == TimelineEventType::RoomPowerLevels
 		&& pdu.sender() != <OwnedUserId as AsRef<UserId>>::as_ref(&self.services.globals.server_user)
@@ -129,9 +132,6 @@ pub async fn build_and_append_pdu(
 
 			// Also check that space-managed users aren't omitted
 			// Clone data out of guards to avoid holding locks across await
-			type SpaceEnforcementData =
-				(ruma::OwnedRoomId, Vec<(OwnedUserId, HashSet<String>)>, BTreeMap<String, RoleDefinition>);
-
 			let space_data: Vec<SpaceEnforcementData> = {
 				let user_roles_guard = self.services.roles.user_roles.read().await;
 				let roles_guard = self.services.roles.roles.read().await;