refactor: type repair diagnostics and harden invariants (#332)#352
Merged
acgetchell merged 4 commits intomainfrom Apr 29, 2026
Merged
refactor: type repair diagnostics and harden invariants (#332)#352acgetchell merged 4 commits intomainfrom
acgetchell merged 4 commits intomainfrom
Conversation
- Replace stringified flip-repair skip samples with typed diagnostic context. - Make vertex removal transactional across post-removal repair and orientation canonicalization. - Deprecate DelaunayTriangulation::as_triangulation_mut ahead of removal in v0.8.0. - Use scale-aware degeneracy checks for low-dimensional simplex and facet measures. - Add regression and property coverage for rollback behavior, typed diagnostics, and scaled valid measures. - Tolerate throughput formatting precision in benchmark baseline round-trip tests.
Contributor
WalkthroughReplaces stringified repair skip diagnostics with typed sample structs, introduces transactional Changes
Sequence Diagram(s)sequenceDiagram
participant Caller
participant DelaunayTriangulation as DT
participant SnapshotStore as Snapshot
participant TDS
participant RepairEngine as Repair
Caller->>DT: remove_vertex(key)
DT->>Snapshot: take_snapshot(tds, insertion_state, spatial_index)
DT->>TDS: tds_mut_for_repair() (invalidate caches)
TDS->>Repair: perform_flip_repair(context)
alt Repair succeeds
Repair-->>TDS: success (mutated TDS)
DT-->>Caller: Ok
else Repair fails
Repair-->>DT: DelaunayRepairError
DT->>Snapshot: restore_snapshot()
DT-->>Caller: Err(RepairOperationFailed { operation: VertexRemoval, source })
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 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. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
coderabbitai
Bot
added
enhancement
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 14 |
🟢 Coverage 94.01% diff coverage · +0.72% coverage variation
Metric Results Coverage variation ✅ +0.72% coverage variation (-1.00%) Diff coverage ✅ 94.01% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (fea5898) 50908 45309 89.00% Head commit (a95c610) 51492 (+584) 46199 (+890) 89.72% (+0.72%) Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch:
<coverage of head commit> - <coverage of common ancestor commit>Diff coverage details
Coverable lines Covered lines Diff coverage Pull request (#352) 802 754 94.01% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/triangulation/delaunay.rs`:
- Around line 8169-8207: The test
test_remove_vertex_rolls_back_on_repair_failure should assert the specific
RepairFailed error instead of accepting any Err and should be made
dimension-generic; replace the broad assert!(result.is_err()) with a precise
assertion that the error matches the RepairFailed variant (e.g. using
matches!(result.unwrap_err(), DelaunayError::RepairFailed{..}) or equivalent
pattern match against RepairFailed) to ensure the forced-repair path (activated
by ForceRepairNonconvergentGuard::enable()) was exercised, and refactor the test
into a pastey macro to run for D=2..=5 so the failure/rollback behavior is
validated across dimensions.
- Around line 6302-6365: After a successful remove_vertex transaction you must
invalidate cached topology-dependent state to avoid stale keys: in the
Ok(cells_removed) success path (the match on result after the closure) clear
self.last_inserted_cell (set to None) and reset/clear self.spatial_index (use
its clear() or replace with an empty spatial index instance) so both caches are
invalidated before returning Ok(cells_removed); locate the match result handling
around remove_vertex / apply_bistellar_flip_k1_inverse and update the success
branch accordingly.
In `@src/triangulation/delaunayize.rs`:
- Around line 559-562: The topology pass is being marked as succeeded
unconditionally by setting topology_repair: PlManifoldRepairStats { succeeded:
true, .. } even when repair_facet_oversharing failed; update the logic in
delaunayize.rs so that topology_repair.succeeded reflects the actual outcome of
repair_facet_oversharing (or the returned PlManifoldRepairStats) instead of
hardcoding true—propagate the real PlManifoldRepairStats from the repair attempt
into topology_repair and only set succeeded = true when the repair call
indicates success (leave false or the default when it failed and a fallback
rebuild is performed).
🪄 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: 24f0f56e-563b-427a-9d47-5679e8405796
📒 Files selected for processing (11)
scripts/tests/test_benchmark_utils.pysrc/core/algorithms/flips.rssrc/core/tds.rssrc/core/triangulation.rssrc/geometry/util/measures.rssrc/lib.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rstests/proptest_geometry.rstests/proptest_triangulation.rs
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #352 +/- ##
==========================================
+ Coverage 88.98% 89.70% +0.72%
==========================================
Files 58 58
Lines 50713 51297 +584
==========================================
+ Hits 45125 46015 +890
+ Misses 5588 5282 -306
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Make vertex removal transactional across repair and orientation canonicalization failures. - Preserve typed repair sources with operation-specific Delaunay diagnostics. - Deprecate direct mutable triangulation access ahead of planned v0.8.0 removal. - Clarify delaunayize fallback repair statistics and documentation. - Add regression, coverage, doctest, and prelude coverage for repair diagnostics and invariant behavior.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (1)
src/triangulation/delaunay.rs (1)
4702-4710:⚠️ Potential issue | 🟠 MajorUse the new repair-access invalidation path consistently.
tds_mut_for_repair()clearslast_inserted_cell, but the in-file manual repair entrypoints still mutateself.tri.tdsdirectly. After a successfulrepair_delaunay_with_flips*call, the next insert can still carry a stale cell hint from pre-repair topology. Please route those paths through the same invalidation logic, or clear the caches before borrowingself.tri.tds.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triangulation/delaunay.rs` around lines 4702 - 4710, Several in-file manual repair entrypoints (e.g., repair_delaunay_with_flips and its variants) mutate self.tri.tds directly but do not run the same cache invalidation done by tds_mut_for_repair(), so stale hints (insertion_state.last_inserted_cell / spatial_index) survive and break subsequent inserts; update those repair call sites to obtain mutable access via tds_mut_for_repair() instead of borrowing self.tri.tds directly, or explicitly clear insertion_state.last_inserted_cell = None and spatial_index = None immediately before any direct mutation of self.tri.tds so all repair paths share the same invalidation logic.
🧹 Nitpick comments (1)
src/triangulation/delaunay.rs (1)
8433-8474: Assert cache restoration in the rollback helper too.This helper proves vertex/cell rollback, but the new contract also restores
insertion_stateandspatial_index. Add assertions forlast_inserted_cellandspatial_indexbefore/after the forced failure so the transactional guarantee stays covered end-to-end.As per coding guidelines, "Unit tests must cover known values, error paths, and dimension-generic correctness for Rust; dimension-generic tests MUST cover D=2 through D=5 using pastey macros".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triangulation/delaunay.rs` around lines 8433 - 8474, The test assert_remove_vertex_rollback must also capture and assert restoration of insertion-related caches: record dt.last_inserted_cell and a snapshot of dt.spatial_index (or dt.insertion_state/spatial_index representation) before calling dt.remove_vertex, then after the forced failure assert those values equal the originals to prove rollback restored last_inserted_cell and spatial_index; add these checks around the ForceRepairNonconvergentGuard::enable / dt.remove_vertex call in assert_remove_vertex_rollback and expand the dimension-generic test to cover D=2 through D=5 using the existing pastey macro pattern so the cache-restore assertions run for each dimension.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/core/algorithms/flips.rs`:
- Around line 8241-8310: The test dynamic_flip_rejects_bad_context is only
instantiated for D=3 and must be made dimension-generic for D=2..=5; wrap the
test body in a pastey macro that generates the same assertions for const D =
2..=5, replacing concrete type uses (Tds<f64, (), (), 3>, VertexKey, CellKey)
with generic equivalents parameterized on D and ensuring FlipContextDyn and
apply_bistellar_flip_dynamic are constructed with the correct arities per D (use
the same removed_face_vertices/inserted_face_vertices/removed_cells shapes but
sized according to D), and invoke apply_bistellar_flip_dynamic(&mut tds, k,
&context) as before; keep all existing assertion messages and final
tds.number_of_vertices()/number_of_cells() checks so the runtime-k guards are
exercised for every dimension.
- Around line 4358-4383: The derived Debug impls for InsertedSimplexSkipSample,
RidgeMultiplicitySkipSample, MissingCellSkipSample and RepairSkipLocation
changed the log payload shape; update these types so emit_repair_debug_summary
continues to emit the original raw summary format by implementing a custom Debug
or Display that reproduces the previous summary string shape (or alternatively
modify emit_repair_debug_summary to format these types explicitly rather than
using ?sample). Locate the types InsertedSimplexSkipSample,
RidgeMultiplicitySkipSample, MissingCellSkipSample and enum RepairSkipLocation
and add a manual fmt::Debug or fmt::Display impl that formats fields exactly as
the old summary text, ensuring all places that call emit_repair_debug_summary
(reference function emit_repair_debug_summary) keep compatible output.
In `@tests/delaunay_public_api_coverage.rs`:
- Around line 141-147: The tracing::debug! diagnostics in
tests/delaunay_public_api_coverage.rs (the calls logging case,
vertex_key/inserted_uuid, "initial vertices" loop, and "inserted vertex coords")
must be gated behind the test-debug feature; wrap those debug invocations in a
feature guard (e.g., surrounding the block with if cfg!(feature = "test-debug")
{ ... } or annotate helper functions/blocks with #[cfg(feature = "test-debug")])
so the logging only runs when the test-debug feature is enabled, keeping the
existing messages and variables (vertex_key, inserted_uuid, vertices,
inserted_coords) intact.
---
Duplicate comments:
In `@src/triangulation/delaunay.rs`:
- Around line 4702-4710: Several in-file manual repair entrypoints (e.g.,
repair_delaunay_with_flips and its variants) mutate self.tri.tds directly but do
not run the same cache invalidation done by tds_mut_for_repair(), so stale hints
(insertion_state.last_inserted_cell / spatial_index) survive and break
subsequent inserts; update those repair call sites to obtain mutable access via
tds_mut_for_repair() instead of borrowing self.tri.tds directly, or explicitly
clear insertion_state.last_inserted_cell = None and spatial_index = None
immediately before any direct mutation of self.tri.tds so all repair paths share
the same invalidation logic.
---
Nitpick comments:
In `@src/triangulation/delaunay.rs`:
- Around line 8433-8474: The test assert_remove_vertex_rollback must also
capture and assert restoration of insertion-related caches: record
dt.last_inserted_cell and a snapshot of dt.spatial_index (or
dt.insertion_state/spatial_index representation) before calling
dt.remove_vertex, then after the forced failure assert those values equal the
originals to prove rollback restored last_inserted_cell and spatial_index; add
these checks around the ForceRepairNonconvergentGuard::enable / dt.remove_vertex
call in assert_remove_vertex_rollback and expand the dimension-generic test to
cover D=2 through D=5 using the existing pastey macro pattern so the
cache-restore assertions run for each dimension.
🪄 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: 65efe486-d59a-453c-ad12-584b62a430d7
📒 Files selected for processing (10)
README.mddocs/api_design.mddocs/dev/rust.mddocs/workflows.mdsrc/core/algorithms/flips.rssrc/geometry/util/measures.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rstests/delaunay_public_api_coverage.rstests/prelude_exports.rs
✅ Files skipped from review due to trivial changes (2)
- README.md
- src/geometry/util/measures.rs
- Preserve legacy repair-summary sample formatting while keeping typed diagnostic samples. - Exercise dynamic flip context guards across D=2 through D=5. - Invalidate insertion caches consistently before repair-driven TDS mutations. - Assert remove-vertex rollback restores insertion hints and spatial-index state. - Gate public API coverage debug tracing behind the test-debug feature.
coderabbitai
Bot
added
the
documentation
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/delaunay_public_api_coverage.rs (1)
89-160: Consider gating the manual search helper behindtest-debug.This is investigation tooling rather than stable regression coverage, so moving it behind the feature flag would keep the default test suite smaller while still preserving the repro hunt when needed.
Suggested tweak
+#[cfg(feature = "test-debug")] #[test] #[ignore = "manual search helper; run explicitly to discover natural repro cases"] fn find_stale_key_after_rebuild() {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/delaunay_public_api_coverage.rs` around lines 89 - 160, The test function find_stale_key_after_rebuild is manual investigation tooling and should be compiled only when the test-debug feature is enabled; wrap the whole test with a conditional compile attribute (e.g. add #[cfg(feature = "test-debug")] immediately above the function) and keep or remove the #[ignore = "..."] attribute as you prefer (keeping it is fine) so the helper is excluded from default test builds and only available when the test-debug feature is enabled.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/triangulation/delaunay.rs`:
- Around line 8455-8536: The test only exercises the inverse-k=1 fast-path
because assert_remove_vertex_rollback() creates the removable vertex via
flip_k1_insert(), leaving the fallback self.tri.remove_vertex(vertex_key) path
untested; add a new dimension-generic test (using the existing
gen_remove_vertex_rollback_tests! macro pattern) or extend
assert_remove_vertex_rollback() to also construct a removable vertex whose star
is not a simplex (i.e., create/insert a vertex that does not use
flip_k1_insert() so that remove_vertex() takes the fallback transactional path),
then run the same forced-repair guard and rollback assertions (checking
dt.number_of_vertices, dt.number_of_cells, insertion_state.last_inserted_cell,
spatial_index snapshot, and that the inserted UUID is present) to cover the
inverse-k!=1 branch for D=2..5.
---
Nitpick comments:
In `@tests/delaunay_public_api_coverage.rs`:
- Around line 89-160: The test function find_stale_key_after_rebuild is manual
investigation tooling and should be compiled only when the test-debug feature is
enabled; wrap the whole test with a conditional compile attribute (e.g. add
#[cfg(feature = "test-debug")] immediately above the function) and keep or
remove the #[ignore = "..."] attribute as you prefer (keeping it is fine) so the
helper is excluded from default test builds and only available when the
test-debug feature is enabled.
🪄 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: ae61e5c4-77d3-4791-a3c8-0f66d116e635
📒 Files selected for processing (4)
src/core/algorithms/flips.rssrc/core/collections/spatial_hash_grid.rssrc/triangulation/delaunay.rstests/delaunay_public_api_coverage.rs
✅ Files skipped from review due to trivial changes (1)
- src/core/collections/spatial_hash_grid.rs
- Add D=2..=5 rollback coverage for the non-k1 remove_vertex fallback path. - Reuse cache and topology rollback assertions across fast-path and fallback removal tests. - Compile the stale-key investigation helper only with the test-debug feature.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/triangulation/delaunay.rs (1)
4782-4786: Reuseinvalidate_repair_caches()here to avoid drift.
as_triangulation_mut()duplicates the invalidation logic that now has a dedicated helper.♻️ Proposed refactor
pub fn as_triangulation_mut(&mut self) -> &mut Triangulation<K, U, V, D> { // Direct mutable access can invalidate performance caches. - self.insertion_state.last_inserted_cell = None; - self.spatial_index = None; + self.invalidate_repair_caches(); &mut self.tri }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/triangulation/delaunay.rs` around lines 4782 - 4786, The method as_triangulation_mut currently duplicates cache invalidation by manually clearing insertion_state.last_inserted_cell and spatial_index; replace that duplicated logic with a call to the existing helper invalidate_repair_caches() at the start of as_triangulation_mut so the same centralized invalidation behavior is used before returning &mut self.tri, ensuring no drift between implementations and keeping insertion_state.last_inserted_cell and spatial_index cleared consistently.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@src/triangulation/delaunay.rs`:
- Around line 4782-4786: The method as_triangulation_mut currently duplicates
cache invalidation by manually clearing insertion_state.last_inserted_cell and
spatial_index; replace that duplicated logic with a call to the existing helper
invalidate_repair_caches() at the start of as_triangulation_mut so the same
centralized invalidation behavior is used before returning &mut self.tri,
ensuring no drift between implementations and keeping
insertion_state.last_inserted_cell and spatial_index cleared consistently.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 0e60ce5e-7cbb-46b0-8e7c-1ad2e8e8d448
📒 Files selected for processing (2)
src/triangulation/delaunay.rstests/delaunay_public_api_coverage.rs
This was referenced Apr 29, 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 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.
Closes #332