From c2e737d684c93c49d95b7a3881aed7058193038d Mon Sep 17 00:00:00 2001 From: Levi Neuwirth Date: Sun, 21 Jun 2026 19:15:45 -0400 Subject: [PATCH] Item 6 (part 1): Agent E honesty/correctness + Agent D extension preservation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit E-D (layout-ir, honest solver tier): add SolverTier::Stub (a non-conformance rung below Minimal) and have StubSolver report it instead of falsely claiming the Minimal conformance tier; the passthrough evaluates no constraints and computes no quality metrics. E-C (layout-ir, constraint/reference validation): ConstrainedLayoutIR::validate() now also checks the LayoutConstraint vector — NoCollision/Align/PositionWithin must name glyphs in the set, SystemBreakAt/PageBreakAt must name existing slots, PositionWithin regions must be finite/non-negative — rejecting dangling references instead of silently accepting them. E-B (layout-ir, content-sensitive ScoreVersion): derive ScoreVersion from the whole score's canonical bytes (Agent B's whole-score codec) rather than the layout projection's object identities, so a pure content edit that changes no identifier still changes the version — required for correct incremental-layout cache invalidation. D-A (bundle, extension-root preservation): Bundle::commit now enforces preservation — after the builder closure runs, every prior extension declaration it did not re-declare (by extension_id) is carried forward verbatim, so an extension-unaware writer cannot silently orphan an unknown extension's preserved_chunk_roots. An extension-aware writer that re-declares its id keeps control. Each fix has a regression test; per-crate DECISIONS updated. (Item-6 remainder: D-B operation-block summaries next; E-A real time-axis deferred per request.) Co-Authored-By: Claude Opus 4.8 (1M context) --- crates/epiphany-bundle/DECISIONS.md | 16 ++-- crates/epiphany-bundle/src/bundle.rs | 90 ++++++++++++++++++++ crates/epiphany-layout-ir/DECISIONS.md | 23 ++++- crates/epiphany-layout-ir/src/constrained.rs | 49 +++++++++++ crates/epiphany-layout-ir/src/logical.rs | 71 ++++++++------- crates/epiphany-layout-ir/src/solver.rs | 68 +++++++++++++-- crates/epiphany-testkit/src/layout_stub.rs | 1 + 7 files changed, 268 insertions(+), 50 deletions(-) diff --git a/crates/epiphany-bundle/DECISIONS.md b/crates/epiphany-bundle/DECISIONS.md index f835883..28a8649 100644 --- a/crates/epiphany-bundle/DECISIONS.md +++ b/crates/epiphany-bundle/DECISIONS.md @@ -178,14 +178,18 @@ yet enforced so a later integration knows where to extend. to exercise yet. Non-canonical opaque chunks at unknown majors are carried verbatim (they are never parsed). -- **Extensions: required → read-only; opaque preservation is partial.** An +- **Extensions: required → read-only; opaque preservation is now enforced.** An unknown *required* extension forces read-only (v0 understands no extensions, so all are unknown). Optional-extension `preserved_chunk_roots` are carried in - the manifest, but the bundle does not yet *enforce* that a commit's builder - closure preserves them, nor evaluate edit barriers / the unsafe-edit path — - barrier operands (`OperationKindTag`, `ObjectKind`, `EditBarrier`) are owned by - Agents C/E. The commit closure is, however, validated to never publish - dangling or mismatched *canonical* roots. + the manifest, and (M4 follow-up) `commit` now **enforces** preservation: + after the builder closure runs, every prior extension declaration the closure + did not itself re-declare (by `extension_id`) is carried forward verbatim, so + an extension-*unaware* writer cannot silently orphan an unknown extension's + roots; an extension-*aware* writer that re-declares its own id keeps control. + (Edit barriers / the unsafe-edit path are still not evaluated — barrier + operands `OperationKindTag`/`ObjectKind`/`EditBarrier` are owned by Agents + C/E.) The commit closure is also validated to never publish dangling or + mismatched *canonical* roots. ## Pass 11 candidates (ambiguities for the spec, not resolved in code) diff --git a/crates/epiphany-bundle/src/bundle.rs b/crates/epiphany-bundle/src/bundle.rs index 656e370..fdad198 100644 --- a/crates/epiphany-bundle/src/bundle.rs +++ b/crates/epiphany-bundle/src/bundle.rs @@ -542,6 +542,27 @@ impl Bundle { new_chunks: &new_refs, generation: next_generation, }); + + // Extension-root preservation (Chapter 8 §"Behavior Under Unknown + // Extensions"). The bundle's job is preservation: an extension-unaware + // commit closure must not silently drop unknown extensions and their + // `preserved_chunk_roots` (which would orphan those chunks). Carry + // forward every prior extension declaration the closure did not itself + // re-declare; an extension-*aware* writer that re-declares its own + // `extension_id` keeps full control of that declaration. (The manifest + // encoder sorts/dedups `extension_declarations`, so append order does not + // affect the canonical form.) + let redeclared: std::collections::BTreeSet = manifest + .extension_declarations + .iter() + .map(|e| e.extension_id) + .collect(); + for prior in &previous.extension_declarations { + if !redeclared.contains(&prior.extension_id) { + manifest.extension_declarations.push(prior.clone()); + } + } + manifest.generation = next_generation; manifest.manifest_id = manifest.derive_id(); @@ -1286,6 +1307,75 @@ mod tests { .contains(&IntegrityAnomaly::UnknownRequiredExtension)); } + #[test] + fn commit_preserves_unknown_extension_roots_when_the_closure_drops_them() { + // The bundle's job is preservation: an extension-*unaware* writer that + // rebuilds the manifest from scratch must not orphan an unknown + // (optional) extension's preserved roots. + let mut bundle = fresh_bundle(); + let ext_id = crate::ids::ExtensionId([7; 16]); + let ext_root = ChunkRef { + id: ChunkId(ContentHash([42; 32])), + kind: ChunkKind::ExtensionData, + schema_version: SchemaVersion::V0, + offset: 4096, + compressed_length: 8, + uncompressed_length: 8, + compression: CompressionAlgorithm::None, + hash: ContentHash([42; 32]), + }; + + // 1) An extension-aware commit declares the optional extension + a root. + bundle + .commit(&[], |ctx| { + let mut m = ctx.previous_manifest.clone(); + m.extension_declarations + .push(crate::manifest::ExtensionDeclaration { + extension_id: ext_id, + version: crate::ids::SemVer::new(1, 0, 0), + required: false, + preserved_chunk_roots: vec![ext_root], + affected_object_kinds: Vec::new(), + edit_barriers: Vec::new(), + }); + m + }) + .unwrap(); + assert!(bundle + .manifest() + .extension_declarations + .iter() + .any(|e| e.extension_id == ext_id)); + + // 2) An extension-unaware commit rebuilds the manifest from empty, + // carrying only what it understands (operation roots). The bundle must + // still carry the extension and its root forward. + let doc = bundle.manifest().document_id; + bundle + .commit(&[], |ctx| { + let mut m = Manifest::empty(doc); + m.operation_roots = ctx.previous_manifest.operation_roots.clone(); + m + }) + .unwrap(); + + let survives = |m: &Manifest| { + m.extension_declarations.iter().any(|e| { + e.extension_id == ext_id + && e.preserved_chunk_roots.iter().any(|r| r.id == ext_root.id) + }) + }; + assert!( + survives(bundle.manifest()), + "unknown extension + its root must survive an extension-unaware commit" + ); + + // 3) And it survives a reopen (durably preserved). + let image = bundle.into_store().into_bytes(); + let reopened = Bundle::open(MemStore::from_bytes(image)).unwrap(); + assert!(survives(reopened.manifest())); + } + #[test] fn profile_selection_is_stable_across_reload() { // Finding 9: the superblock's profile_id is the canonical-first profile, diff --git a/crates/epiphany-layout-ir/DECISIONS.md b/crates/epiphany-layout-ir/DECISIONS.md index 817eb93..8b4a007 100644 --- a/crates/epiphany-layout-ir/DECISIONS.md +++ b/crates/epiphany-layout-ir/DECISIONS.md @@ -167,10 +167,31 @@ object is covered); the provenance-preservation contract itself is unchanged. boundary's `RenderIRProducer::produce(resolved, scale, config)` takes the spec's `ScaleContext`/`RenderConfiguration`. The quality-metric/tie-breaking *types* exist; what the QUICKSTART defers is normalization computation. The - exact non-optional interface is preserved: the stub reports `Minimal` and an + exact non-optional interface is preserved: the stub reports the + non-conformance `SolverTier::Stub` rung (M5 follow-up — *not* `Minimal`, since a + passthrough that evaluates no constraints and computes no quality metrics must + not claim the lowest conformance tier; `Stub` orders below `Minimal`), an all-worst `QualityMetricVector`, and rejects explicit constraints it cannot evaluate rather than claiming them satisfied. +- **Constraint references are validated (M5 follow-up).** + `ConstrainedLayoutIR::validate()` now also checks the `LayoutConstraint` + vector: `NoCollision`/`Align`/`PositionWithin` must name glyphs in the set, + `SystemBreakAt`/`PageBreakAt` must name existing spring slots, and a + `PositionWithin` region must be finite/non-negative. Dangling constraint + references are rejected (`UnknownConstraintGlyph`/`UnknownConstraintSlot`/ + `InvalidConstraintRegion`) rather than silently accepted. `Registered` + (extension) constraints stay opaque/conservative. Score-graph *source* + validation (that a `Provenance::source` names a real graph object) still + belongs at the `to_logical` boundary, which holds the `Score`. + +- **`ScoreVersion` is content-sensitive (M5 follow-up).** It is now derived from + the whole score's canonical bytes (Agent B's whole-score codec) rather than the + layout projection's object identities, so a pure content edit (a respelling, a + duration change) that changes no identifier still changes the version — + required for correct incremental-layout cache invalidation (Chapter 7 + §"Incremental Layout"). + ## Pass 11 candidates (ambiguities for the spec, not resolved in code) 1. **Agent E's stated dependency set vs. the edit-barrier types.** The QUICKSTART diff --git a/crates/epiphany-layout-ir/src/constrained.rs b/crates/epiphany-layout-ir/src/constrained.rs index be19be4..d4bc3c2 100644 --- a/crates/epiphany-layout-ir/src/constrained.rs +++ b/crates/epiphany-layout-ir/src/constrained.rs @@ -159,6 +159,12 @@ pub enum ConstrainedValidationError { SlotMismatch(GlyphObjectId), InvalidSlotGeometry(SpringSlotId), InvalidGlyphBounds(GlyphObjectId), + /// A constraint references a glyph that is not in the glyph set. + UnknownConstraintGlyph(GlyphObjectId), + /// A break constraint references a spring slot that does not exist. + UnknownConstraintSlot(SpringSlotId), + /// A `PositionWithin` constraint carries a non-finite or inverted region. + InvalidConstraintRegion(GlyphObjectId), } /// A malformed logical-stage value that cannot be transformed without losing @@ -285,6 +291,49 @@ impl ConstrainedLayoutIR { return Err(ConstrainedValidationError::BandMismatch(glyph.id())); } } + + // Constraints must reference objects that exist: a dangling glyph or + // slot reference is a malformed problem, not a silently-accepted one. + let glyph_exists = |id: GlyphObjectId| -> bool { glyphs_by_id.contains_key(&id) }; + for constraint in &self.constraints { + match constraint { + LayoutConstraint::NoCollision { a, b } | LayoutConstraint::Align { a, b, .. } => { + if !glyph_exists(*a) { + return Err(ConstrainedValidationError::UnknownConstraintGlyph(*a)); + } + if !glyph_exists(*b) { + return Err(ConstrainedValidationError::UnknownConstraintGlyph(*b)); + } + } + LayoutConstraint::PositionWithin { glyph, region } => { + if !glyph_exists(*glyph) { + return Err(ConstrainedValidationError::UnknownConstraintGlyph(*glyph)); + } + let r = [ + region.origin.x.0, + region.origin.y.0, + region.size.width.0, + region.size.height.0, + ]; + let region_ok = r.iter().all(|v| v.is_finite()) + && region.size.width.0 >= 0.0 + && region.size.height.0 >= 0.0; + if !region_ok { + return Err(ConstrainedValidationError::InvalidConstraintRegion(*glyph)); + } + } + LayoutConstraint::SystemBreakAt { slot, .. } + | LayoutConstraint::PageBreakAt { slot, .. } => { + if !slot_ids.contains(slot) { + return Err(ConstrainedValidationError::UnknownConstraintSlot(*slot)); + } + } + // A Registered (extension) constraint is opaque; treated + // conservatively (not rejected) per "Behavior Under Unknown + // Extensions". + LayoutConstraint::Registered(_, _) => {} + } + } Ok(()) } } diff --git a/crates/epiphany-layout-ir/src/logical.rs b/crates/epiphany-layout-ir/src/logical.rs index 534e684..0aa80d4 100644 --- a/crates/epiphany-layout-ir/src/logical.rs +++ b/crates/epiphany-layout-ir/src/logical.rs @@ -16,7 +16,7 @@ use std::collections::BTreeSet; use epiphany_core::{AnnotationAnchor, RegionId, Score, StaffId, TimeAnchor, TypedObjectId}; -use epiphany_determinism::{CanonicalEncode, DomainTag, Preimage}; +use epiphany_determinism::{DomainTag, Preimage}; use crate::engraving::{EngravingDecision, EngravingDecisionKind, EngravingOverride}; use crate::provenance::{LayoutObjectId, Provenance}; @@ -368,7 +368,7 @@ pub fn to_logical(score: &Score) -> LogicalLayoutIR { } } - let source = derive_score_version(®ions, &cross_region); + let source = derive_score_version(score); LogicalLayoutIR { source, regions, @@ -378,42 +378,18 @@ pub fn to_logical(score: &Score) -> LogicalLayoutIR { } } -fn derive_score_version( - regions: &[LayoutRegion], - cross_region: &[CrossRegionObject], -) -> ScoreVersion { +/// Derives the [`ScoreVersion`] from the **whole score's canonical content** +/// (Agent B's whole-score codec), not merely the layout projection's object +/// identities. Any score edit — including one that changes an event's content +/// without changing any identifier (e.g. a respelling or a duration change) — +/// therefore yields a different version, which is what incremental-layout cache +/// invalidation depends on (Chapter 7 §"Incremental Layout"). The former +/// derivation keyed on layout-object `stable_id`s alone, so a pure content edit +/// left the version unchanged. +fn derive_score_version(score: &Score) -> ScoreVersion { let mut preimage = Preimage::new(DomainTag::CONFLICT); preimage.push_bytes(b"layout-score-version"); - for region in regions { - preimage.push_bytes(®ion.provenance.source.to_canonical_bytes()); - match ®ion.time_axis { - TimeAxisModel::Metric(_) => { - preimage.push_u64_le(0); - } - TimeAxisModel::Proportional(axis) => { - preimage.push_u64_le(1); - preimage.push_u64_le(axis.duration_ns as u64); - preimage.push_u64_le(axis.space_per_second.0.to_bits() as u64); - } - TimeAxisModel::Aleatoric(_) => { - preimage.push_u64_le(2); - } - TimeAxisModel::Registered(id, payload) => { - preimage.push_u64_le(3); - preimage.push_u64_le((id.0 >> 64) as u64); - preimage.push_u64_le(id.0 as u64); - preimage.push_bytes(&payload.0); - } - } - for object in ®ion.objects { - preimage.push_u64_le((object.provenance().stable_id.0 >> 64) as u64); - preimage.push_u64_le(object.provenance().stable_id.0 as u64); - } - } - for object in cross_region { - preimage.push_u64_le((object.provenance.stable_id.0 >> 64) as u64); - preimage.push_u64_le(object.provenance.stable_id.0 as u64); - } + preimage.push_bytes(&score.canonical_bytes()); ScoreVersion(*preimage.finish().as_bytes()) } @@ -557,9 +533,30 @@ pub(crate) fn cross_cutting_objects(score: &Score) -> Vec<(TypedObjectId, Vec SolverTier { - SolverTier::Minimal + // Honest: a passthrough that evaluates no constraints is below Minimal. + SolverTier::Stub } fn version(&self) -> SolverVersion { @@ -541,8 +552,11 @@ mod tests { } #[test] - fn stub_uses_the_minimal_interface_tier_and_worst_metrics() { - assert_eq!(StubSolver.tier(), SolverTier::Minimal); + fn stub_reports_the_non_conformant_stub_tier_and_worst_metrics() { + // Honest non-conformance: a passthrough reports Stub, never Minimal, and + // Stub orders below every real conformance tier. + assert_eq!(StubSolver.tier(), SolverTier::Stub); + assert!(SolverTier::Stub < SolverTier::Minimal); assert_eq!(StubSolver.version(), SolverVersion(0)); let input = constrained(vec![glyph("noteheadBlack")]); assert_eq!( @@ -553,6 +567,48 @@ mod tests { ); } + #[test] + fn validate_rejects_dangling_constraint_references() { + use crate::constrained::{ + BreakKind, ConstrainedValidationError, GlyphObjectId, LayoutConstraint, + }; + let mut input = constrained(vec![glyph("noteheadBlack")]); + assert!(input.validate().is_ok()); + let real = input.glyphs[0].id(); + + // A constraint naming a glyph that is not in the set is rejected, not + // silently accepted. + let ghost = GlyphObjectId(real.0 ^ 0xABCD); + input + .constraints + .push(LayoutConstraint::NoCollision { a: real, b: ghost }); + assert_eq!( + input.validate(), + Err(ConstrainedValidationError::UnknownConstraintGlyph(ghost)) + ); + + // A break constraint on a non-existent slot is rejected. + input.constraints = vec![LayoutConstraint::SystemBreakAt { + slot: SpringSlotId(999), + kind: BreakKind::Hard, + }]; + assert_eq!( + input.validate(), + Err(ConstrainedValidationError::UnknownConstraintSlot( + SpringSlotId(999) + )) + ); + + // A well-formed constraint reference validates — even though the stub + // solver still refuses to *evaluate* it (it cannot claim it satisfied). + input.constraints = vec![LayoutConstraint::NoCollision { a: real, b: real }]; + assert!(input.validate().is_ok()); + assert_eq!( + StubSolver.solve(&input, &SolverConfig::default()).status, + SolveStatus::InternalError + ); + } + #[test] fn unknown_glyph_yields_internal_error_not_panic() { let mut unknown = glyph("noSuchGlyph"); diff --git a/crates/epiphany-testkit/src/layout_stub.rs b/crates/epiphany-testkit/src/layout_stub.rs index 007f3c1..3cb2de5 100644 --- a/crates/epiphany-testkit/src/layout_stub.rs +++ b/crates/epiphany-testkit/src/layout_stub.rs @@ -816,6 +816,7 @@ pub fn gen_edit_context(rng: &mut Rng) -> EditContext { /// A solver tier (every variant). pub fn gen_solver_tier(rng: &mut Rng) -> SolverTier { *rng.choose(&[ + SolverTier::Stub, SolverTier::Minimal, SolverTier::Standard, SolverTier::Advanced,