From 30416218de6019807c0514f63b83d1b145ba3cd8 Mon Sep 17 00:00:00 2001 From: Muse Sweep Date: Sun, 25 Oct 2026 03:14:31 -0500 Subject: [PATCH] fix(id-world): manifest plan leaves omitted grants/ceiling/schedules/profile alone (#817) --- pkgs/id/src/world_manifest.rs | 212 +++++++++++++++++++++++++--------- 1 file changed, 159 insertions(+), 53 deletions(-) diff --git a/pkgs/id/src/world_manifest.rs b/pkgs/id/src/world_manifest.rs index 033c8c553..5cf26a288 100644 --- a/pkgs/id/src/world_manifest.rs +++ b/pkgs/id/src/world_manifest.rs @@ -20,8 +20,9 @@ use serde::{Deserialize, Serialize}; use crate::world_caps::{MAX_NAME_BYTES, validate_ceiling_names, validate_grant_names}; use crate::world_limits::WorldExecutionProfile; -/// A declarative description of a world. Empty fields are left alone by -/// [`plan`], so a minimal manifest drifts nothing. +/// A declarative description of a world. Fields the manifest omits are left +/// alone by [`plan`], so a minimal manifest drifts nothing; a field that is +/// present — even empty, like `grants = []` — is authoritative. #[derive(Clone, Debug, Default, PartialEq, Eq, Serialize, Deserialize)] #[serde(default, rename_all = "snake_case")] pub struct WorldManifest { @@ -34,14 +35,23 @@ pub struct WorldManifest { /// The deterministic seed for the program's initializer. pub seed: Option, /// Capabilities to grant. `*` grants every capability the ceiling allows. - pub grants: Vec, - /// The capability ceiling. `None` means the whole catalog; an empty list - /// means no capability is allowed. + /// `None` (the field is omitted) leaves the live grants alone; `Some([])` + /// revokes every grant. + pub grants: Option>, + /// The capability ceiling: the names in the list are the only capabilities + /// allowed, so an empty list allows none. `None` (the field is omitted) + /// leaves the live ceiling alone — a manifest that never mentions the + /// ceiling can never widen it. (In a [`WorldState`] and in + /// [`ApplyStep::SetCeiling`], `None` instead means the whole catalog.) pub ceiling: Option>, - /// Requested execution profile bounds. Omitted fields inherit the server. + /// Requested execution profile bounds. Omitted fields inherit the server, + /// so the all-default profile leaves the live profile alone. pub profile: WorldExecutionProfile, - /// Pending schedules: id to fire time, in unix milliseconds. - pub schedules: BTreeMap, + /// Pending schedules: id to fire time, in unix milliseconds. `None` (the + /// field is omitted) leaves the live schedules alone; a present map is + /// reconciled exactly — missing ids are scheduled, undeclared ones + /// cancelled. + pub schedules: Option>, } /// The relevant current state of a live world, as `id world stats` would see @@ -152,13 +162,17 @@ impl WorldManifest { /// /// Names the first offending field. pub fn validate(&self) -> Result<()> { - validate_grant_names(&self.grants)?; + if let Some(grants) = &self.grants { + validate_grant_names(grants)?; + } if let Some(ceiling) = &self.ceiling { validate_ceiling_names(ceiling)?; } - for (id, at_ms) in &self.schedules { - crate::world_caps::validate_schedule_id(id)?; - ensure!(*at_ms > 0, "schedule {id:?} must fire in the future"); + if let Some(schedules) = &self.schedules { + for (id, at_ms) in schedules { + crate::world_caps::validate_schedule_id(id)?; + ensure!(*at_ms > 0, "schedule {id:?} must fire in the future"); + } } ensure!( self.module_hash.is_none() || self.module_source.is_none(), @@ -204,43 +218,55 @@ pub fn plan(desired: &WorldManifest, current: &WorldState) -> Vec { }); } - // Grants: replace when the set differs. - let desired_grants: std::collections::BTreeSet = - desired.grants.iter().cloned().collect(); - let current_grants: std::collections::BTreeSet = - current.grants.iter().cloned().collect(); - if desired_grants != current_grants { - steps.push(ApplyStep::SetGrants { - grants: desired.grants.clone(), - }); + // Grants: an omitted field leaves the live set alone; a present one + // replaces it when the set differs (an empty list revokes everything). + if let Some(desired_grants) = &desired.grants { + let desired_set: std::collections::BTreeSet = + desired_grants.iter().cloned().collect(); + let current_set: std::collections::BTreeSet = + current.grants.iter().cloned().collect(); + if desired_set != current_set { + steps.push(ApplyStep::SetGrants { + grants: desired_grants.clone(), + }); + } } - // Ceiling: replace when it differs. - if desired.ceiling != current.ceiling { - steps.push(ApplyStep::SetCeiling { - ceiling: desired.ceiling.clone(), - }); + // Ceiling: an omitted field leaves the live ceiling alone, so a manifest + // that never mentions the ceiling can never widen it; a present list + // replaces it when it differs. + if let Some(ceiling) = &desired.ceiling { + if current.ceiling.as_deref() != Some(ceiling.as_slice()) { + steps.push(ApplyStep::SetCeiling { + ceiling: Some(ceiling.clone()), + }); + } } - // Profile: replace when it differs. - if desired.profile != current.profile { + // Profile: the all-default profile inherits the server on every field, + // so it is how a manifest says nothing about the profile — leave the + // live profile alone. A present profile replaces when it differs. + if desired.profile != WorldExecutionProfile::default() && desired.profile != current.profile { steps.push(ApplyStep::SetProfile { profile: desired.profile, }); } - // Schedules: add missing, cancel extra. - for (id, at_ms) in &desired.schedules { - if current.schedules.get(id) != Some(at_ms) { - steps.push(ApplyStep::ScheduleAt { - id: id.clone(), - at_ms: *at_ms, - }); + // Schedules: an omitted field leaves the live schedules alone; a present + // map is reconciled exactly — add missing, cancel extra. + if let Some(desired_schedules) = &desired.schedules { + for (id, at_ms) in desired_schedules { + if current.schedules.get(id) != Some(at_ms) { + steps.push(ApplyStep::ScheduleAt { + id: id.clone(), + at_ms: *at_ms, + }); + } } - } - for id in current.schedules.keys() { - if !desired.schedules.contains_key(id) { - steps.push(ApplyStep::CancelSchedule { id: id.clone() }); + for id in current.schedules.keys() { + if !desired_schedules.contains_key(id) { + steps.push(ApplyStep::CancelSchedule { id: id.clone() }); + } } } @@ -311,6 +337,13 @@ pub fn diff(desired: &WorldManifest, current: &WorldState) -> String { if steps.is_empty() { return "no drift".to_owned(); } + render_steps(&steps) +} + +/// Render plan steps as drift lines. A `SetCeiling` to `None` (the whole +/// catalog) and to `Some([])` (no capability allowed) are opposite operations +/// and render distinctly: `[*] (whole catalog)` vs `[]`. +fn render_steps(steps: &[ApplyStep]) -> String { let mut out = String::new(); for step in steps { match step { @@ -328,10 +361,12 @@ pub fn diff(desired: &WorldManifest, current: &WorldState) -> String { ApplyStep::SetGrants { grants } => { out.push_str(&format!("~ grants [{}]\n", grants.join(", "))) } - ApplyStep::SetCeiling { ceiling } => { - let names = ceiling.as_deref().unwrap_or(&[]).join(", "); - out.push_str(&format!("~ ceiling [{names}]\n")); - } + ApplyStep::SetCeiling { ceiling } => match ceiling { + Some(names) => { + out.push_str(&format!("~ ceiling [{}]\n", names.join(", "))); + } + None => out.push_str("~ ceiling [*] (whole catalog)\n"), + }, ApplyStep::SetProfile { profile } => { out.push_str(&format!("~ profile {profile:?}\n")); } @@ -378,7 +413,10 @@ wasm_fuel = 5_000_000 let manifest = parse(manifest_toml()).unwrap(); assert_eq!(manifest.module_hash, Some("abc123".to_owned())); assert_eq!(manifest.seed, Some(42)); - assert_eq!(manifest.grants, vec!["chat.say", "time.now"]); + assert_eq!( + manifest.grants, + Some(vec!["chat.say".to_owned(), "time.now".to_owned()]) + ); assert_eq!( manifest.ceiling, Some(vec![ @@ -388,7 +426,10 @@ wasm_fuel = 5_000_000 ]) ); assert_eq!(manifest.profile.wasm_fuel, Some(5_000_000)); - assert_eq!(manifest.schedules.get("round"), Some(&1_800_000_000_000)); + assert_eq!( + manifest.schedules, + Some(BTreeMap::from([("round".to_owned(), 1_800_000_000_000)])) + ); assert_eq!(round_trip(&manifest).unwrap(), manifest); } @@ -397,21 +438,83 @@ wasm_fuel = 5_000_000 let manifest = WorldManifest::default(); // A default (empty) manifest leaves a default (empty) world alone. assert!(plan(&manifest, &WorldState::default()).is_empty()); - // And a partially-populated world only drifts where it differs from the - // empty manifest: the grants and ceiling are removed. + // And it leaves a fully populated world alone too (#817): every field + // is unspecified, so nothing is revoked, widened, or cancelled. + let state = WorldState { + module_hash: Some("abc".to_owned()), + seed: 7, + grants: vec!["chat.say".to_owned()], + ceiling: Some(vec!["chat.say".to_owned()]), + schedules: BTreeMap::from([("round".to_owned(), 5)]), + ..WorldState::default() + }; + assert!(plan(&manifest, &state).is_empty()); + } + + #[test] + fn a_minimal_manifest_only_installs_the_module() { + // #817 regression: a manifest that only names a module must not drift + // grants, ceiling, profile, or schedules on a populated world. + let manifest = parse("module_hash = \"def456\"").unwrap(); let state = WorldState { module_hash: Some("abc".to_owned()), seed: 7, grants: vec!["chat.say".to_owned()], ceiling: Some(vec!["chat.say".to_owned()]), + schedules: BTreeMap::from([("round".to_owned(), 5)]), ..WorldState::default() }; let steps = plan(&manifest, &state); - assert_eq!(steps.len(), 2, "{steps:?}"); - assert!(steps.iter().all(|s| matches!( - s, - ApplyStep::SetGrants { .. } | ApplyStep::SetCeiling { .. } - ))); + assert_eq!(steps.len(), 1, "{steps:?}"); + assert!(matches!(steps[0], ApplyStep::InstallModule { .. })); + } + + #[test] + fn an_explicitly_empty_field_is_authoritative() { + // #817: `grants = []` / `ceiling = []` / `schedules = {}` in the TOML + // are present-but-empty, which clears — unlike omitting them. + let manifest = parse("grants = []\nceiling = []\nschedules = {}").unwrap(); + assert_eq!(manifest.grants, Some(Vec::new())); + assert_eq!(manifest.ceiling, Some(Vec::new())); + assert_eq!(manifest.schedules, Some(BTreeMap::new())); + let state = WorldState { + grants: vec!["chat.say".to_owned()], + ceiling: Some(vec!["chat.say".to_owned()]), + schedules: BTreeMap::from([("round".to_owned(), 5)]), + ..WorldState::default() + }; + let steps = plan(&manifest, &state); + assert!( + steps + .iter() + .any(|s| matches!(s, ApplyStep::SetGrants { grants } if grants.is_empty())), + "{steps:?}" + ); + assert!( + steps.iter().any( + |s| matches!(s, ApplyStep::SetCeiling { ceiling } if ceiling.as_deref() == Some(&[][..])) + ), + "{steps:?}" + ); + assert!( + steps + .iter() + .any(|s| matches!(s, ApplyStep::CancelSchedule { id } if id == "round")), + "{steps:?}" + ); + } + + #[test] + fn ceiling_none_and_empty_render_distinctly() { + // #817: widening to the whole catalog and narrowing to nothing are + // opposite operations and must not render the same in `world diff`. + let widened = render_steps(&[ApplyStep::SetCeiling { ceiling: None }]); + let emptied = render_steps(&[ApplyStep::SetCeiling { + ceiling: Some(Vec::new()), + }]); + assert_ne!(widened, emptied); + assert!(widened.contains("whole catalog"), "{widened}"); + assert!(emptied.contains("ceiling []"), "{emptied}"); } #[test] @@ -445,7 +548,10 @@ wasm_fuel = 5_000_000 #[test] fn schedules_are_added_and_removed() { let manifest = WorldManifest { - schedules: BTreeMap::from([("keep".to_owned(), 5), ("new".to_owned(), 6)]), + schedules: Some(BTreeMap::from([ + ("keep".to_owned(), 5), + ("new".to_owned(), 6), + ])), ..WorldManifest::default() }; let state = WorldState {