From d93baac3ba289d64bdb90b94c0a7a03b7a90ae1d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Thu, 25 Jun 2026 21:45:51 -0400 Subject: [PATCH] Agent K M2d review follow-up: harden the Group-4 score-settings ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address the five-finding review of M2d (1e4ab24) plus the two-finding follow-up review; all fixes are code/test/doc only, no spec change needed (the catalog/core-spec already classify metadata as advisory LWW). - SetMetadata is now a true advisory LWW: it silently last-writer-wins in canonical order and records no conflict, matching operation_catalog §set-user-system-break. Dropped the StructuralFieldCollision, the `last_metadata` working slot, and the `env` parameter; rewrote the conflict test as `concurrent_differing_set_metadata_is_advisory_lww` (no conflict, state stays clean, permutation-independent resolution). - SetMetricGrid / SetUserPageBreak / SetUserSystemBreak share a new `layout_region_slot` precondition backed by a `staff_based_regions` index: the target must be live and staff-based (FreeGraphic regions have neither a metric-grid nor a break slot). The index is read from base-free state, so reduce() and reduce_onto() reach the same verdict for missing, tombstoned, and FreeGraphic regions. - SetMetricGrid now rejects a grid whose meter_sequence names an undeclared time signature, rather than installing an invariant-violating grid. - User breaks materialize under the canonical LWW key: `apply_break_lww` drops any existing anchor resolving to the same position before adding, so the graph break list stays in lockstep with the resolved-position ledger map (shared `resolved_anchor_position`). Applied to page and system breaks alike. - Coverage: SetMetadata/SetMetricGrid/CreateVoice/DeleteVoice added to the tag-distinctness test; layout_stub `gen_operation_kind_tag` extended to every normative tag; the MaterializedState decode test populates page_breaks; four direct regression tests pin each fixed bug; the stale SetMetadata/score_metadata doc comments now say advisory LWW. Gates: build/fmt/clippy -D warnings clean; cargo test --workspace green (533); conformance_suite scale 1 passes. Stages only core/ops/testkit; the unrelated Agent-I working tree is left untouched. Co-Authored-By: Claude Opus 4.8 --- crates/epiphany-core/src/graph.rs | 9 + crates/epiphany-ops/src/payload.rs | 49 +-- crates/epiphany-ops/src/reduce.rs | 227 +++++++------- crates/epiphany-ops/src/valuegen.rs | 5 +- crates/epiphany-ops/tests/graph_reduction.rs | 301 ++++++++++++++++++- crates/epiphany-testkit/src/generators.rs | 4 + crates/epiphany-testkit/src/layout_stub.rs | 9 +- 7 files changed, 449 insertions(+), 155 deletions(-) diff --git a/crates/epiphany-core/src/graph.rs b/crates/epiphany-core/src/graph.rs index 52d4e7f..a061ab8 100644 --- a/crates/epiphany-core/src/graph.rs +++ b/crates/epiphany-core/src/graph.rs @@ -583,6 +583,15 @@ impl RegionContent { } } + /// Mutable staff-based content, if this region is staff-based or hybrid. + pub fn staff_based_mut(&mut self) -> Option<&mut StaffBasedContent> { + match self { + RegionContent::StaffBased(c) => Some(c), + RegionContent::Hybrid { staves, .. } => Some(staves), + RegionContent::FreeGraphic(_) => None, + } + } + /// Mutable access to the staff instances, if this content has any (used by /// editing and by the invariant shrinker in [`crate::generators`]). pub fn staff_instances_mut(&mut self) -> Option<&mut Vec> { diff --git a/crates/epiphany-ops/src/payload.rs b/crates/epiphany-ops/src/payload.rs index bf27ca0..47cb464 100644 --- a/crates/epiphany-ops/src/payload.rs +++ b/crates/epiphany-ops/src/payload.rs @@ -626,19 +626,27 @@ pub struct SetUserSystemBreakOp { pub present: bool, } +/// The musical position a [`TimeAnchor`] resolves to for user-break LWW +/// bucketing. A region-relative musical offset resolves to that offset; any +/// other anchor shape resolves to the region origin (the break still applies to +/// the region, the prototype LWW key is coarse — see `DECISIONS.md`). The graph +/// reducer keys break anchors by this same position, so two anchors resolving +/// here to one position occupy a single LWW slot. +pub(crate) fn resolved_anchor_position(anchor: &TimeAnchor) -> MusicalPosition { + match anchor { + TimeAnchor::Region { + offset: epiphany_core::AnchorOffset::Musical(d), + .. + } => MusicalPosition(d.0.clone()), + _ => MusicalPosition::origin(), + } +} + impl SetUserSystemBreakOp { - /// The anchor's resolved musical position — the canonical LWW bucketing key. - /// A region-relative musical offset resolves to that offset; any other anchor - /// shape resolves to the region origin (the break still applies to the - /// region, the prototype LWW key is coarse — see `DECISIONS.md`). + /// The anchor's resolved musical position — the canonical LWW bucketing key + /// (see [`resolved_anchor_position`]). pub fn resolved_position(&self) -> MusicalPosition { - match &self.anchor { - TimeAnchor::Region { - offset: epiphany_core::AnchorOffset::Musical(d), - .. - } => MusicalPosition(d.0.clone()), - _ => MusicalPosition::origin(), - } + resolved_anchor_position(&self.anchor) } } @@ -976,8 +984,9 @@ impl CanonicalEncode for DeleteVoiceOp { // --- Group 4 (M2d): score settings (Chapter 6 §6.10). LWW field-overwrite. --- /// Overwrite the score metadata (Chapter 6 §6.10 SetMetadata). Carries the full -/// [`ScoreMetadata`] (v1); the score-singleton field-overwrite is last-writer-wins -/// (concurrent differing ⇒ structural-field-collision). +/// [`ScoreMetadata`] (v1); the score-singleton field-overwrite is *advisory* +/// last-writer-wins — the latest write in canonical order silently wins and no +/// conflict is recorded (operation_catalog §set-user-system-break "LWW advisory"). #[derive(Clone, PartialEq, Eq, Debug)] pub struct SetMetadataOp { pub metadata: ScoreMetadata, @@ -1025,15 +1034,9 @@ pub struct SetUserPageBreakOp { impl SetUserPageBreakOp { /// The anchor's resolved musical position — the canonical LWW bucketing key - /// (see [`SetUserSystemBreakOp::resolved_position`]). + /// (see [`resolved_anchor_position`]). pub fn resolved_position(&self) -> MusicalPosition { - match &self.anchor { - TimeAnchor::Region { - offset: epiphany_core::AnchorOffset::Musical(d), - .. - } => MusicalPosition(d.0.clone()), - _ => MusicalPosition::origin(), - } + resolved_anchor_position(&self.anchor) } } @@ -1098,6 +1101,10 @@ mod tests { OperationKindTag::InsertIdentifiedPitch, OperationKindTag::DeleteIdentifiedPitch, OperationKindTag::ModifyIdentifiedPitch, + OperationKindTag::CreateVoice, + OperationKindTag::DeleteVoice, + OperationKindTag::SetMetadata, + OperationKindTag::SetMetricGrid, ]; let encoded: std::collections::BTreeSet<_> = tags .iter() diff --git a/crates/epiphany-ops/src/reduce.rs b/crates/epiphany-ops/src/reduce.rs index ade9560..39b3dd2 100644 --- a/crates/epiphany-ops/src/reduce.rs +++ b/crates/epiphany-ops/src/reduce.rs @@ -33,7 +33,7 @@ use std::collections::{BTreeMap, BTreeSet}; use epiphany_core::{ derive_promoted_voice_id, AnchorOffset, CanonicalValue, Event, EventDuration, EventId, EventPosition, MetricGrid, MusicalDuration, MusicalPosition, OperationId, Pitch, PitchId, - PitchSpelling, RegionEdge, RegionId, RegionTimeModel, Score, ScoreMetadata, SpellingAttachment, + PitchSpelling, RegionEdge, RegionId, RegionTimeModel, Score, SpellingAttachment, SpellingDirective, SpellingScope, SpellingSource, StaffInstance, StaffInstanceId, TimeAnchor, TransactionId, TypedObjectId, Voice, VoiceId, VoiceOrigin, }; @@ -341,9 +341,10 @@ struct Reducer<'a> { // LWW working state for ModifyCrossCutting (Group 2), mirroring the leaf-field // modify maps above: last modifier + value it wrote, keyed by structure id. last_cross_cutting_modify: BTreeMap, - // LWW working state for the Group-4 (M2d) score-settings overwrites: the last - // writer and the value it wrote (the resolved value lives in the graph). - last_metadata: Option<(OperationId, ScoreMetadata)>, + // LWW working state for SetMetricGrid (Group 4, M2d): the last writer and the + // grid it wrote, used to detect concurrent *differing* grids (the resolved + // value lives in the graph). SetMetadata is advisory LWW — no working state, + // no conflict — so the latest write in canonical order silently wins. last_metric_grid: BTreeMap)>, structures: BTreeMap>, // Live child sets for the structural-container empty-only delete (Group 3): @@ -351,6 +352,12 @@ struct Reducer<'a> { // voice's live events are read from `voice_occupancy`.) region_instances: BTreeMap>, instance_voices: BTreeMap>, + // Regions whose content carries a staff-based slot (staff-based or hybrid), + // and so can hold a metric grid or user break. FreeGraphic regions cannot; + // tracking this in the base-free reducer lets SetMetricGrid / SetUserPageBreak + // / SetUserSystemBreak reach the same precondition verdict with or without a + // graph, so reduce() and reduce_onto() never disagree on those ops. + staff_based_regions: BTreeSet, migrated_regions: BTreeSet, region_migrator: BTreeMap, descriptors: BTreeMap, @@ -375,11 +382,11 @@ struct WorkingSnapshot { last_event_modify: BTreeMap, last_pitch_modify: BTreeMap, last_cross_cutting_modify: BTreeMap, - last_metadata: Option<(OperationId, ScoreMetadata)>, last_metric_grid: BTreeMap)>, structures: BTreeMap>, region_instances: BTreeMap>, instance_voices: BTreeMap>, + staff_based_regions: BTreeSet, migrated_regions: BTreeSet, region_migrator: BTreeMap, descriptors: BTreeMap, @@ -387,6 +394,19 @@ struct WorkingSnapshot { graph: Option, } +/// Apply a user break to a region's break list under the canonical LWW key — the +/// anchor's resolved musical position. Any existing anchor resolving to that same +/// position is dropped first, so two anchors at one position never both persist, +/// then the new anchor is pushed iff the break is present. The graph break list +/// then matches the resolved-position-keyed ledger map. +fn apply_break_lww(breaks: &mut Vec, anchor: &TimeAnchor, present: bool) { + let resolved = crate::payload::resolved_anchor_position(anchor); + breaks.retain(|existing| crate::payload::resolved_anchor_position(existing) != resolved); + if present { + breaks.push(anchor.clone()); + } +} + fn intervals_overlap( a_position: &MusicalPosition, a_duration: &MusicalDuration, @@ -453,11 +473,11 @@ impl<'a> Reducer<'a> { last_event_modify: BTreeMap::new(), last_pitch_modify: BTreeMap::new(), last_cross_cutting_modify: BTreeMap::new(), - last_metadata: None, last_metric_grid: BTreeMap::new(), structures: BTreeMap::new(), region_instances: BTreeMap::new(), instance_voices: BTreeMap::new(), + staff_based_regions: BTreeSet::new(), migrated_regions: BTreeSet::new(), region_migrator: BTreeMap::new(), descriptors: BTreeMap::new(), @@ -518,6 +538,9 @@ impl<'a> Reducer<'a> { for region in &score.canvas.regions { self.objects .insert(TypedObjectId::Region(region.id), ObjectState::Live); + if region.content.staff_based().is_some() { + self.staff_based_regions.insert(region.id); + } let instance_set = self.region_instances.entry(region.id).or_default(); for instance in region.staff_instances() { instance_set.insert(instance.id); @@ -1250,7 +1273,7 @@ impl<'a> Reducer<'a> { OperationKind::DeleteStaffInstance(op) => self.delete_staff_instance(env, op), OperationKind::CreateVoice(op) => self.create_voice(env, op), OperationKind::DeleteVoice(op) => self.delete_voice(env, op), - OperationKind::SetMetadata(op) => self.set_metadata(env, op), + OperationKind::SetMetadata(op) => self.set_metadata(op), OperationKind::SetMetricGrid(op) => self.set_metric_grid(env, op), OperationKind::SetUserPageBreak(op) => self.set_user_page_break(op), }, @@ -1265,43 +1288,14 @@ impl<'a> Reducer<'a> { &mut self, op: &crate::payload::SetUserSystemBreakOp, ) -> OperationEffect { + if let Some(effect) = self.layout_region_slot(op.region) { + return effect; + } if let Some(score) = self.graph.as_mut() { - let Some(region) = score - .canvas - .regions - .iter_mut() - .find(|region| region.id == op.region) - else { - return OperationEffect::NoOp { - reason: NoOpReason::PreconditionFailedUnderReduction { - reason: PreconditionFailureReason::TargetMissing, - }, - }; - }; - let breaks = match &mut region.content { - epiphany_core::RegionContent::StaffBased(content) => { - &mut content.user_system_breaks + if let Some(region) = score.canvas.regions.iter_mut().find(|r| r.id == op.region) { + if let Some(content) = region.content.staff_based_mut() { + apply_break_lww(&mut content.user_system_breaks, &op.anchor, op.present); } - epiphany_core::RegionContent::Hybrid { staves, .. } => { - &mut staves.user_system_breaks - } - epiphany_core::RegionContent::FreeGraphic(_) => { - return OperationEffect::NoOp { - reason: NoOpReason::PreconditionFailedUnderReduction { - reason: PreconditionFailureReason::TargetMissing, - }, - } - } - }; - // The value-typed payload (v1) carries the full TimeAnchor, so the - // graph break is the anchor itself rather than a reconstructed one. - let anchor = op.anchor.clone(); - if op.present { - if !breaks.contains(&anchor) { - breaks.push(anchor); - } - } else { - breaks.retain(|candidate| candidate != &anchor); } } @@ -1313,40 +1307,44 @@ impl<'a> Reducer<'a> { // --- Group 4 (M2d): score settings (LWW field-overwrite). -------------- // - // SetMetadata / SetMetricGrid mirror the modify ops: the resolved value lives - // in the graph (reduce_onto); MaterializedState records only the effect and, - // on a concurrent differing write, a StructuralFieldCollision. SetUserPageBreak - // mirrors SetUserSystemBreak: it is a canonical LWW advisory (page_breaks). + // SetMetadata is an *advisory* last-writer-wins field (operation_catalog + // §set-user-system-break "LWW advisory"): the latest write in canonical order + // silently wins, with no conflict — a clean concurrent metadata edit keeps the + // state clean. SetMetricGrid is a structural field-overwrite: the resolved grid + // lives in the graph and a concurrent differing grid records a + // StructuralFieldCollision. SetUserPageBreak mirrors SetUserSystemBreak: a + // canonical LWW advisory (page_breaks). + // + // SetMetricGrid / SetUserPageBreak target a region's staff-based slot, so they + // share `layout_region_slot`: the region must be live *and* staff-based (a + // FreeGraphic region has neither a metric grid nor a break list). That verdict + // reads only the base-free indices, so reduce() and reduce_onto() agree on it. - fn set_metadata(&mut self, env: &OperationEnvelope, op: &SetMetadataOp) -> OperationEffect { - let effect = match &self.last_metadata { - Some((prev_op, prev_meta)) if self.concurrent(env.id, *prev_op) => { - if *prev_meta == op.metadata { - return OperationEffect::NoOp { - reason: NoOpReason::AlreadyApplied, - }; - } - let prev_op = *prev_op; - let conflict = ConflictRecord::new( - ConflictKind::StructuralFieldCollision { - winner: env.id, - loser: prev_op, - field: FieldPath("metadata".to_string()), - }, - vec![env.id, prev_op], - vec![], - ); - let cid = conflict.id; - self.conflicts.insert(conflict); - OperationEffect::Conflicted { conflict: cid } - } - _ => OperationEffect::Applied, - }; - self.last_metadata = Some((env.id, op.metadata.clone())); + /// `Some(NoOp)` when `region` cannot carry a metric grid or user break — it is + /// missing, tombstoned, or FreeGraphic; `None` when it has a staff-based slot. + fn layout_region_slot(&self, region: RegionId) -> Option { + let live = matches!( + self.objects.get(&TypedObjectId::Region(region)), + Some(ObjectState::Live) + ); + if live && self.staff_based_regions.contains(®ion) { + None + } else { + Some(OperationEffect::NoOp { + reason: NoOpReason::PreconditionFailedUnderReduction { + reason: PreconditionFailureReason::TargetMissing, + }, + }) + } + } + + fn set_metadata(&mut self, op: &SetMetadataOp) -> OperationEffect { + // Advisory LWW: no conflict, no idempotence short-circuit. The resolved + // value is the last write in canonical order, held in the graph. if let Some(score) = self.graph.as_mut() { score.metadata = op.metadata.clone(); } - effect + OperationEffect::Applied } fn set_metric_grid( @@ -1354,15 +1352,28 @@ impl<'a> Reducer<'a> { env: &OperationEnvelope, op: &SetMetricGridOp, ) -> OperationEffect { - if !matches!( - self.objects.get(&TypedObjectId::Region(op.region)), - Some(ObjectState::Live) - ) { - return OperationEffect::NoOp { - reason: NoOpReason::PreconditionFailedUnderReduction { - reason: PreconditionFailureReason::TargetMissing, - }, - }; + if let Some(effect) = self.layout_region_slot(op.region) { + return effect; + } + // A non-empty grid names a time signature per meter change; the graph + // invariant (epiphany-core invariants.rs) rejects a grid that references an + // undeclared signature, so reject it here rather than install an + // invariant-violating grid. Time signatures are seeded from the base, so + // this verdict is identical with or without a graph. + if let Some(grid) = &op.grid { + for change in &grid.meter_sequence { + if !matches!( + self.objects + .get(&TypedObjectId::TimeSignature(change.time_signature)), + Some(ObjectState::Live) + ) { + return OperationEffect::NoOp { + reason: NoOpReason::PreconditionFailedUnderReduction { + reason: PreconditionFailureReason::TargetMissing, + }, + }; + } + } } let prev = self .last_metric_grid @@ -1401,53 +1412,23 @@ impl<'a> Reducer<'a> { return; }; if let Some(region) = score.canvas.regions.iter_mut().find(|r| r.id == region) { - match &mut region.content { - epiphany_core::RegionContent::StaffBased(content) => { - content.default_metric_grid = grid.clone(); - } - epiphany_core::RegionContent::Hybrid { staves, .. } => { - staves.default_metric_grid = grid.clone(); - } - epiphany_core::RegionContent::FreeGraphic(_) => {} + if let Some(content) = region.content.staff_based_mut() { + content.default_metric_grid = grid.clone(); } } } fn set_user_page_break(&mut self, op: &SetUserPageBreakOp) -> OperationEffect { + if let Some(effect) = self.layout_region_slot(op.region) { + return effect; + } if let Some(score) = self.graph.as_mut() { - let Some(region) = score - .canvas - .regions - .iter_mut() - .find(|region| region.id == op.region) - else { - return OperationEffect::NoOp { - reason: NoOpReason::PreconditionFailedUnderReduction { - reason: PreconditionFailureReason::TargetMissing, - }, - }; - }; - let breaks = match &mut region.content { - epiphany_core::RegionContent::StaffBased(content) => &mut content.user_page_breaks, - epiphany_core::RegionContent::Hybrid { staves, .. } => &mut staves.user_page_breaks, - epiphany_core::RegionContent::FreeGraphic(_) => { - return OperationEffect::NoOp { - reason: NoOpReason::PreconditionFailedUnderReduction { - reason: PreconditionFailureReason::TargetMissing, - }, - } + if let Some(region) = score.canvas.regions.iter_mut().find(|r| r.id == op.region) { + if let Some(content) = region.content.staff_based_mut() { + apply_break_lww(&mut content.user_page_breaks, &op.anchor, op.present); } - }; - let anchor = op.anchor.clone(); - if op.present { - if !breaks.contains(&anchor) { - breaks.push(anchor); - } - } else { - breaks.retain(|candidate| candidate != &anchor); } } - self.page_breaks .insert((op.region, op.resolved_position()), op.present); OperationEffect::Applied @@ -2075,6 +2056,9 @@ impl<'a> Reducer<'a> { self.graph_create_region(&op.region); self.mint_container(env, robj); self.region_instances.entry(op.region_id()).or_default(); + if op.region.content.staff_based().is_some() { + self.staff_based_regions.insert(op.region_id()); + } OperationEffect::Applied } @@ -2177,6 +2161,7 @@ impl<'a> Reducer<'a> { }, ); self.region_instances.remove(&op.region); + self.staff_based_regions.remove(&op.region); self.graph_delete_region(op.region); OperationEffect::Applied } @@ -3201,11 +3186,11 @@ impl<'a> Reducer<'a> { last_event_modify: self.last_event_modify.clone(), last_pitch_modify: self.last_pitch_modify.clone(), last_cross_cutting_modify: self.last_cross_cutting_modify.clone(), - last_metadata: self.last_metadata.clone(), last_metric_grid: self.last_metric_grid.clone(), structures: self.structures.clone(), region_instances: self.region_instances.clone(), instance_voices: self.instance_voices.clone(), + staff_based_regions: self.staff_based_regions.clone(), migrated_regions: self.migrated_regions.clone(), region_migrator: self.region_migrator.clone(), descriptors: self.descriptors.clone(), @@ -3227,11 +3212,11 @@ impl<'a> Reducer<'a> { self.last_event_modify = s.last_event_modify; self.last_pitch_modify = s.last_pitch_modify; self.last_cross_cutting_modify = s.last_cross_cutting_modify; - self.last_metadata = s.last_metadata; self.last_metric_grid = s.last_metric_grid; self.structures = s.structures; self.region_instances = s.region_instances; self.instance_voices = s.instance_voices; + self.staff_based_regions = s.staff_based_regions; self.migrated_regions = s.migrated_regions; self.region_migrator = s.region_migrator; self.descriptors = s.descriptors; diff --git a/crates/epiphany-ops/src/valuegen.rs b/crates/epiphany-ops/src/valuegen.rs index 969a6c6..5ca35cc 100644 --- a/crates/epiphany-ops/src/valuegen.rs +++ b/crates/epiphany-ops/src/valuegen.rs @@ -269,8 +269,9 @@ pub fn region(id: RegionId) -> Region { } /// Score metadata with a `nth`-distinct title (M2d) — distinct `nth` give -/// distinct [`ScoreMetadata`] values so a harness can make concurrent -/// `SetMetadata`s agree or conflict deterministically. +/// distinct [`ScoreMetadata`] values so a harness can drive concurrent +/// `SetMetadata`s, an advisory LWW field that resolves by canonical order with +/// no conflict. pub fn score_metadata(nth: u8) -> epiphany_core::ScoreMetadata { epiphany_core::ScoreMetadata { title: Some(format!("title-{nth}")), diff --git a/crates/epiphany-ops/tests/graph_reduction.rs b/crates/epiphany-ops/tests/graph_reduction.rs index 5509e23..0a3fe7f 100644 --- a/crates/epiphany-ops/tests/graph_reduction.rs +++ b/crates/epiphany-ops/tests/graph_reduction.rs @@ -1297,7 +1297,7 @@ fn score_settings_materialize_in_the_graph_and_ledger() { } #[test] -fn concurrent_differing_set_metadata_conflicts() { +fn concurrent_differing_set_metadata_is_advisory_lww() { let base = epiphany_core::generators::valid_score(100); // Two concurrent SetMetadata (neither sees the other) with differing values. let a = envelope( @@ -1321,16 +1321,297 @@ fn concurrent_differing_set_metadata_conflicts() { })), ); let mut set = OperationSet::new(); - set.accept_all(vec![a, b]); + set.accept_all(vec![a.clone(), b.clone()]); let result = set.reduce_onto(&base); - assert_eq!( - result.state.conflicts.records().len(), - 1, - "concurrent differing SetMetadata records exactly one conflict" + + // Metadata is an advisory last-writer-wins field: a clean concurrent edit + // raises no conflict and leaves the materialized state clean (matching the + // catalog/core-spec "LWW advisory" classification). + assert!( + result.state.conflicts.records().is_empty(), + "concurrent differing SetMetadata is advisory — it records no conflict" + ); + assert!( + result.state.is_clean(), + "an advisory metadata edit keeps the materialized state clean" + ); + assert!( + result + .state + .effects + .iter() + .all(|(_, effect)| matches!(effect, OperationEffect::Applied)), + "both writes apply; the last in canonical order silently wins" + ); + + // The resolved value is one of the two writes and is permutation-independent. + let resolved = result.score.metadata.clone(); + assert!( + resolved == valuegen::score_metadata(1) || resolved == valuegen::score_metadata(2), + "the resolved metadata is one of the concurrent writes" + ); + let mut reversed = OperationSet::new(); + reversed.accept_all(vec![b, a]); + assert_eq!( + reversed.reduce_onto(&base).score.metadata, + resolved, + "metadata resolution is independent of acceptance order" + ); + assert!(check_invariants(&result.score).is_empty()); +} + +/// Whether an effect is the precondition NoOp the layout ops use for a target +/// that is missing, tombstoned, or not staff-based. +fn is_target_missing(effect: &OperationEffect) -> bool { + matches!( + effect, + OperationEffect::NoOp { + reason: NoOpReason::PreconditionFailedUnderReduction { + reason: PreconditionFailureReason::TargetMissing, + }, + } + ) +} + +#[test] +fn set_user_page_break_on_a_missing_region_is_a_consistent_noop() { + let base = epiphany_core::generators::valid_score(100); + let ghost = epiphany_core::RegionId::new(ReplicaId(123), 7); // absent from the base + let op = envelope( + 58, + 0, + 10, + CausalContext::new(), + None, + OperationPayload::Primitive(OperationKind::SetUserPageBreak( + epiphany_ops::SetUserPageBreakOp { + region: ghost, + anchor: valuegen::region_start_anchor( + ghost, + MusicalPosition(RationalTime::from_int(0)), + ), + present: true, + }, + )), + ); + let mut set = OperationSet::new(); + set.accept(op.clone()); + + // Graph-aware: the absent region has no break slot, so the op is a NoOp and + // nothing enters the canonical page_breaks. + let graph = set.reduce_onto(&base); + assert!(is_target_missing(&effect_of(&graph, op.id))); + assert!( + graph.state.page_breaks.is_empty(), + "no canonical page break is recorded for a missing region" + ); + + // Base-free: with no region ever minted the reducer reaches the same verdict, + // so reduce() and reduce_onto() agree. + let bookkeeping = set.reduce(); + let effect = bookkeeping + .effects + .iter() + .find(|(e, _)| *e == op.id) + .map(|(_, eff)| eff) + .expect("the operation has an effect"); + assert!(is_target_missing(effect)); + assert!(bookkeeping.page_breaks.is_empty()); +} + +#[test] +fn layout_ops_on_a_free_graphic_region_are_rejected() { + let mut base = epiphany_core::generators::valid_score(100); + // A staff-less FreeGraphic region: it has neither a metric-grid nor a break + // slot, so both layout ops must reject it. The staff-based index is read with + // or without a graph, so reduce() and reduce_onto() reach the same verdict. + let fg_id = epiphany_core::RegionId::new(ReplicaId(99), 0); + let mut fg = valuegen::region(fg_id); + fg.content = epiphany_core::RegionContent::FreeGraphic(epiphany_core::GraphicContent { + objects: Vec::new(), + }); + base.canvas.regions.push(fg); + + let page = envelope( + 59, + 0, + 10, + CausalContext::new(), + None, + OperationPayload::Primitive(OperationKind::SetUserPageBreak( + epiphany_ops::SetUserPageBreakOp { + region: fg_id, + anchor: valuegen::region_start_anchor( + fg_id, + MusicalPosition(RationalTime::from_int(0)), + ), + present: true, + }, + )), + ); + let grid = envelope( + 59, + 1, + 11, + CausalContext::new().with_seen(ReplicaId(59), 0), + None, + OperationPayload::Primitive(OperationKind::SetMetricGrid( + epiphany_ops::SetMetricGridOp { + region: fg_id, + grid: Some(valuegen::metric_grid()), + }, + )), + ); + let mut set = OperationSet::new(); + set.accept_all(vec![page.clone(), grid.clone()]); + let result = set.reduce_onto(&base); + + assert!( + is_target_missing(&effect_of(&result, page.id)), + "a page break on a FreeGraphic region is rejected" + ); + assert!( + is_target_missing(&effect_of(&result, grid.id)), + "a metric grid on a FreeGraphic region is rejected" + ); + assert!( + result.state.page_breaks.is_empty(), + "nothing is recorded for the FreeGraphic region" + ); +} + +#[test] +fn set_metric_grid_rejects_an_undeclared_time_signature_reference() { + let base = epiphany_core::generators::valid_score(100); + let region = base.canvas.regions[0].id; + let grid_before = base.canvas.regions[0] + .content + .staff_based() + .expect("fixture is staff based") + .default_metric_grid + .clone(); + + // A grid whose single meter change names a time signature the score never + // declares — the graph invariant (epiphany-core) forbids installing it. + let bogus = epiphany_core::TimeSignatureId::new(ReplicaId(200), 1); + let bad_grid = epiphany_core::MetricGrid { + meter_sequence: vec![epiphany_core::MeterChange { + anchor: valuegen::region_start_anchor( + region, + MusicalPosition(RationalTime::from_int(0)), + ), + time_signature: bogus, + }], + }; + let op = envelope( + 60, + 0, + 10, + CausalContext::new(), + None, + OperationPayload::Primitive(OperationKind::SetMetricGrid( + epiphany_ops::SetMetricGridOp { + region, + grid: Some(bad_grid), + }, + )), + ); + let mut set = OperationSet::new(); + set.accept(op.clone()); + let result = set.reduce_onto(&base); + + assert!( + is_target_missing(&effect_of(&result, op.id)), + "a grid referencing an undeclared time signature is rejected" + ); + let grid_after = result + .score + .canvas + .regions + .iter() + .find(|r| r.id == region) + .expect("the region survives") + .content + .staff_based() + .expect("still staff based") + .default_metric_grid + .clone(); + assert_eq!( + grid_before, grid_after, + "the rejected grid leaves the region's metric grid unchanged" + ); + assert!(check_invariants(&result.score).is_empty()); +} + +#[test] +fn user_breaks_at_one_resolved_position_collapse_to_a_single_anchor() { + let base = epiphany_core::generators::valid_score(100); + let region = base.canvas.regions[0].id; + let offset = RationalTime::from_int(4); + // Two structurally distinct anchors (region start vs. end) that resolve to the + // *same* musical position — the canonical LWW key — both set present. + let start_anchor = TimeAnchor::Region { + id: region, + edge: RegionEdge::Start, + offset: AnchorOffset::Musical(MusicalDuration(offset.clone())), + }; + let end_anchor = TimeAnchor::Region { + id: region, + edge: RegionEdge::End, + offset: AnchorOffset::Musical(MusicalDuration(offset.clone())), + }; + let first = envelope( + 61, + 0, + 10, + CausalContext::new(), + None, + OperationPayload::Primitive(OperationKind::SetUserPageBreak( + epiphany_ops::SetUserPageBreakOp { + region, + anchor: start_anchor, + present: true, + }, + )), + ); + let second = envelope( + 61, + 1, + 11, + CausalContext::new().with_seen(ReplicaId(61), 0), + None, + OperationPayload::Primitive(OperationKind::SetUserPageBreak( + epiphany_ops::SetUserPageBreakOp { + region, + anchor: end_anchor.clone(), + present: true, + }, + )), + ); + let mut set = OperationSet::new(); + set.accept_all(vec![first, second]); + let result = set.reduce_onto(&base); + + let breaks = &result + .score + .canvas + .regions + .iter() + .find(|r| r.id == region) + .expect("the region survives") + .content + .staff_based() + .expect("fixture is staff based") + .user_page_breaks; + assert_eq!( + breaks.as_slice(), + &[end_anchor], + "two anchors at one resolved position collapse to the single last writer" + ); + assert_eq!( + result.state.page_breaks.len(), + 1, + "exactly one canonical LWW slot for the resolved position" ); - assert!(matches!( - result.state.conflicts.records()[0].kind, - ConflictKind::StructuralFieldCollision { .. } - )); assert!(check_invariants(&result.score).is_empty()); } diff --git a/crates/epiphany-testkit/src/generators.rs b/crates/epiphany-testkit/src/generators.rs index 6e42a87..235cf96 100644 --- a/crates/epiphany-testkit/src/generators.rs +++ b/crates/epiphany-testkit/src/generators.rs @@ -1633,6 +1633,10 @@ mod tests { (region_id(&mut rng), musical_position(&mut rng)), rng.boolean(), ); + state.page_breaks.insert( + (region_id(&mut rng), musical_position(&mut rng)), + rng.boolean(), + ); state .pending .push((operation_id(&mut rng), pending_reason(&mut rng))); diff --git a/crates/epiphany-testkit/src/layout_stub.rs b/crates/epiphany-testkit/src/layout_stub.rs index c39f230..2b893bd 100644 --- a/crates/epiphany-testkit/src/layout_stub.rs +++ b/crates/epiphany-testkit/src/layout_stub.rs @@ -715,7 +715,7 @@ pub fn gen_vertical_band(rng: &mut Rng) -> VerticalBand { /// An operation-kind tag (every variant Agent C's type provides, including the /// registered form). pub fn gen_operation_kind_tag(rng: &mut Rng) -> OperationKindTag { - match rng.below(17) { + match rng.below(24) { 0 => OperationKindTag::InsertEvent, 1 => OperationKindTag::DeleteEvent, 2 => OperationKindTag::ModifyEvent, @@ -732,6 +732,13 @@ pub fn gen_operation_kind_tag(rng: &mut Rng) -> OperationKindTag { 13 => OperationKindTag::SetUserSystemBreak, 14 => OperationKindTag::SetUserPageBreak, 15 => OperationKindTag::DeclareTransaction, + 16 => OperationKindTag::InsertIdentifiedPitch, + 17 => OperationKindTag::DeleteIdentifiedPitch, + 18 => OperationKindTag::ModifyIdentifiedPitch, + 19 => OperationKindTag::CreateVoice, + 20 => OperationKindTag::DeleteVoice, + 21 => OperationKindTag::SetMetadata, + 22 => OperationKindTag::SetMetricGrid, _ => OperationKindTag::Registered(epiphany_ops::OperationKindRegistryId( rng.next_u64() as u128 )),