From c47f4b5cec4d5dd89db5668c9b5cc87c90a44a75 Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Wed, 24 Jun 2026 18:25:48 -0400 Subject: [PATCH] Agent K M2a review follow-up: graph-materialization fixes for the leaf-field ops MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit From the M2a review (no bug in the bookkeeping reduction; the gap was that the Group-1 ops' *graph* materialization — reduce_onto — was unexercised by the gates, which hid two invalid-graph edges). Fixes are graph-materialization only; the bookkeeping projection, and therefore convergence/determinism, is unchanged. - DeleteIdentifiedPitch of a single-pitch note's last pitch left an empty (Chapter-5-invalid) PitchedEvent via EventArena::get_mut (which bypasses insert's well-formedness guard). It now degrades the note to a Rest of the same id/voice/position/duration; InsertIdentifiedPitch into a rest is the dual (rest -> one-pitch note), keeping the graph consistent with the bookkeeping that mints/tombstones the pitch object either way. - ModifyEvent now skips placement-changing (move) and malformed-empty pitched replacements in the graph rather than corrupting invariant 3 (VoiceEventsSortedNonOverlap) via get_mut; voice re-sort stays deferred and the LWW bookkeeping still records the modify. Coverage: graph_edit_session (criterion 1, reduce_onto + check_invariants) now emits all five Group-1 kinds, so the real-Score gate exercises their graph mutations at scale; plus two targeted reduce_onto regression tests (note->rest, rest->note) in tests/graph_reduction.rs. Docs: DECISIONS.md records the note<->rest equivalence and the ModifyEvent placement deferral (catalog section prose routed to M2e); Transpose / P12-K2 note the i8-saturation caveat; valuegen::pitch_value_nth no longer implies spelling() is injective. Gates: build/fmt/clippy -D warnings clean; cargo test --workspace green (ops graph_reduction 13, ops lib 51); conformance_suite scale 1 passes. The unrelated Agent-I working tree is left untouched; this commit stages only ops/testkit/spec. Co-Authored-By: Claude Opus 4.8 --- crates/epiphany-ops/DECISIONS.md | 42 ++++++++ crates/epiphany-ops/src/payload.rs | 8 +- crates/epiphany-ops/src/reduce.rs | 76 ++++++++++++-- crates/epiphany-ops/src/valuegen.rs | 7 +- crates/epiphany-ops/tests/graph_reduction.rs | 103 +++++++++++++++++++ crates/epiphany-testkit/src/generators.rs | 57 ++++++++-- spec/PASS12_BATCH.md | 2 +- 7 files changed, 274 insertions(+), 21 deletions(-) diff --git a/crates/epiphany-ops/DECISIONS.md b/crates/epiphany-ops/DECISIONS.md index 9265f25..ea20a44 100644 --- a/crates/epiphany-ops/DECISIONS.md +++ b/crates/epiphany-ops/DECISIONS.md @@ -66,6 +66,48 @@ graph. Insert/delete, voice promotion, supported reference-level cross-cutting values, system breaks, migration checks, transaction rollback, and undo mutate that graph. The base-free `reduce()` remains the operation-set convergence API. +## M2a (Group 1) — event & pitch leaf-field ops: graph materialization + +The Group-1 leaf-field ops (`ModifyEvent`, `Transpose`, `InsertIdentifiedPitch`, +`DeleteIdentifiedPitch`, `ModifyIdentifiedPitch`) reuse M1's reduction +disciplines unchanged; the canonical `MaterializedState` records only their +effect-log entry and — for the field-overwrite ops — a `StructuralFieldCollision` +on a concurrent differing write. Their resolved values live in the graph +(`reduce_onto`), which is derived state, not a second canonical store, so the two +decisions below are **graph-materialization-only**: the bookkeeping projection, +and therefore convergence/determinism, is unaffected. Both exist because an +in-place `EventArena::get_mut` edit bypasses `insert`'s well-formedness guard, so +the reducer must keep the graph Chapter-5-valid itself (otherwise the malformed +state surfaces only later, in `check_invariants`). Both are exercised at scale by +`run_graph_convergence` (criterion 1, now emitting these kinds) and pinned by +targeted `reduce_onto` tests in `tests/graph_reduction.rs`. + +- **A note and a rest are the same slot under pitch add/remove (note↔rest + conversion).** `DeleteIdentifiedPitch` of a single-pitch note's *only* pitch + degrades the event to a `Rest` of the same id/voice/position/duration rather + than leaving an empty pitched event — Chapter 5 forbids the empty chord ("use + `Rest` for the no-pitch case", `ArenaError::EmptyPitchedEvent`). + `InsertIdentifiedPitch` into a rest is the dual: the rest becomes a one-pitch + note. This preserves the ops' disciplines (delete-wins / mint) and keeps the + graph consistent with the bookkeeping (which tombstones / mints the pitch + object either way). *Rejected alternative:* a "would empty the event" + precondition failure — it needs a new `PreconditionFailureReason` against a + ratified set, breaks delete-wins, and is a worse fit than the editor-natural + "deleting a note's last pitch leaves a rest". **For the spec:** the Operation + Catalog §"Insert/Delete identified pitch" (M2e) ratifies this note↔rest + equivalence normatively. + +- **`ModifyEvent` defers placement changes in the graph.** A `ModifyEvent` whose + payload moves the event (different position or duration) is *not* applied to + the graph: re-sorting a voice's event list on a placement change is deferred, + and applying a move via `get_mut` would break invariant 3 + (`VoiceEventsSortedNonOverlap`). Same-placement field edits apply, preserving + the existing voice membership (owned by the voice list); a malformed (empty) + pitched replacement is likewise skipped. The LWW bookkeeping records the modify + either way. **For the spec:** the catalog §ModifyEvent (M2e) states the + placement-change deferral as the prototype boundary (a full re-sort/move op is + a later refinement). + ## Pass 11 candidates (ambiguities for the spec, not resolved in code) ### P11-C1 — operation payload schemas are deferred; we carry identifiers + fingerprints diff --git a/crates/epiphany-ops/src/payload.rs b/crates/epiphany-ops/src/payload.rs index 7c882b9..a6876c8 100644 --- a/crates/epiphany-ops/src/payload.rs +++ b/crates/epiphany-ops/src/payload.rs @@ -679,9 +679,11 @@ impl CanonicalEncode for ModifyEventOp { } /// Transpose live pitches by a chromatic interval (Chapter 6 §6.10 Transpose). -/// Pitch identifiers are preserved; reduction is order-dependent (transpositions -/// do not commute). `chromatic_steps` is a minimal interval (a CMN alteration -/// shift); rich interval algebra is deferred (Chapter 4 tuning catalog; P12-K2). +/// Pitch identifiers are preserved; reduction is order-dependent in the general +/// case (interval composition need not commute). `chromatic_steps` is a minimal +/// interval (a CMN alteration shift) that, in this prototype, commutes except at +/// the alteration's `i8` saturation bound; rich interval algebra is deferred +/// (Chapter 4 tuning catalog; P12-K2). #[derive(Clone, PartialEq, Eq, Debug)] pub struct TransposeOp { pub targets: Vec, diff --git a/crates/epiphany-ops/src/reduce.rs b/crates/epiphany-ops/src/reduce.rs index 3ebd4d2..1870c34 100644 --- a/crates/epiphany-ops/src/reduce.rs +++ b/crates/epiphany-ops/src/reduce.rs @@ -2024,10 +2024,27 @@ impl<'a> Reducer<'a> { let Some(score) = self.graph.as_mut() else { return; }; + // A ModifyEvent carrying a malformed (empty) pitched event must not + // corrupt the arena: `get_mut` bypasses `insert`'s well-formedness guard, + // so an empty chord would only be caught later by `check_invariants`. + // Skip the graph replace in that case (bookkeeping still records it). + if let Event::Pitched(pe) = new_event { + if !pe.is_well_formed() { + return; + } + } if let Some(existing) = score.events.get_mut(new_event.id()) { - // Preserve the original voice membership (placement is owned by the - // voice lists; ModifyEvent overwrites the event's fields, not its - // container). Re-sorting on a position change is deferred. + // Re-sorting a voice on a placement change is deferred, so a + // ModifyEvent that moves the event (different position or duration) + // is not applied to the graph yet: doing so via `get_mut` would + // break invariant 3 (voice events sorted, non-overlapping). The LWW + // bookkeeping still records it; same-placement field edits apply, + // preserving the original voice membership (owned by the voice list). + if new_event.position() != existing.position() + || new_event.duration() != existing.duration() + { + return; + } let voice = existing.voice(); let mut replacement = new_event.clone(); replacement.set_voice(voice); @@ -2047,10 +2064,33 @@ impl<'a> Reducer<'a> { let Some(score) = self.graph.as_mut() else { return; }; - if let Some(Event::Pitched(pe)) = score.events.get_mut(event) { + let Some(slot) = score.events.get_mut(event) else { + return; + }; + if let Event::Pitched(pe) = slot { if !pe.pitches.iter().any(|ip| ip.id == pitch.id) { pe.pitches.push(pitch.clone()); } + return; + } + // Adding a pitch to a rest turns the rest into a note — the dual of a + // last-pitch delete (below). Without this, the bookkeeping mints the + // pitch live while the graph silently drops it (a non-pitched slot has + // no pitch list), so the two would diverge. + if let Event::Rest(rest) = slot { + let replacement = epiphany_core::PitchedEvent { + id: rest.id, + voice: rest.voice, + position: rest.position.clone(), + duration: rest.duration.clone(), + pitches: vec![pitch.clone()], + articulations: Vec::new(), + dynamic: None, + ornaments: Vec::new(), + stem: epiphany_core::StemConfiguration, + grace: None, + }; + *slot = Event::Pitched(replacement); } } @@ -2061,8 +2101,28 @@ impl<'a> Reducer<'a> { let Some(event) = Self::graph_event_of_pitch(score, pitch) else { return; }; - if let Some(Event::Pitched(pe)) = score.events.get_mut(event) { - pe.pitches.retain(|ip| ip.id != pitch); + let Some(slot) = score.events.get_mut(event) else { + return; + }; + if let Event::Pitched(pe) = slot { + if pe.pitches.iter().filter(|ip| ip.id != pitch).count() == 0 { + // Removing the last pitch would leave an empty (invalid) pitched + // event; Chapter 5 forbids that ("use Rest for the no-pitch + // case"), so the note degrades to a rest of the same placement + // and duration. Keeps `get_mut` from materializing a malformed + // chord that `check_invariants` would later reject. + let rest = epiphany_core::Rest { + id: pe.id, + voice: pe.voice, + position: pe.position.clone(), + duration: pe.duration.clone(), + vertical_position: None, + visible: true, + }; + *slot = Event::Rest(rest); + } else { + pe.pitches.retain(|ip| ip.id != pitch); + } } } @@ -2089,7 +2149,9 @@ impl<'a> Reducer<'a> { }; if let Some(Event::Pitched(pe)) = score.events.get_mut(event) { if let Some(ip) = pe.pitches.iter_mut().find(|ip| ip.id == pitch) { - // Minimal interval: shift the CMN alteration. Full interval + // Minimal interval: shift the CMN alteration, saturating at the + // `i8` bound (a lossy stand-in — an extreme transpose clamps + // rather than renormalizing nominal/octave). Full interval // algebra (Chapter 4 tuning) is deferred — P12-K2. if let epiphany_core::PitchSpacePosition::Cmn { alteration, .. } = &mut ip.pitch.scale_position.position diff --git a/crates/epiphany-ops/src/valuegen.rs b/crates/epiphany-ops/src/valuegen.rs index d34d4a8..b43517e 100644 --- a/crates/epiphany-ops/src/valuegen.rs +++ b/crates/epiphany-ops/src/valuegen.rs @@ -51,9 +51,10 @@ pub fn identified_pitch(id: PitchId) -> IdentifiedPitch { } } -/// A distinct CMN [`Pitch`] per `nth` (injective over the `u8`, like -/// [`spelling`]): nominal = `nth % 7`, octave = `nth / 7`. Lets a harness make -/// concurrent `ModifyIdentifiedPitch`es agree or conflict deterministically. +/// A distinct CMN [`Pitch`] per `nth`: nominal = `nth % 7`, octave = `nth / 7`. +/// Unlike [`spelling`] (which fixes the octave, so distinct `nth` can collide), +/// this is injective over the whole `u8`, letting a harness make concurrent +/// `ModifyIdentifiedPitch`es agree or conflict deterministically. pub fn pitch_value_nth(nth: u8) -> Pitch { let nominal = match nth % 7 { 0 => CmnNominal::C, diff --git a/crates/epiphany-ops/tests/graph_reduction.rs b/crates/epiphany-ops/tests/graph_reduction.rs index 42d5e13..4a361f5 100644 --- a/crates/epiphany-ops/tests/graph_reduction.rs +++ b/crates/epiphany-ops/tests/graph_reduction.rs @@ -535,3 +535,106 @@ fn graph_materialization_is_deterministic_across_base_corpus_and_delivery_order( ); } } + +#[test] +fn delete_last_identified_pitch_degrades_the_note_to_a_rest() { + // A single-pitch note whose only pitch is deleted must NOT materialize as an + // empty (invalid) pitched event; Chapter 5 forbids that, so it degrades to a + // rest of the same placement (and `check_invariants` would reject otherwise). + let base = epiphany_core::generators::valid_score(100); + let (staff_instance, target_voice) = target(&base); + let event = EventId::new(ReplicaId(60), 0); + let pitch = PitchId::new(ReplicaId(60), 1); + let insert_note = envelope( + 60, + 0, + 10, + CausalContext::new(), + None, + insert(staff_instance, target_voice, event, pitch, 100), + ); + let delete_pitch = envelope( + 60, + 1, + 20, + CausalContext::new().with_seen(ReplicaId(60), 0), + None, + OperationPayload::Primitive(OperationKind::DeleteIdentifiedPitch( + epiphany_ops::DeleteIdentifiedPitchOp { pitch }, + )), + ); + let mut set = OperationSet::new(); + set.accept_all(vec![insert_note, delete_pitch]); + + let result = set.reduce_onto(&base); + + assert!( + matches!( + result.score.events.get(event), + Some(epiphany_core::Event::Rest(_)) + ), + "a note whose last pitch is deleted must become a rest, not an empty chord" + ); + assert!( + matches!( + result.state.objects.get(&TypedObjectId::Pitch(pitch)), + Some(epiphany_ops::ObjectState::Tombstoned { .. }) + ), + "the deleted pitch is tombstoned in the bookkeeping projection" + ); + assert!(check_invariants(&result.score).is_empty()); +} + +#[test] +fn insert_identified_pitch_into_a_rest_promotes_it_to_a_note() { + // Adding a pitch to a rest turns the rest into a note — the dual of the + // last-pitch delete — so the graph holds the pitch the bookkeeping minted + // (otherwise the live pitch object would have no graph counterpart). + let base = epiphany_core::generators::valid_score(100); + let (staff_instance, target_voice) = target(&base); + let rest = EventId::new(ReplicaId(61), 0); + let insert_rest = envelope( + 61, + 0, + 10, + CausalContext::new(), + None, + OperationPayload::Primitive(OperationKind::InsertEvent(InsertEventOp { + staff_instance, + event: valuegen::insert_event_value( + rest, + target_voice, + MusicalPosition(RationalTime::from_int(100)), + MusicalDuration::whole(), + &[], + ), + })), + ); + let pitch = PitchId::new(ReplicaId(61), 1); + let add_pitch = envelope( + 61, + 1, + 20, + CausalContext::new().with_seen(ReplicaId(61), 0), + None, + OperationPayload::Primitive(OperationKind::InsertIdentifiedPitch( + epiphany_ops::InsertIdentifiedPitchOp { + event: rest, + pitch: valuegen::identified_pitch(pitch), + }, + )), + ); + let mut set = OperationSet::new(); + set.accept_all(vec![insert_rest, add_pitch]); + + let result = set.reduce_onto(&base); + + match result.score.events.get(rest) { + Some(epiphany_core::Event::Pitched(pe)) => assert!( + pe.pitches.iter().any(|ip| ip.id == pitch), + "the inserted pitch is present on the promoted note" + ), + other => panic!("expected the rest to become a note, got {other:?}"), + } + assert!(check_invariants(&result.score).is_empty()); +} diff --git a/crates/epiphany-testkit/src/generators.rs b/crates/epiphany-testkit/src/generators.rs index 256a73f..bdbe097 100644 --- a/crates/epiphany-testkit/src/generators.rs +++ b/crates/epiphany-testkit/src/generators.rs @@ -1050,18 +1050,61 @@ pub fn graph_edit_session( let total = targets.len() as u64 * n; for _ in 0..80 { let r = rng.below(2) as usize; - let payload = if rng.boolean() { - OperationPayload::Primitive(OperationKind::DeleteEvent(DeleteEventOp { + // Mix the original edit kinds with the Group-1 (M2) leaf-field ops so the + // real-Score gate exercises their *graph* materialization (reduce_onto + + // check_invariants), not just the bookkeeping projection. Each targets a + // live object minted by the insert phase above. + let kind = match rng.below(7) { + 0 => OperationKind::DeleteEvent(DeleteEventOp { event: obj_event(rng.below(total)), tuplet_compensation: TupletCompensation::NotInTuplet, - })) - } else { - OperationPayload::Primitive(OperationKind::RespellPitch(RespellPitchOp { + }), + 1 => OperationKind::RespellPitch(RespellPitchOp { pitch: obj_pitch(rng.below(total)), spelling: valuegen::spelling(rng.below(4) as u8 + 1), - })) + }), + 2 => { + // Rebuild the event at its *original* placement (so the graph + // applies the modify rather than deferring it as a move). + let idx = rng.below(total); + let (_, voice) = targets[(idx / n) as usize]; + OperationKind::ModifyEvent(ModifyEventOp { + event: valuegen::insert_event_value( + obj_event(idx), + voice, + MusicalPosition( + RationalTime::new( + GRAPH_SESSION_OFFSET * EVENTS_PER_BAR as i64 + (idx % n) as i64, + 2, + ) + .unwrap(), + ), + MusicalDuration(RationalTime::new(1, 2).unwrap()), + &[obj_pitch(idx)], + ), + }) + } + 3 => OperationKind::Transpose(TransposeOp { + targets: vec![obj_pitch(rng.below(total))], + chromatic_steps: rng.below(5) as i32 - 2, + }), + // Fresh pitch id (beyond the inserted 0..total range): adds a pitch to + // a note, or turns a rest (left by a last-pitch delete) back into one. + 4 => OperationKind::InsertIdentifiedPitch(InsertIdentifiedPitchOp { + event: obj_event(rng.below(total)), + pitch: valuegen::identified_pitch(obj_pitch(total + rng.below(total))), + }), + // Deletes a single-pitch note's only pitch → exercises the note→rest + // degradation path. + 5 => OperationKind::DeleteIdentifiedPitch(DeleteIdentifiedPitchOp { + pitch: obj_pitch(rng.below(total)), + }), + _ => OperationKind::ModifyIdentifiedPitch(ModifyIdentifiedPitchOp { + pitch: obj_pitch(rng.below(total)), + value: valuegen::pitch_value_nth(rng.below(7) as u8), + }), }; - session.author(rng, r, payload); + session.author(rng, r, OperationPayload::Primitive(kind)); } (targets, session.out) } diff --git a/spec/PASS12_BATCH.md b/spec/PASS12_BATCH.md index 290779b..1d8543c 100644 --- a/spec/PASS12_BATCH.md +++ b/spec/PASS12_BATCH.md @@ -30,7 +30,7 @@ code instead is the failure mode this batch exists to prevent. | P12-I2 | `epiphany-render-svg` / `engrave` I | Stable layout-object id derivation (`MUSCLOID`, Pass-11 item 2.6, deferred to I) is still unwired: the frozen `epiphany-determinism` exposes no `MUSCLOID` tag, so provenance is traced via the provisional `stable_id`. Wiring the ratified derivation is Track A work (already noted in `layout-ir/DECISIONS.md`). | G (determinism tag) / Track A | | P12-I3 | `epiphany-layout-ir` I | The bundled `BRAVURA_METRICS` are *approximations* that disagree with the genuine Bravura outlines the renderer now extracts from the font (e.g. `timeSig4`: metrics bbox `[40,0,1240,2048]` vs real outline ≈ `[0.08,-1.0,1.8,1.004]` staff spaces). Real spacing needs exact metrics; regenerate the metrics table from the font or reconcile it with the outline source. | G / Pass 12 (glyph metrics) | | P12-K1 | `epiphany-ops` K | A v0 `RespellPitch` carried a `ContentHash` *fingerprint* of the spelling, not the `PitchSpelling`. The v0→v1 migration (Operation Catalog, M1) cannot invert a fingerprint, so it recovers the spelling from the score-graph context (an explicit per-pitch spelling attachment whose canonical bytes hash to the fingerprint) and returns `MigrationError::Irreversible` (bundle opens read-only) when the context lacks it. Every other representative payload migrates self-contained; this is the lone exception. Confirm the read-only fallback is the intended disposition vs. requiring a v0 corpus that preserves spelling pre-images. | G / Pass 12 (migration) | -| P12-K2 | `epiphany-ops` K | The `Transpose` op (Operation Catalog, M2 Group 1) carries a minimal `chromatic_steps: i32` interval and `reduce_onto` applies it as a CMN *alteration* shift only. Faithful interval algebra (diatonic vs. chromatic intervals, octave/nominal renormalization, transposition in non-CMN pitch spaces) is the deferred Chapter 4 tuning-catalog territory. Pin the interval representation and transposition semantics when the tuning catalog lands. | G / Pass 12 (tuning) | +| P12-K2 | `epiphany-ops` K | The `Transpose` op (Operation Catalog, M2 Group 1) carries a minimal `chromatic_steps: i32` interval and `reduce_onto` applies it as a CMN *alteration* shift only. Faithful interval algebra (diatonic vs. chromatic intervals, octave/nominal renormalization, transposition in non-CMN pitch spaces) is the deferred Chapter 4 tuning-catalog territory. The prototype also clamps the shifted alteration to the `i8` range, so an extreme transpose silently saturates instead of renormalizing — another reason the representation needs pinning. Pin the interval representation and transposition semantics when the tuning catalog lands. | G / Pass 12 (tuning) | ## Not yet open elsewhere