fix!(cdt): enforce Delaunay initialization invariants#123
Merged
acgetchell merged 3 commits intomainfrom May 7, 2026
Merged
Conversation
- Require constructor-grade Level 1-4 Delaunay validation before publishing labeled, strip, and toroidal CDT meshes. - Split evolved-state validation from initialization so completed simulations enforce structural PL-manifold, topology, foliation, causality, and cell-classification invariants without requiring Delaunay-ness. - Add validation cadence controls, checkpoint invariant checks, typed validation error categories, and regression coverage for the blocked periodic toroidal observables behavior. BREAKING CHANGE: CDT validation and error APIs now use stricter constructor checks and typed error categories, including CdtValidationCheck, DelaunayValidationLevel, CheckpointResumeReason, CdtTopology, and MoveType fields in public error variants.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
WalkthroughCentralizes and types CDT/Delaunay validation and errors, separates strict constructor (initial) vs evolved-state validation with configurable cadence, switches toroidal construction to a periodic-image Delaunay generator, removes toroidal post-move early rejection, and adds feature-gated slow integration and regression tests. ChangesCore validation, backends, builders, wiring, and tests
Sequence Diagram(s) sequenceDiagram
autonumber
participant M as Metropolis
participant T as CdtTriangulation
participant D as DelaunayBackend
participant C as Checkpoint/Serializer
M->>T: request mutation candidate / step
T->>D: apply mutation (insert/flip/subdivide)
D->>D: validate_structural() (Level 1-3)
alt structural failure
D-->>T: ValidationFailed(Level 3) + detail
D->>D: rollback snapshot
T-->>M: mutation rejected
else structural ok
D-->>T: mutation accepted
T->>T: validate_evolved_cdt_if_due()
alt evolved validation due
T->>D: validate_delaunay() (Level 1-4)
D-->>T: OK / ValidationFailed(Level 4)
T-->>M: continue or surface validation failure
end
end
M->>C: serialize checkpoint
C->>D: persist delaunay_check_policy
C-->>M: checkpoint saved
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #123 +/- ##
==========================================
+ Coverage 91.96% 92.49% +0.52%
==========================================
Files 19 19
Lines 9774 10315 +541
==========================================
+ Hits 8989 9541 +552
+ Misses 785 774 -11
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
tests/proptest_foliation.rs (1)
71-93:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAssert toroidal cell classification here too.
This property now checks Delaunay/topology/foliation/causality, but it still never exercises
validate_cell_classification(). A toroidal constructor regression that preserves the other invariants would still pass this test.Based on PR objectives: constructors should enforce topology, foliation, causality, and strict cell classification before returning.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/proptest_foliation.rs` around lines 71 - 93, The test toroidal_cdt_static_invariants currently never verifies cell classification; after constructing the triangulation with CdtTriangulation::from_toroidal_cdt and the existing Delaunay/topology/foliation/causality assertions, add an assertion that tri.validate_cell_classification() returns Ok (e.g. prop_assert!(tri.validate_cell_classification().is_ok())) so the toroidal constructor is required to enforce strict cell classification before returning.
🧹 Nitpick comments (1)
tests/proptest_foliation.rs (1)
175-191: ⚡ Quick winThis “determinism” property only checks aggregate counts.
Two different meshes with the same counts and
slice_sizes()will still pass here. Consider comparing a stable mesh fingerprint instead, such as serialized geometry or sorted vertex/face data, so the test actually detects nondeterministic strip construction.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/proptest_foliation.rs` around lines 175 - 191, The test cdt_strip_determinism currently only compares aggregate counts (vertex_count, edge_count, face_count, slice_sizes) which can miss non-deterministic differences; modify the test to compare a stable mesh fingerprint: call CdtTriangulation::from_cdt_strip twice and serialize or produce a deterministic canonical representation (e.g., sorted vertex list and sorted face/index lists or a stable binary/JSON serialization) for t1 and t2, then assert equality of those fingerprints instead of or in addition to the counts so the test detects structural differences in the triangulation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/geometry/backends/delaunay.rs`:
- Around line 659-666: The validate_structural function currently calls
dt.tds().validate(), which only runs Levels 1–2 checks; change it to call
dt.as_triangulation().validate() so Level 3 topology/manifold checks are
included, and keep the existing map_err to convert the returned error into
DelaunayError::ValidationFailed with DelaunayValidationLevel::Three and the
error detail string; update the call site in validate_structural (method name
validate_structural, using self.dt) accordingly.
---
Outside diff comments:
In `@tests/proptest_foliation.rs`:
- Around line 71-93: The test toroidal_cdt_static_invariants currently never
verifies cell classification; after constructing the triangulation with
CdtTriangulation::from_toroidal_cdt and the existing
Delaunay/topology/foliation/causality assertions, add an assertion that
tri.validate_cell_classification() returns Ok (e.g.
prop_assert!(tri.validate_cell_classification().is_ok())) so the toroidal
constructor is required to enforce strict cell classification before returning.
---
Nitpick comments:
In `@tests/proptest_foliation.rs`:
- Around line 175-191: The test cdt_strip_determinism currently only compares
aggregate counts (vertex_count, edge_count, face_count, slice_sizes) which can
miss non-deterministic differences; modify the test to compare a stable mesh
fingerprint: call CdtTriangulation::from_cdt_strip twice and serialize or
produce a deterministic canonical representation (e.g., sorted vertex list and
sorted face/index lists or a stable binary/JSON serialization) for t1 and t2,
then assert equality of those fingerprints instead of or in addition to the
counts so the test detects structural differences in the triangulation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e29b2e90-debd-4060-a7b5-c2df4441d59e
📒 Files selected for processing (27)
Cargo.tomlREADME.mddocs/code_organization.mddocs/dev/commands.mddocs/dev/rust.mddocs/dev/testing.mddocs/dev/tooling-alignment.mddocs/foliation.mdjustfilesrc/cdt/ergodic_moves.rssrc/cdt/metropolis.rssrc/cdt/observables.rssrc/cdt/results.rssrc/cdt/triangulation.rssrc/cdt/triangulation/builders.rssrc/cdt/triangulation/foliation.rssrc/cdt/triangulation/moves.rssrc/cdt/triangulation/validation.rssrc/config.rssrc/errors.rssrc/geometry/backends/delaunay.rssrc/geometry/generators.rssrc/lib.rstests/integration_tests.rstests/physics_integration.rstests/proptest_foliation.rstests/regressions.rs
- Validate Delaunay-backed mutations against the Level 1-3 structural contract before publishing edited geometry. - Roll back insert, remove, flip, and subdivision edits that would leave invalid manifold structure or negative cell orientation. - Treat toroidal checkpoint restore as invalid until upstream periodic-offset serialization preserves the data needed for structural validation. - Strengthen foliation property coverage for toroidal cell classification and deterministic strip mesh structure. BREAKING CHANGE: Delaunay-backed mutation and checkpoint restore paths now reject structurally invalid geometry that could previously be observed after successful edits or deserialization.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/geometry/backends/delaunay.rs (1)
2205-2247: ⚖️ Poor tradeoffConsider an explicitly-broken fixture instead of relying on
seed = 53.The test exercises the rollback branch of
validate_mutation_or_restoreby assuming that flipping the first flippable edge in aseeded_delaunay2(8, (0.0, 10.0), 53)triangulation produces a Level 1–3 invalid geometry. That's a fragile coupling to upstream RNG/flip behavior — any unrelated change to thedelaunaycrate's seeding or candidate-edge ordering could make the flip succeed and silently turn this into a "rollback path never exercised" green test.A more robust alternative is to drive the rollback path deterministically (e.g., feeding
validate_mutation_or_restorea deliberately-corrupted snapshot via a test-only helper, or usingwith_cleared_neighbors_for_testto construct a known-invalid post-state). Not strictly required, but worth considering.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/geometry/backends/delaunay.rs` around lines 2205 - 2247, The test relies on a fragile RNG seed (seeded_delaunay2(..., 53)) to produce a failing flip; instead make the failure deterministic by constructing a known-broken post-flip state and passing that into the rollback path. Modify invalid_structural_flip_rolls_back_geometry to: create the base triangulation via seeded_delaunay2 / DelaunayBackend::from_triangulation as before, pick a flippable edge with edges()/can_flip_edge()/flip_edge, but after performing the flip deliberately corrupt the topology using a test-only helper (e.g., call with_cleared_neighbors_for_test or build a corrupted snapshot) so that validate_mutation_or_restore (or flip_edge's internal call) will deterministically hit the Level 1–3 validation failure and trigger rollback; assert the same error shape and that vertex/edge/face counts and validate_structural() are restored. Reference symbols: invalid_structural_flip_rolls_back_geometry, seeded_delaunay2, DelaunayBackend::from_triangulation, edges(), can_flip_edge, flip_edge, validate_mutation_or_restore, with_cleared_neighbors_for_test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/errors.rs`:
- Around line 50-62: The Display impl for CdtValidationCheck is inconsistent:
change the ErgodicMoveCandidateGeometry arm in impl fmt::Display for
CdtValidationCheck to return the snake_case token (e.g.
"ergodic_move_candidate_geometry") instead of a space-separated phrase so it
matches the other variants used in CdtError::ValidationFailed; after changing
the formatter string, update the test assertion in
validation_check_display_covers_all_categories to expect the new
"ergodic_move_candidate_geometry" token.
---
Nitpick comments:
In `@src/geometry/backends/delaunay.rs`:
- Around line 2205-2247: The test relies on a fragile RNG seed
(seeded_delaunay2(..., 53)) to produce a failing flip; instead make the failure
deterministic by constructing a known-broken post-flip state and passing that
into the rollback path. Modify invalid_structural_flip_rolls_back_geometry to:
create the base triangulation via seeded_delaunay2 /
DelaunayBackend::from_triangulation as before, pick a flippable edge with
edges()/can_flip_edge()/flip_edge, but after performing the flip deliberately
corrupt the topology using a test-only helper (e.g., call
with_cleared_neighbors_for_test or build a corrupted snapshot) so that
validate_mutation_or_restore (or flip_edge's internal call) will
deterministically hit the Level 1–3 validation failure and trigger rollback;
assert the same error shape and that vertex/edge/face counts and
validate_structural() are restored. Reference symbols:
invalid_structural_flip_rolls_back_geometry, seeded_delaunay2,
DelaunayBackend::from_triangulation, edges(), can_flip_edge, flip_edge,
validate_mutation_or_restore, with_cleared_neighbors_for_test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 226c5511-8e80-41f8-b731-fa0aa16a7598
📒 Files selected for processing (5)
src/cdt/triangulation.rssrc/cdt/triangulation/moves.rssrc/errors.rssrc/geometry/backends/delaunay.rstests/proptest_foliation.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/cdt/triangulation.rs
- tests/proptest_foliation.rs
- Emit `ergodic_move_candidate_geometry` consistently with the other `CdtValidationCheck` display identifiers. - Keep validation regression coverage deterministic for structural Delaunay rollback and checkpointed Metropolis runs.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: CDT validation and error APIs now use stricter constructor checks and typed error categories, including CdtValidationCheck, DelaunayValidationLevel, CheckpointResumeReason, CdtTopology, and MoveType fields in public error variants.
Closes #62