Agent K M2a review follow-up: graph-materialization fixes for the leaf-field ops
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 <noreply@anthropic.com>
This commit is contained in:
parent
1658fd18f3
commit
c47f4b5cec
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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<PitchId>,
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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());
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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)
|
||||
}
|
||||
|
|
|
|||
|
|
@ -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
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue