This repository was archived by the owner on Aug 12, 2026. It is now read-only.
Repository navigation
fix(core)!: enforce atomic moves and validated runtime state - #111
Merged
Merged
Conversation
- parse CLI options into invariant-bearing values and reject invalid configurations - propose topology moves from immutable inputs and publish rebuilt manifolds only on success - use owner-safe CGAL snapshots, scope-owned MPFR values, and atomic checkpoint replacement BREAKING CHANGE: Manifold and FoliatedTriangulation no longer expose mutable triangulations or handle-backed cache containers. Move callbacks now accept const manifolds and return synchronized replacement values. Replace update() with updated(), and use delaunay_snapshot() plus scalar queries for inspection. Closes #91
acgetchell
enabled auto-merge (squash)
July 20, 2026 06:01
WalkthroughThe PR centralizes runtime validation, changes manifold and move operations to snapshot-based value semantics, adds MPFR value helpers, hardens serialization, and expands tests for CLI boundaries, move atomicity, ownership, and exception behavior. ChangesValidated topology and runtime flow
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related issues
Possibly related PRs
Sequence Diagram(s)sequenceDiagram
participant CLI
participant RuntimeConfig
participant Manifold
participant Move
CLI->>RuntimeConfig: Validate CLI parameters
RuntimeConfig-->>CLI: Return triangulation and simulation config
CLI->>Manifold: Construct validated universe
Move->>Manifold: Request Delaunay snapshot
Move->>Move: Apply move to local triangulation
Move-->>Manifold: Return rebuilt manifold proposal
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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 `@include/Foliated_triangulation.hpp`:
- Around line 1103-1111: Update the empty-source branch of the
FoliatedTriangulation copy constructor to copy m_initial_radius and
m_foliation_spacing from other before returning, while preserving the existing
triangulation-copy path for non-empty sources.
In `@include/Runtime_config.hpp`:
- Around line 189-225: Factor the shared point-count and Int_precision
upper-bound calculation from make_generated_population() and
make_foliated_ball() into a small reusable helper, using
expected_points_per_timeslice() as the common source. Update both entry points
to consume the helper’s bounds result, while retaining make_foliated_ball()’s
per-timeslice loop validation separately.
In `@tests/Vertex_test.cpp`:
- Around line 25-33: Strengthen snapshot_vertices_are_owned instead of checking
only handles sourced from the snapshot. Ensure the test asserts a non-empty
collected vertex set and validates its count against the corresponding
manifold/live triangulation expectation, or otherwise verifies snapshot
independence from the live triangulation. Keep the existing snapshot collection
flow and update the REQUIRE call sites to exercise this meaningful ownership
behavior.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 97523a0c-c5c8-4338-87ab-4b22a22e5ca6
📒 Files selected for processing (36)
.github/CONTRIBUTING.mdREADME.mdcmake/ExpectCommandFailure.cmakeinclude/Apply_move.hppinclude/Ergodic_moves_3.hppinclude/Foliated_triangulation.hppinclude/Geometry.hppinclude/Manifold.hppinclude/Metropolis.hppinclude/Move_command.hppinclude/Mpfr_value.hppinclude/Periodic_3_complex.hppinclude/Periodic_3_triangulations.hppinclude/Runtime_config.hppinclude/S3Action.hppinclude/Utilities.hppsrc/CMakeLists.txtsrc/cdt.cppsrc/initialize.cpptests/Apply_move_test.cpptests/Bistellar_flip_test.cpptests/CMakeLists.txttests/Ergodic_moves_3_test.cpptests/Foliated_triangulation_test.cpptests/Function_ref_test.cpptests/Geometry_test.cpptests/Manifold_test.cpptests/Metropolis_test.cpptests/Move_always_test.cpptests/Move_command_test.cpptests/Move_tracker_test.cpptests/Runtime_config_test.cpptests/S3Action_test.cpptests/Tetrahedron_test.cpptests/Utilities_test.cpptests/Vertex_test.cpp
💤 Files with no reviewable changes (1)
- tests/Apply_move_test.cpp
- use a contiguous spacelike-facet cache so move construction remains noexcept across standard libraries - centralize generated-population bounds before allocation - preserve physical parameters in empty copies and strengthen snapshot ownership checks
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
tests/Foliated_triangulation_test.cpp (1)
77-81: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for
require_nonempty()'s throw path.The PR adds an explicit rejection of empty Delaunay input (
require_nonempty), directly matching the stated objective to "reject configurations that would create an empty triangulation." No test here exercises constructingFoliatedTriangulation_3from an empty/defaultDelaunay_t<3>and asserting it throwsstd::invalid_argument.Suggested addition near this scenario:
THEN("Constructing from an empty Delaunay triangulation throws.") { REQUIRE_THROWS_AS(FoliatedTriangulation_3{Delaunay_t<3>{}}, std::invalid_argument); }Want me to draft the full test case?
As per path instructions: "Comprehensive test coverage and proper test organization."🤖 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/Foliated_triangulation_test.cpp` around lines 77 - 81, Add a test case near the existing Delaunay constructibility scenario that constructs FoliatedTriangulation_3 from a default-empty Delaunay_t<3> and asserts it throws std::invalid_argument, covering the require_nonempty() rejection path.Source: Path instructions
include/Foliated_triangulation.hpp (2)
1300-1300: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNaming:
is_fixed()mutates state, unlike itsis_*siblings.Every other
is_*predicate in this class (is_correct,is_delaunay,is_tds_valid,is_foliated,is_initialized) is a pure, side-effect-free query.is_fixed()swaps a rebuilt replacement into*thiswhen changes are detected — afix()/try_fix()name would better signal the mutation to callers, reducing the risk of someone treating it as a cheap, non-mutating check.As per path instructions, this segment falls under: "Focus on design patterns, interfaces, and encapsulation."
🤖 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 `@include/Foliated_triangulation.hpp` at line 1300, Rename the mutating is_fixed() method to fix() or try_fix() so its interface clearly signals that it may rebuild and replace the current triangulation. Update all declarations, definitions, and call sites consistently, while leaving the pure is_* predicates unchanged.Source: Path instructions
1299-1316: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
is_fixed()may under-fix:fix_timevaluesneeds to loop, not run once.
fix_timevaluesremoves vertices whose cells are acausal/unclassified in a single detect-and-remove pass, but removing vertices re-triangulates the local star and can introduce new invalid cells. That's exactly why the free-functionmake_triangulation()loopsfix_timevaluesup toMAX_FIX_PASSES(lines 1030‑1037).is_fixed()calls it only once, so a triangulation needing multiple rounds of vertex removal can come back only partially fixed whilechangedstill reportstrue, and the resultingreplacementmay still violateis_correct().🐛 Proposed fix: loop fix_timevalues to convergence
[[nodiscard]] auto is_fixed() -> bool { Delaunay updated{m_triangulation}; auto const fixed_vertices = foliated_triangulations::fix_vertices<3>( updated, m_initial_radius, m_foliation_spacing); - auto const fixed_cells = foliated_triangulations::fix_cells<3>(updated); - auto const fixed_timeslices = - foliated_triangulations::fix_timevalues<3>(updated); + bool fixed_timeslices = false; + for (auto pass = 0; pass < MAX_FIX_PASSES && + foliated_triangulations::fix_timevalues<3>(updated); + ++pass) + { + fixed_timeslices = true; + } + auto const fixed_cells = foliated_triangulations::fix_cells<3>(updated); auto const changed = fixed_vertices || fixed_cells || fixed_timeslices;🤖 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 `@include/Foliated_triangulation.hpp` around lines 1299 - 1316, Update FoliatedTriangulation::is_fixed() so fix_timevalues<3>(updated) runs repeatedly, matching make_triangulation()’s bounded MAX_FIX_PASSES behavior, until no further time-value fixes occur or the pass limit is reached. Accumulate whether any pass changed the triangulation in fixed_timeslices, while preserving the existing vertex/cell fixes and replacement logic.
🤖 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.
Outside diff comments:
In `@include/Foliated_triangulation.hpp`:
- Line 1300: Rename the mutating is_fixed() method to fix() or try_fix() so its
interface clearly signals that it may rebuild and replace the current
triangulation. Update all declarations, definitions, and call sites
consistently, while leaving the pure is_* predicates unchanged.
- Around line 1299-1316: Update FoliatedTriangulation::is_fixed() so
fix_timevalues<3>(updated) runs repeatedly, matching make_triangulation()’s
bounded MAX_FIX_PASSES behavior, until no further time-value fixes occur or the
pass limit is reached. Accumulate whether any pass changed the triangulation in
fixed_timeslices, while preserving the existing vertex/cell fixes and
replacement logic.
In `@tests/Foliated_triangulation_test.cpp`:
- Around line 77-81: Add a test case near the existing Delaunay constructibility
scenario that constructs FoliatedTriangulation_3 from a default-empty
Delaunay_t<3> and asserts it throws std::invalid_argument, covering the
require_nonempty() rejection path.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 040dab80-a988-4d57-a12e-96ac6ae3dfef
📒 Files selected for processing (5)
include/Foliated_triangulation.hppinclude/Runtime_config.hppinclude/Utilities.hpptests/Foliated_triangulation_test.cpptests/Vertex_test.cpp
This was referenced Jul 20, 2026
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 subscribe to this conversation on GitHub.
Already have an account?
Sign in.
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: Manifold and FoliatedTriangulation no longer expose mutable triangulations or handle-backed cache containers. Move callbacks now accept const manifolds and return synchronized replacement values. Replace update() with updated(), and use delaunay_snapshot() plus scalar queries for inspection.
Closes #91
Summary by CodeRabbit