refactor!: harden Delaunay invariants and tooling#362
Conversation
- Enforce checked Delaunay construction and reconstruction so explicit connectivity, deserialization, and TDS wrapping cannot produce invalid DelaunayTriangulation values. - Replace stringly insertion and construction failures with typed, matchable errors, and route topology/TDS failures without production panics. - Tighten public API names, trait bounds, prelude exports, docs, and doctests around mutation, reconstruction, construction statistics, and feature-gated behavior. - Standardize on DenseSlotMap storage, remove SlotMap backend comparison tooling, and align CI, docs.rs, changelog, benchmark, Semgrep, and release workflows. - Harden benchmark comparison parsing so individual regressions and malformed baseline sections fail explicitly. BREAKING CHANGE: Delaunay construction and reconstruction now reject unchecked or non-Delaunay topology that could previously be wrapped as DelaunayTriangulation. The public API also removes the optional SlotMap backend/comparison path, renames legacy get_* accessors to noun-style accessors, narrows focused prelude exports, and changes insertion/construction error matching to typed variants.
|
I can’t produce the required hidden review stack artifact because it must include every provided rangeId exactly once (the PR contains hundreds of rangeIds). Generating a fully valid artifact with all IDs here would exceed what I can reliably produce in this response. If you want, I can:
Which option do you prefer? ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 26 |
🟢 Coverage 76.06% diff coverage · -0.21% coverage variation
Metric Results Coverage variation ✅ -0.21% coverage variation (-1.00%) Diff coverage ✅ 76.06% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (af6e210) 51549 46329 89.87% Head commit (c8e4715) 52522 (+973) 47092 (+763) 89.66% (-0.21%) 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 (#362) 2235 1700 76.06% 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.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #362 +/- ##
==========================================
- Coverage 89.85% 89.63% -0.22%
==========================================
Files 58 58
Lines 51354 52331 +977
==========================================
+ Hits 46145 46908 +763
- Misses 5209 5423 +214
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
coderabbitai
Bot
added
bug
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
tests/proptest_facet.rs (1)
84-101:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAvoid silently skipping missing cell keys in this invariant test.
Since keys come from
tds.cell_keys(), a missingtds.cell(cell_key)should fail the property, not be skipped. This currently masks potential TDS key/index consistency bugs.Suggested tightening
- if let Some(cell) = tds.cell(cell_key) { - let cell_vertex_count = cell.vertices().len(); + let cell = tds + .cell(cell_key) + .expect("cell key from tds.cell_keys() must resolve"); + let cell_vertex_count = cell.vertices().len(); - for facet_index in 0..=($dim as u8) { - if let Ok(facet) = FacetView::new(&tds, cell_key, facet_index) { - if let Ok(facet_vertices) = facet.vertices() { - let facet_vertex_count = facet_vertices.count(); - prop_assert_eq!( - facet_vertex_count, - cell_vertex_count - 1, - "{}D facet should have one fewer vertex than cell", - $dim - ); - } - } + for facet_index in 0..=($dim as u8) { + if let Ok(facet) = FacetView::new(&tds, cell_key, facet_index) { + if let Ok(facet_vertices) = facet.vertices() { + let facet_vertex_count = facet_vertices.count(); + prop_assert_eq!( + facet_vertex_count, + cell_vertex_count - 1, + "{}D facet should have one fewer vertex than cell", + $dim + ); + } } } - }As per coding guidelines
tests/proptest_*.rs: “Proptests under tests/proptest_*.rs must cover algebraic and topological invariants …, not just 'does it not panic'”.🤖 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_facet.rs` around lines 84 - 101, The test currently silently skips when tds.cell(cell_key) is None; replace the conditional skip with an assertion so missing cells fail the property: assert (via prop_assert! or prop_assert_eq! with Option) that tds.cell(cell_key) is Some(...) for each cell_key from tds.cell_keys(), then unwrap the Option into cell and proceed — update the block around tds.cell(cell_key), cell_key, and subsequent use of cell, and keep FacetView::new(...), facet.vertices() and prop_assert_eq! checks unchanged except using the unwrapped cell.tests/proptest_triangulation.rs (1)
103-117:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winTreat missing cells as invariant failures, not skip conditions.
In this helper,
Nonefromtds_*.cell(cell_key)currently degrades into “no match” and can be discarded viaprop_assume!(matched_any). That masks key-to-cell consistency issues in topology-sensitive tests.As per coding guidelines
tests/proptest_*.rs: “Proptests under tests/proptest_*.rs must cover algebraic and topological invariants …, not just 'does it not panic'”.🤖 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_triangulation.rs` around lines 103 - 117, The code currently treats None from tds_orig.cell(orig_key) and tds_transformed.cell(trans_key) as “no match” and can silently skip checks; instead assert these presence checks so missing cells fail the property test. Replace the conditional patterns around tds_orig.cell(orig_key) and the tds_transformed.cell(trans_key) lookup with explicit assertions (e.g. prop_assert!(tds_orig.cell(orig_key).is_some(), "...") and prop_assert!(tds_transformed.cell(trans_key).is_some(), "...")) or use expect/unwrap in the helper so absent cells trigger a test failure; also remove the logic that uses matched_any/prop_assume! to mask missing-cell cases. Ensure you reference and update the code paths using orig_key, trans_key, tds_orig.cell, tds_transformed.cell, and matched_any accordingly.examples/pachner_roundtrip_4d.rs (1)
189-220:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace the remaining
expectwith the typed error path.
snapshot_topology()still panics on a missing vertex key, which bypasses the newPachnerRoundtripErrorflow and hides the actual roundtrip failure behind an example crash.Suggested fix
-fn snapshot_topology(dt: &Dt4) -> TopologySnapshot { +fn snapshot_topology(dt: &Dt4) -> Result<TopologySnapshot, PachnerRoundtripError> { let tds = dt.tds(); let mut vertex_uuids: Vec<Uuid> = tds.vertices().map(|(_, vertex)| vertex.uuid()).collect(); vertex_uuids.sort(); - let mut cell_vertex_uuids: Vec<Vec<Uuid>> = tds + let mut cell_vertex_uuids: Vec<Vec<Uuid>> = tds .cells() .map(|(_, cell)| { - let mut uuids: Vec<Uuid> = cell + let mut uuids: Vec<Uuid> = cell .vertices() .iter() - .map(|&vkey| tds.vertex(vkey).expect("vertex key missing in TDS").uuid()) + .map(|&vkey| { + tds.vertex(vkey) + .ok_or(PachnerRoundtripError::MissingVertex { vertex_key: vkey }) + .map(|vertex| vertex.uuid()) + }) + .collect::<Result<Vec<_>, _>>()?; - .collect(); uuids.sort(); - uuids + Ok(uuids) }) - .collect(); + .collect::<Result<Vec<_>, _>>()?; cell_vertex_uuids.sort(); - TopologySnapshot { + Ok(TopologySnapshot { vertex_uuids, cell_vertex_uuids, - } + }) } fn assert_roundtrip( move_kind: MoveKind, dt: &Dt4, baseline: &TopologySnapshot, ) -> Result<(), PachnerRoundtripError> { - let after = snapshot_topology(dt); + let after = snapshot_topology(dt)?;🤖 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 `@examples/pachner_roundtrip_4d.rs` around lines 189 - 220, snapshot_topology currently panics via expect when tds.vertex(vkey) is missing; change it to return Result<TopologySnapshot, PachnerRoundtripError> and propagate a typed error instead of panicking. Replace the .expect("vertex key missing in TDS") call in the cell_vertex_uuids construction with a fallible lookup (e.g. tds.vertex(vkey).ok_or_else(|| PachnerRoundtripError::MissingVertex { vkey, context: "snapshot_topology" }) or map_err) so the function returns an Err variant when a vertex is absent; then update callers (notably assert_roundtrip) to call snapshot_topology(dt)? (or handle the Result) and propagate PachnerRoundtripError accordingly so no panic remains and missing-vertex cases flow through the typed error path.src/core/vertex.rs (1)
741-760: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick win
is_valid(self)now moves non-Copy vertices — change to&self.Before this PR,
U: DataType(implyingU: Copy) madeVertex<T,U,D>: Copy, so the by-value receiver was a silent copy. RemovingU: DataTypefrom thewhereclause (line 743) makesis_validcallable onVertex<T, String, D>— but thatVertexis notCopy, so the call consumes it. This violates the "Borrow by default" coding guideline and is a practical footgun for any non-Copy metadata caller.The method body reads only
self.point.validate()andvalidate_uuid(&self.uuid()), both of which work on a shared reference.🔧 Proposed fix
- pub fn is_valid(self) -> Result<(), VertexValidationError> + pub fn is_valid(&self) -> Result<(), VertexValidationError> where T: CoordinateScalar, Point<T, D>: Coordinate<T, D>, { self.point .validate() .map_err(|source| VertexValidationError::InvalidPoint { source })?; - validate_uuid(&self.uuid())?; + validate_uuid(&self.uuid())?; Ok(()) }As per coding guidelines: "Borrow by default (use
&T,&mut T,&[T]); return borrowed views where possible."🤖 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/core/vertex.rs` around lines 741 - 760, The is_valid method currently takes self by value and therefore consumes non-Copy Vertex instances (e.g., Vertex<T,String,D>); change the signature of Vertex::is_valid to take &self (pub fn is_valid(&self) -> Result<(), VertexValidationError>) and update any internal uses to operate on &self (keeping self.point.validate() and validate_uuid(&self.uuid()) as-is since they work with references); ensure the where bounds on T/U/D remain compatible and run tests to catch any call sites that may need borrowing adjustments.src/triangulation/delaunay.rs (1)
1014-1023: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMark the new public wrapper as
#[must_use].Dropping this value silently loses both the construction failure and the partial statistics payload, and the repo guidelines call out public wrapper types specifically.
As per coding guidelines, `src/**/*.rs`: Error enums must be `#[non_exhaustive]`; public wrapper types must be `#[must_use]`.Suggested fix
+#[must_use = "inspect both the construction error and the partial construction statistics"] #[derive(Debug, Clone, thiserror::Error)] #[error("{error}")] #[non_exhaustive] pub struct DelaunayTriangulationConstructionErrorWithStatistics {🤖 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/triangulation/delaunay.rs` around lines 1014 - 1023, The public wrapper struct DelaunayTriangulationConstructionErrorWithStatistics should be annotated with #[must_use] so callers cannot accidentally drop it (losing the underlying DelaunayTriangulationConstructionError and the ConstructionStatistics); add the #[must_use] attribute directly above the struct declaration for DelaunayTriangulationConstructionErrorWithStatistics (keeping the existing #[derive(...)]/#[non_exhaustive] attributes intact).src/core/algorithms/incremental_insertion.rs (4)
1081-1087:⚠️ Potential issue | 🟠 Major | ⚡ Quick winKeep rollback-safe cavity failures retryable.
is_retryable()now makes everyCavityFillingerror terminal, but several of these reasons are still geometrically sensitive after the TDS snapshot has been rolled back for perturbation retry (InvalidFacetSharingAfterRepair,NeighborRebuild::NonManifoldTopology, geometry-only topology failures, etc.). That conflates the rollback-based perturbation retry with the in-place hull-extension fallback and will stop insertions that should still get a perturbation attempt.Based on learnings: perturbation retry rolls back to a snapshot before retry, so geometrically-sensitive insertion errors remain retryable there and should not be conflated with the in-place hull-extension fallback.
🤖 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/core/algorithms/incremental_insertion.rs` around lines 1081 - 1087, The current is_retryable() treats every Self::CavityFilling as terminal; instead, update is_retryable() in incremental_insertion.rs to inspect the CavityFilling inner reason and return true for rollback-safe, geometry-only failure reasons (e.g., InvalidFacetSharingAfterRepair, NeighborRebuild::NonManifoldTopology and other geometry-only topology failures) while still returning false for truly terminal cases; specifically, modify the match arm handling Self::CavityFilling { .. } to match on the contained enum/variant(s) and only mark those rollback-safe variants as retryable, leaving NeighborWiring, Location, DelaunayValidationFailed, DelaunayRepairFailed, DuplicateCoordinates, DuplicateUuid unchanged.
1913-1919:⚠️ Potential issue | 🟠 Major | ⚡ Quick winA missing internal cell should fail this helper immediately.
Silently skipping a stale
internal_cellsentry turns it into an incompleteexternal_facetsresult, so the subsequent cavity wiring can proceed with a hole in the boundary description. Since this helper already returnsResult, it should surfaceNeighborWiringError::MissingCellhere too.As per coding guidelines: Every fallible operation must return Result<_, _Error>; every mutating operation must preserve invariants checked by Tds::is_valid (Level 1–3).
🤖 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/core/algorithms/incremental_insertion.rs` around lines 1913 - 1919, The loop currently silently skips when tds.cell(cell_key) returns None, which yields incomplete external_facets; instead return an error from the helper: replace the `continue` after `let Some(cell) = tds.cell(cell_key) else { continue; };` with an immediate `Err(NeighborWiringError::MissingCell(cell_key))` (or construct the variant used in this module) so the helper propagates the failure; ensure the helper signature (the function that iterates internal_cells and calls tds.cell) returns Result and preserve invariants checked by Tds::is_valid when making this change.
1592-1601:⚠️ Potential issue | 🟠 Major | ⚡ Quick winDon’t silently drop external facets that fail to glue.
Both the “no matching new-cell facet” path and the “already shared by >1 new cell” path currently just continue. That lets
wire_cavity_neighbors()returnOk(())with missing links or an ignored 3-way share request, which is exactly the kind of local inconsistency this API is supposed to surface as a typed wiring failure.As per coding guidelines: Every mutating operation must preserve invariants checked by Tds::is_valid (Level 1–3), and every fallible operation must return Result<_, _Error>.
Also applies to: 1606-1616
🤖 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/core/algorithms/incremental_insertion.rs` around lines 1592 - 1601, The code currently silently continues when an external facet fails to glue (in the facet_map.get_mut(&facet_key) None branch and the similar >1-new-cell share branch), which can leave Tds invariants broken and lets wire_cavity_neighbors() return Ok(()); instead change both branches to return an Err (e.g., a WiringError variant) carrying contextual info (cell_key, external.facet_index(), facet_key and any existing unmatched list/count) so callers get a typed wiring failure; update the code paths in wire_cavity_neighbors(), the facet_map.get_mut(&facet_key) none branch and the >1-share branch (lines around the get_mut check and the 1606-1616 block) to emit that error instead of continue and ensure callers propagate the Result to preserve Tds::is_valid guarantees.
1540-1555:⚠️ Potential issue | 🔴 Critical | ⚡ Quick winReject malformed cell arity here instead of panicking later.
If a cell has more than
D + 1vertices,wire_cavity_neighbors()can pass one of those extra facet indices intoset_neighbor(), which then allocates onlyD + 1neighbor slots and indexes past the end. That turns malformed input into a panic instead of a typedNeighborWiringError.🐛 Proposed fix
fn set_neighbor<T, U, V, const D: usize>( tds: &mut Tds<T, U, V, D>, cell_key: CellKey, facet_idx: u8, neighbor: Option<CellKey>, ) -> Result<(), InsertionError> where T: CoordinateScalar, U: DataType, V: DataType, { let cell = tds .cell_mut(cell_key) .ok_or(NeighborWiringError::MissingCell { cell_key })?; + let vertex_count = cell.number_of_vertices(); + let facet_idx = usize::from(facet_idx); + if facet_idx >= vertex_count { + return Err(NeighborWiringError::InvalidFacetIndex { + cell_key, + facet_index: facet_idx, + vertex_count, + } + .into()); + } let mut neighbors = cell.neighbors().map_or_else( - || SmallBuffer::from_elem(None, D + 1), + || SmallBuffer::from_elem(None, vertex_count), |n| n.iter().copied().collect(), ); - neighbors[usize::from(facet_idx)] = neighbor; + neighbors[facet_idx] = neighbor; cell.neighbors = Some(neighbors); Ok(()) }As per coding guidelines: Every fallible operation must return Result<_, _Error>; panics reserved for documented, debug-only precondition violations; library code in src/ must not panic on user input.
Also applies to: 1976-1986
🤖 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/core/algorithms/incremental_insertion.rs` around lines 1540 - 1555, Reject malformed cells by validating cell.number_of_vertices() before the facet loop: if the arity exceeds the allowed D+1 (the practical neighbor slot count used by set_neighbor()/wire_cavity_neighbors()), return a Result::Err with NeighborWiringError::FacetIndexOverflow instead of continuing; add this check just before the for facet_idx in 0..cell.number_of_vertices() loop (use cell.number_of_vertices() - 1 for the facet_index in the error and u8::MAX or the appropriate max constant for max) so later calls to set_neighbor()/wire_cavity_neighbors() cannot index past the allocated neighbor slots.
🧹 Nitpick comments (3)
scripts/tag_release.py (1)
223-230: ⚡ Quick winRemove the now-unreachable
_delete_taghelper.After switching to
git tag -ffor transactional replacement (lines 397–403),_delete_tagis dead code with no remaining call sites. Leaving it in place risks a future maintainer reintroducing the delete-before-create pattern and inadvertently losing the transactional safety that the new approach provides.🗑️ Proposed fix
-def _delete_tag(tag_version: str) -> None: - """ - Delete the specified local Git tag from the repository. - - Parameters: - tag_version (str): Git tag name to remove (e.g., "v1.2.3"). - """ - run_git_command(["tag", "-d", tag_version]) - -🤖 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 `@scripts/tag_release.py` around lines 223 - 230, Remove the now-unused helper function _delete_tag: delete the entire _delete_tag(tag_version: str) function definition (including its docstring and the run_git_command call) so the codebase no longer contains the dead helper that encourages delete-before-create semantics; ensure there are no remaining references to _delete_tag elsewhere (rename or update any tests or callers if found) and rely on the existing transactional git tag replacement logic (git tag -f) instead.src/triangulation/delaunayize.rs (1)
547-554: 💤 Low valueOptional: defer the up-front fallback snapshot to the topology-fail path.
When
config.fallback_rebuildis set,snapshot_rebuild_stateis unconditionally collected on entry (lines 547–554), even though it's only consumed in the topology-failure arm. The Delaunay-failure path at lines 602–611 deliberately takes a fresh post-repair snapshot, so on the common success path the up-front snapshot is computed and then dropped.For the typical "repair completes cleanly" case this is wasted O(V + C) work plus a
FastHashMapallocation. Snapshotting only inside the topology-fail arm (and surfacing the snapshot error asFallbackCellDataSnapshotFailedfrom there) keeps the same error contract while avoiding the cost on success.Also applies to: 600-625
🤖 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/triangulation/delaunayize.rs` around lines 547 - 554, Currently snapshot_rebuild_state is called unconditionally when config.fallback_rebuild is true, doing wasted O(V+C) work on the common successful repair path; move the snapshot logic into the topology-fail branch so you only call snapshot_rebuild_state(tds) when you actually need the fallback rebuild state (i.e., inside the topology-failure arm that consumes fallback_state), remove the early fallback_state variable, and ensure any error from snapshot_rebuild_state is propagated as the existing FallbackCellDataSnapshotFailed error variant from that branch; leave the Delaunay-failure arm behavior (which takes a fresh post-repair snapshot) unchanged.src/core/triangulation.rs (1)
441-451: ⚡ Quick winAdd
#[must_use]to the public metrics snapshot.
DuplicateDetectionMetricsis a public wrapper-like return type, but it is not marked#[must_use]. That misses the repo’s API contract and makes it easier to ignore the snapshot unintentionally.♻️ Suggested change
+#[must_use] #[derive(Debug, Clone, Copy, Default)] pub struct DuplicateDetectionMetrics {As per coding guidelines "Error enums must be
#[non_exhaustive]; public wrapper types must be#[must_use]."🤖 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/core/triangulation.rs` around lines 441 - 451, Mark the public wrapper type DuplicateDetectionMetrics with the #[must_use] attribute so callers cannot silently ignore returned metric snapshots; add the attribute directly above the struct declaration for DuplicateDetectionMetrics in triangulation.rs (the struct with fields total_checks, grid_used, grid_fallbacks, grid_candidates) to enforce the repository API guideline for public wrapper return types.
🤖 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 `@scripts/archive_changelog.py`:
- Around line 76-89: The version-parsing code currently partitions the label
only on '-' so build metadata (the "+..." suffix) can leak into core/prerelease
parsing; update the logic that handles the input label (the variable label used
before core, separator, prerelease = label.partition("-")) to first strip off
any build metadata by splitting on '+' and keeping only the left side, then
proceed with partitioning on '-' and the existing numeric/prerelease handling
(the nums tuple generation and prerelease_key creation). Ensure tests that pass
labels like "1.2.3+build.7" and "1.2.3-rc.1+build.7" still sort correctly by
ignoring the "+build" part.
In `@scripts/benchmark_utils.py`:
- Around line 2123-2131: The CLI currently raises BaselineParseError from
scripts/benchmark_utils.py (where BaselineParseError is raised for malformed
baseline sections), but the compare-baselines/compare-tags code paths only catch
FileNotFoundError/RuntimeError; update the CLI to catch BaselineParseError and
convert it into a controlled error/exit by wrapping calls that parse baselines
(e.g., inside render_baseline_comparison or the functions that drive the
compare-baselines and compare-tags flows) so that any BaselineParseError is
caught, logged via the existing CLI error path, and causes a clean exit rather
than an uncaught traceback; locate references to BaselineParseError and
render_baseline_comparison to make this change.
In `@scripts/postprocess_changelog.py`:
- Around line 224-230: The breaking-change detection currently checks the
literal "[**breaking**]" in sline; update it to use the existing
_BREAKING_MARKER_RE so all marker variants (case/format differences) are
detected and normalized; i.e., set is_breaking =
bool(_BREAKING_MARKER_RE.search(sline)) and continue to call
_append_unique(breaking_entries, _compact_entry(sline, strip_breaking=True))
when matched (keeping the same calls to _append_unique and _compact_entry so
strip_breaking removes any matched marker).
In `@src/core/algorithms/locate.rs`:
- Line 140: Derived Eq on ConflictError is unstable because RidgeFan.extra_cells
and DisconnectedBoundary.disconnected_cells are Vec<CellKey> whose order depends
on FastHashMap iteration; at the sites where RidgeFan and DisconnectedBoundary
are constructed (when collecting from ridge_map.values() into extra_cells and
when collecting from boundary_facets / facet_to_conflict into
disconnected_cells), sort the Vec<CellKey> (e.g., call sort_unstable()) before
placing them into the variant so comparisons become deterministic (use the
existing CellKey Ord implementation).
In `@src/core/vertex.rs`:
- Around line 1007-1016: The comment in the vertex_string_data_usage_examples
test is stale: remove or update the lines claiming String wouldn't compile with
vertex! due to Copy/DataType constraints, since VertexBuilder and the Vertex
generic no longer require U: DataType and the new test
test_vertex_builder_accepts_non_data_type_metadata proves Vertex<f64, String, 2>
is valid; locate the vertex_string_data_usage_examples test and either delete
the misleading comment or replace it with a short note that String now works
with Vertex/vertex! (mentioning VertexBuilder and the vertex! macro) and remove
the incorrect reference to the DataType/Copy requirement.
In `@src/lib.rs`:
- Around line 663-668: LinearAlgebraFailure currently stores only a details:
String and loses the typed source when converting from
StackMatrixDispatchError::La; change the variant to preserve the underlying La
error so Error::source() works (e.g. replace LinearAlgebraFailure { details:
String } with a typed source like LinearAlgebraFailure(#[from] LaError) or
LinearAlgebraFailure { #[source] source: LaError }), and update the
From<StackMatrixDispatchError> impl to pass the La error through instead of
to_string() so the error chain is preserved symmetrically with MatrixError and
CoordinateConversionError.
In `@src/triangulation/builder.rs`:
- Around line 1380-1404: The current helper
validate_explicit_delaunay_property_if_euclidean lets explicit non-Euclidean
builds bypass validation by returning Ok() when
dt.global_topology().is_euclidean() is false; instead, change this to reject
non-Euclidean explicit builds until a Level 4 validator exists by returning an
error (e.g., an ExplicitConstructionError variant or
DelaunayTriangulationConstructionError) when
!dt.global_topology().is_euclidean(), with a clear message like "Explicit
non-Euclidean connectivity is not supported for construction; Level 4 validator
required", and keep the existing dt.is_valid() path for Euclidean topologies so
only Euclidean explicit connectivity can succeed.
In `@src/triangulation/delaunay.rs`:
- Around line 346-361: The construction error enum is missing a typed variant
for post-build topology checks, so final tri.validate() failures are being
categorized as GeometricDegeneracy; add a new variant FinalTopologyValidation to
the lower-layer payload enum (the same family as
InsertionTopologyValidation/Tds/InsufficientVertices) and update all
construction/finalization sites that call tri.validate() to return
DelaunayTriangulationConstructionError::Triangulation::FinalTopologyValidation
with the validation error payload instead of mapping them to
GeometricDegeneracy; ensure the enum docs/comments mention it mirrors the
incremental InsertionTopologyValidation and update any matches/exports that need
the new variant.
---
Outside diff comments:
In `@examples/pachner_roundtrip_4d.rs`:
- Around line 189-220: snapshot_topology currently panics via expect when
tds.vertex(vkey) is missing; change it to return Result<TopologySnapshot,
PachnerRoundtripError> and propagate a typed error instead of panicking. Replace
the .expect("vertex key missing in TDS") call in the cell_vertex_uuids
construction with a fallible lookup (e.g. tds.vertex(vkey).ok_or_else(||
PachnerRoundtripError::MissingVertex { vkey, context: "snapshot_topology" }) or
map_err) so the function returns an Err variant when a vertex is absent; then
update callers (notably assert_roundtrip) to call snapshot_topology(dt)? (or
handle the Result) and propagate PachnerRoundtripError accordingly so no panic
remains and missing-vertex cases flow through the typed error path.
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 1081-1087: The current is_retryable() treats every
Self::CavityFilling as terminal; instead, update is_retryable() in
incremental_insertion.rs to inspect the CavityFilling inner reason and return
true for rollback-safe, geometry-only failure reasons (e.g.,
InvalidFacetSharingAfterRepair, NeighborRebuild::NonManifoldTopology and other
geometry-only topology failures) while still returning false for truly terminal
cases; specifically, modify the match arm handling Self::CavityFilling { .. } to
match on the contained enum/variant(s) and only mark those rollback-safe
variants as retryable, leaving NeighborWiring, Location,
DelaunayValidationFailed, DelaunayRepairFailed, DuplicateCoordinates,
DuplicateUuid unchanged.
- Around line 1913-1919: The loop currently silently skips when
tds.cell(cell_key) returns None, which yields incomplete external_facets;
instead return an error from the helper: replace the `continue` after `let
Some(cell) = tds.cell(cell_key) else { continue; };` with an immediate
`Err(NeighborWiringError::MissingCell(cell_key))` (or construct the variant used
in this module) so the helper propagates the failure; ensure the helper
signature (the function that iterates internal_cells and calls tds.cell) returns
Result and preserve invariants checked by Tds::is_valid when making this change.
- Around line 1592-1601: The code currently silently continues when an external
facet fails to glue (in the facet_map.get_mut(&facet_key) None branch and the
similar >1-new-cell share branch), which can leave Tds invariants broken and
lets wire_cavity_neighbors() return Ok(()); instead change both branches to
return an Err (e.g., a WiringError variant) carrying contextual info (cell_key,
external.facet_index(), facet_key and any existing unmatched list/count) so
callers get a typed wiring failure; update the code paths in
wire_cavity_neighbors(), the facet_map.get_mut(&facet_key) none branch and the
>1-share branch (lines around the get_mut check and the 1606-1616 block) to emit
that error instead of continue and ensure callers propagate the Result to
preserve Tds::is_valid guarantees.
- Around line 1540-1555: Reject malformed cells by validating
cell.number_of_vertices() before the facet loop: if the arity exceeds the
allowed D+1 (the practical neighbor slot count used by
set_neighbor()/wire_cavity_neighbors()), return a Result::Err with
NeighborWiringError::FacetIndexOverflow instead of continuing; add this check
just before the for facet_idx in 0..cell.number_of_vertices() loop (use
cell.number_of_vertices() - 1 for the facet_index in the error and u8::MAX or
the appropriate max constant for max) so later calls to
set_neighbor()/wire_cavity_neighbors() cannot index past the allocated neighbor
slots.
In `@src/core/vertex.rs`:
- Around line 741-760: The is_valid method currently takes self by value and
therefore consumes non-Copy Vertex instances (e.g., Vertex<T,String,D>); change
the signature of Vertex::is_valid to take &self (pub fn is_valid(&self) ->
Result<(), VertexValidationError>) and update any internal uses to operate on
&self (keeping self.point.validate() and validate_uuid(&self.uuid()) as-is since
they work with references); ensure the where bounds on T/U/D remain compatible
and run tests to catch any call sites that may need borrowing adjustments.
In `@src/triangulation/delaunay.rs`:
- Around line 1014-1023: The public wrapper struct
DelaunayTriangulationConstructionErrorWithStatistics should be annotated with
#[must_use] so callers cannot accidentally drop it (losing the underlying
DelaunayTriangulationConstructionError and the ConstructionStatistics); add the
#[must_use] attribute directly above the struct declaration for
DelaunayTriangulationConstructionErrorWithStatistics (keeping the existing
#[derive(...)]/#[non_exhaustive] attributes intact).
In `@tests/proptest_facet.rs`:
- Around line 84-101: The test currently silently skips when tds.cell(cell_key)
is None; replace the conditional skip with an assertion so missing cells fail
the property: assert (via prop_assert! or prop_assert_eq! with Option) that
tds.cell(cell_key) is Some(...) for each cell_key from tds.cell_keys(), then
unwrap the Option into cell and proceed — update the block around
tds.cell(cell_key), cell_key, and subsequent use of cell, and keep
FacetView::new(...), facet.vertices() and prop_assert_eq! checks unchanged
except using the unwrapped cell.
In `@tests/proptest_triangulation.rs`:
- Around line 103-117: The code currently treats None from
tds_orig.cell(orig_key) and tds_transformed.cell(trans_key) as “no match” and
can silently skip checks; instead assert these presence checks so missing cells
fail the property test. Replace the conditional patterns around
tds_orig.cell(orig_key) and the tds_transformed.cell(trans_key) lookup with
explicit assertions (e.g. prop_assert!(tds_orig.cell(orig_key).is_some(), "...")
and prop_assert!(tds_transformed.cell(trans_key).is_some(), "...")) or use
expect/unwrap in the helper so absent cells trigger a test failure; also remove
the logic that uses matched_any/prop_assume! to mask missing-cell cases. Ensure
you reference and update the code paths using orig_key, trans_key,
tds_orig.cell, tds_transformed.cell, and matched_any accordingly.
---
Nitpick comments:
In `@scripts/tag_release.py`:
- Around line 223-230: Remove the now-unused helper function _delete_tag: delete
the entire _delete_tag(tag_version: str) function definition (including its
docstring and the run_git_command call) so the codebase no longer contains the
dead helper that encourages delete-before-create semantics; ensure there are no
remaining references to _delete_tag elsewhere (rename or update any tests or
callers if found) and rely on the existing transactional git tag replacement
logic (git tag -f) instead.
In `@src/core/triangulation.rs`:
- Around line 441-451: Mark the public wrapper type DuplicateDetectionMetrics
with the #[must_use] attribute so callers cannot silently ignore returned metric
snapshots; add the attribute directly above the struct declaration for
DuplicateDetectionMetrics in triangulation.rs (the struct with fields
total_checks, grid_used, grid_fallbacks, grid_candidates) to enforce the
repository API guideline for public wrapper return types.
In `@src/triangulation/delaunayize.rs`:
- Around line 547-554: Currently snapshot_rebuild_state is called
unconditionally when config.fallback_rebuild is true, doing wasted O(V+C) work
on the common successful repair path; move the snapshot logic into the
topology-fail branch so you only call snapshot_rebuild_state(tds) when you
actually need the fallback rebuild state (i.e., inside the topology-failure arm
that consumes fallback_state), remove the early fallback_state variable, and
ensure any error from snapshot_rebuild_state is propagated as the existing
FallbackCellDataSnapshotFailed error variant from that branch; leave the
Delaunay-failure arm behavior (which takes a fresh post-repair snapshot)
unchanged.
🪄 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: 0f4a8c2c-953b-415c-a6be-212cbcff4688
📒 Files selected for processing (80)
AGENTS.mdCONTRIBUTING.mdCargo.tomlbenches/README.mdbenches/ci_performance_suite.rsbenches/large_scale_performance.rsbenches/profiling_suite.rscliff.tomldocs/ORIENTATION_SPEC.mddocs/RELEASING.mddocs/code_organization.mddocs/dev/commands.mddocs/dev/tooling-alignment.mddocs/templates/README.mddocs/workflows.mdexamples/pachner_roundtrip_4d.rsexamples/topology_editing_2d_3d.rsexamples/triangulation_3d_100_points.rsexamples/zero_allocation_iterator_demo.rsjustfilepyproject.tomlscripts/README.mdscripts/archive_changelog.pyscripts/benchmark_utils.pyscripts/compare_storage_backends.pyscripts/postprocess_changelog.pyscripts/slurm_storage_comparison.shscripts/tag_release.pyscripts/tests/test_archive_changelog.pyscripts/tests/test_benchmark_utils.pyscripts/tests/test_compare_storage_backends.pyscripts/tests/test_postprocess_changelog.pyscripts/tests/test_tag_release.pysemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/cell.rssrc/core/collections/aliases.rssrc/core/collections/key_maps.rssrc/core/facet.rssrc/core/tds.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/delaunay_validation.rssrc/core/util/jaccard.rssrc/core/util/measurement.rssrc/core/util/uuid.rssrc/core/vertex.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/matrix.rssrc/geometry/point.rssrc/geometry/quality.rssrc/geometry/robust_predicates.rssrc/geometry/traits/coordinate.rssrc/geometry/util/measures.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/manifold.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rssrc/triangulation/flips.rstests/README.mdtests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/delaunay_public_api_coverage.rstests/large_scale_debug.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/proptest_facet.rstests/proptest_serialization.rstests/proptest_tds.rstests/proptest_triangulation.rstests/regressions.rstests/semgrep/scripts/tests/python_exceptions.pytests/storage_backend_compatibility.rstests/triangulation_builder.rs
💤 Files with no reviewable changes (7)
- tests/README.md
- pyproject.toml
- src/geometry/util/triangulation_generation.rs
- tests/storage_backend_compatibility.rs
- scripts/tests/test_compare_storage_backends.py
- scripts/slurm_storage_comparison.sh
- scripts/compare_storage_backends.py
- Preserve typed linear algebra and final topology validation sources in public construction errors. - Reject explicit non-Euclidean connectivity until Level 4 quotient validation is available. - Surface missing vertices, malformed facets, stale cells, and cavity wiring failures as typed errors. - Harden changelog and benchmark tooling around build metadata, breaking markers, and malformed baselines. BREAKING CHANGE: Explicit non-Euclidean connectivity through DelaunayTriangulationBuilder::from_vertices_and_cells now returns an explicit construction validation error until Level 4 quotient validation is supported. LinearAlgebraFailure error variants now carry typed la_stack::LaError sources instead of details strings.
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
src/core/vertex.rs (1)
1856-1941:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winRemove contradictory “String doesn’t work” notes in the same test block.
This test now correctly states String metadata works (Line 1856), but the summary at the end still says String doesn’t work due to
Copy. That contradiction is misleading and should be updated/removed.Suggested comment fix
- // 1. String doesn't work because it doesn't implement Copy - // 2. &str has complex lifetime issues that make it impractical - // 3. &'static str could work but only for compile-time constants - // 4. Recommended alternatives: + // 1. String metadata works for standalone vertices/builders. + // 2. If vertices must satisfy DataType-dependent traits (e.g. serde/hash in this crate), + // prefer compact IDs or enums when that better matches constraints/perf goals. + // 3. Recommended alternatives:🤖 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/core/vertex.rs` around lines 1856 - 1941, Update the summary block at the end of the test to remove the contradictory statement that "String doesn't work" and instead reflect the current behavior shown above: mention that String metadata is supported with Vertex/VertexBuilder/vertex! (but is not Copy), or simply delete bullet 1; edit the summary text that references Copy to match the demonstrated examples (symbols to locate: Vertex, VertexBuilder, vertex! macro, and the final "SUMMARY OF STRING DATA LIMITATIONS" comment).
🧹 Nitpick comments (2)
src/core/algorithms/incremental_insertion.rs (2)
1597-1608: ⚡ Quick win
FacetIndexOverflowis being used to report wrong cell arity, with amaxvalue that contradicts the variant's documented semantics.
NeighborWiringError::FacetIndexOverflowis documented as “A facet index cannot fit in the compact facet-index storage type” withmax: u8being “Maximum storable facet index”. Every other call site is consistent with that contract:
- Line 1622-1625:
max: u8::MAX- Line 2005-2008:
max: u8::MAX- Line 2129-2132:
max: u8::MAXThis early check, however, fires when
vertex_count > D + 1(a malformed cell, analogous toCavityFillingError::WrongCellArity) and reportsmax: u8::try_from(D).unwrap_or(u8::MAX)— i.e.D, the dimension's largest valid facet index, not the compact-storage upper bound. The resultingDisplayreads"facet index N exceeds compact facet-index maximum 3"forD = 3, which conflates two distinct invariants and is genuinely misleading for any consumer pattern-matching on this error.Two reasonable directions; either is fine, but the variant should not silently mean two different things:
♻️ Option A — add a dedicated arity variant (preferred, mirrors `CavityFillingError::WrongCellArity`)
pub enum NeighborWiringError { /// A cell required for neighbor wiring was not found. #[error("cell {cell_key:?} not found during neighbor wiring")] MissingCell { /// Missing cell key. cell_key: CellKey, }, + /// A cell has the wrong number of vertices for the triangulation dimension. + #[error("cell {cell_key:?} has {actual} vertices, expected {expected}")] + WrongCellArity { + /// Cell with the wrong arity. + cell_key: CellKey, + /// Observed vertex count. + actual: usize, + /// Expected vertex count (D + 1). + expected: usize, + }, + /// A facet index is outside the referenced cell's vertex range.- let vertex_count = cell.number_of_vertices(); - if vertex_count > D + 1 { - return Err(NeighborWiringError::FacetIndexOverflow { - facet_index: vertex_count - 1, - max: u8::try_from(D).unwrap_or(u8::MAX), - } - .into()); - } + let vertex_count = cell.number_of_vertices(); + if vertex_count != D + 1 { + return Err(NeighborWiringError::WrongCellArity { + cell_key, + actual: vertex_count, + expected: D + 1, + } + .into()); + }This also tightens the check to
!=, which catches under-arity cells that the current>predicate silently iterates with an incomplete facet count.♻️ Option B — keep `FacetIndexOverflow` and just match the documented semantics
- if vertex_count > D + 1 { - return Err(NeighborWiringError::FacetIndexOverflow { - facet_index: vertex_count - 1, - max: u8::try_from(D).unwrap_or(u8::MAX), - } - .into()); - } + if vertex_count > D + 1 { + return Err(NeighborWiringError::FacetIndexOverflow { + facet_index: vertex_count - 1, + max: u8::MAX, + } + .into()); + }This still leaves the variant doing double duty (true u8 overflow vs. dimension overrun), so Option A is preferred.
🤖 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/core/algorithms/incremental_insertion.rs` around lines 1597 - 1608, The code currently uses NeighborWiringError::FacetIndexOverflow to report a wrong cell arity; instead add and use a dedicated variant (e.g. NeighborWiringError::WrongCellArity { cell_key, expected: usize, found: usize }) and change the check in the loop that inspects cell.number_of_vertices() to test for != (not >) against expected = D + 1; when mismatched return Err(NeighborWiringError::WrongCellArity { cell_key, expected: D + 1, found: vertex_count }.into()). Update the NeighborWiringError enum to include the new WrongCellArity variant and adjust any display/conversion impls accordingly so the error unambiguously reports expected vs found arity.
1146-1172: ⚡ Quick winConfirm the asymmetric retryability of
CellInsertion/InitialSimplexConstructionvs.NeighborRebuild::TopologyValidation.
NeighborRebuild::TopologyValidation { reason }correctly delegates tois_tds_validation_failure_retryable, so a wrappedTdsValidationFailure::Geometric { .. }orOrientationViolation { .. }is retryable. TheCellInsertionarm, however, is unconditionallyfalse, even though itsTdsConstructionFailure::Validation { reason }payload can carry exactly the same geometry-related variants (the validation step insideinsert_cell_with_mappingis a common place to surface degenerate orientation / coherent-orientation failures during cavity filling).
InitialSimplexConstruction { reason }is similarly hard-coded tofalse, even thoughInitialSimplexConstructionError::TdsValidation { source: TdsValidationFailure }andInitialSimplexConstructionError::GeometricDegeneracy { .. }are also geometric in nature.Per the repo's perturbation-retry contract — the TDS is rolled back to a snapshot before retrying, so geometrically-sensitive errors are valid to classify as retryable — this looks inconsistent with the
NeighborRebuildpath. Two possibilities:
- This is a deliberate, conservative classification (e.g. you've observed that perturbation does not, in practice, recover from these particular failure points). If so, it would help to call that out in the doc comment for
is_cavity_filling_error_retryableand on the affected variants, otherwise future maintainers will likely "fix" the asymmetry.- It's an oversight, in which case routing through the existing helpers fixes it cleanly:
♻️ Sketch if the asymmetry is unintentional
const fn is_cavity_filling_error_retryable(err: &CavityFillingError) -> bool { match err { CavityFillingError::InvalidFacetSharingAfterRepair { .. } => true, CavityFillingError::NeighborRebuild { reason } => match reason { NeighborRebuildError::NonManifoldTopology { .. } => true, NeighborRebuildError::TopologyValidation { reason } => { Self::is_tds_validation_failure_retryable(reason) } NeighborRebuildError::Wiring { .. } | NeighborRebuildError::Unexpected { .. } => { false } }, + CavityFillingError::CellInsertion { reason } => match reason { + TdsConstructionFailure::Validation { reason } => { + Self::is_tds_validation_failure_retryable(reason) + } + TdsConstructionFailure::DuplicateUuid { .. } => false, + }, + CavityFillingError::InitialSimplexConstruction { reason } => matches!( + reason, + InitialSimplexConstructionError::TdsValidation { source } + if Self::is_tds_validation_failure_retryable(source) + ) || matches!( + reason, + InitialSimplexConstructionError::GeometricDegeneracy { .. } + ), CavityFillingError::MissingBoundaryCell { .. } | CavityFillingError::MissingInsertedVertex { .. } | CavityFillingError::WrongCellArity { .. } | CavityFillingError::InvalidFacetIndex { .. } | CavityFillingError::CellCreation { .. } - | CavityFillingError::CellInsertion { .. } - | CavityFillingError::InitialSimplexConstruction { .. } | CavityFillingError::RebuiltVertexMissing { .. } | CavityFillingError::EmptyConflictRegion { .. } | CavityFillingError::EmptyBoundary { .. } | CavityFillingError::PerturbationScaleConversion { .. } | CavityFillingError::UnsupportedDegenerateLocation { .. } | CavityFillingError::EmptyFanTriangulation => false, } }Based on learnings (PR 261 perturbation-retry contract: any geometrically-sensitive error is valid to classify as retryable when the TDS is rolled back to a snapshot before retrying).
🤖 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/core/algorithms/incremental_insertion.rs` around lines 1146 - 1172, The match in is_cavity_filling_error_retryable currently returns false for CellInsertion and InitialSimplexConstruction unconditionally; either document that deliberate choice or make them symmetric with NeighborRebuild::TopologyValidation by delegating geometry-related payloads to the existing helper: for CellInsertion inspect the TdsConstructionFailure::Validation { reason } branch and call is_tds_validation_failure_retryable(reason), and for InitialSimplexConstruction handle InitialSimplexConstructionError::TdsValidation { source } (and any geometric degeneracy variant) by routing to is_tds_validation_failure_retryable or treating geometric degeneracy as retryable; update the match arms in is_cavity_filling_error_retryable (and the function doc comment if you keep the conservative behavior) so retryability is consistent and obvious.
🤖 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 `@scripts/archive_changelog.py`:
- Around line 342-345: The fallback currently sets archive_dir_rel =
archive_dir.as_posix() which yields an absolute path and produces invalid
Markdown links; change the except ValueError branch to compute a sensible
relative path using os.path.relpath (relative to changelog_path.parent) and, if
that still results in an absolute path or goes outside the tree, emit a warning
so the user knows --archive-dir is outside the changelog tree; update the except
handling around archive_dir.relative_to(changelog_path.parent) to call
os.path.relpath(archive_dir, changelog_path.parent) and log a warning via the
module logger (or print) referencing archive_dir and changelog_path.parent so
build_root/Markdown links are not silently broken.
In `@src/triangulation/delaunay.rs`:
- Around line 6962-6963: The docs for try_from_tds on DelaunayTriangulation
incorrectly state a “default kernel” while the function signature requires an
explicit kernel: K; update the doc comment for try_from_tds (and the duplicate
docs around the other occurrence) to describe that the caller must provide a
kernel parameter (kernel: K) and explain its role, or alternatively change the
signature to accept a defaulted/implicit kernel if that behavior is
intended—refer to the try_from_tds function and the DelaunayTriangulation
constructor to locate and reconcile the wording with the actual API.
---
Duplicate comments:
In `@src/core/vertex.rs`:
- Around line 1856-1941: Update the summary block at the end of the test to
remove the contradictory statement that "String doesn't work" and instead
reflect the current behavior shown above: mention that String metadata is
supported with Vertex/VertexBuilder/vertex! (but is not Copy), or simply delete
bullet 1; edit the summary text that references Copy to match the demonstrated
examples (symbols to locate: Vertex, VertexBuilder, vertex! macro, and the final
"SUMMARY OF STRING DATA LIMITATIONS" comment).
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 1597-1608: The code currently uses
NeighborWiringError::FacetIndexOverflow to report a wrong cell arity; instead
add and use a dedicated variant (e.g. NeighborWiringError::WrongCellArity {
cell_key, expected: usize, found: usize }) and change the check in the loop that
inspects cell.number_of_vertices() to test for != (not >) against expected = D +
1; when mismatched return Err(NeighborWiringError::WrongCellArity { cell_key,
expected: D + 1, found: vertex_count }.into()). Update the NeighborWiringError
enum to include the new WrongCellArity variant and adjust any display/conversion
impls accordingly so the error unambiguously reports expected vs found arity.
- Around line 1146-1172: The match in is_cavity_filling_error_retryable
currently returns false for CellInsertion and InitialSimplexConstruction
unconditionally; either document that deliberate choice or make them symmetric
with NeighborRebuild::TopologyValidation by delegating geometry-related payloads
to the existing helper: for CellInsertion inspect the
TdsConstructionFailure::Validation { reason } branch and call
is_tds_validation_failure_retryable(reason), and for InitialSimplexConstruction
handle InitialSimplexConstructionError::TdsValidation { source } (and any
geometric degeneracy variant) by routing to is_tds_validation_failure_retryable
or treating geometric degeneracy as retryable; update the match arms in
is_cavity_filling_error_retryable (and the function doc comment if you keep the
conservative behavior) so retryability is consistent and obvious.
🪄 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: adfd1e67-2248-4e3e-a33d-8a498abe82f3
📒 Files selected for processing (22)
examples/pachner_roundtrip_4d.rsscripts/archive_changelog.pyscripts/benchmark_utils.pyscripts/postprocess_changelog.pyscripts/tag_release.pyscripts/tests/test_archive_changelog.pyscripts/tests/test_postprocess_changelog.pyscripts/tests/test_tag_release.pysrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/triangulation.rssrc/core/vertex.rssrc/geometry/point.rssrc/geometry/traits/coordinate.rssrc/lib.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rstests/euler_characteristic.rstests/proptest_facet.rstests/proptest_triangulation.rstests/triangulation_builder.rs
✅ Files skipped from review due to trivial changes (1)
- scripts/tests/test_postprocess_changelog.py
🚧 Files skipped from review as they are similar to previous changes (11)
- scripts/tests/test_tag_release.py
- src/geometry/point.rs
- tests/proptest_triangulation.rs
- src/triangulation/builder.rs
- src/geometry/traits/coordinate.rs
- examples/pachner_roundtrip_4d.rs
- scripts/benchmark_utils.py
- src/core/algorithms/locate.rs
- src/triangulation/delaunayize.rs
- src/core/triangulation.rs
- src/lib.rs
- Add opt-in diagnostics helpers with structured Delaunay violation reports, documentation, and runnable examples. - Replace stringly flip, insertion, repair, and validation failures with typed error variants that preserve predicate, topology, and neighbor-wiring context. - Harden cavity retryability, Delaunay validation diagnostics, periodic canonical vertex mapping, and TDS equality so invalid states fail explicitly. - Enforce non-exhaustive public error enums and document the remaining public sample cleanup needed for `.expect(...)` usage. - Fix changelog archive links when `--archive-dir` is outside the changelog tree. BREAKING CHANGE: Public flip and Delaunay validation error shapes now expose structured variants and boxed repair sources instead of prior string-context fields, so exhaustive matches and direct field access must be updated.
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (7)
examples/zero_allocation_iterator_demo.rs (2)
42-43:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winReplace
eprintln!withprintln!for committed user-facing output.Per the coding guidelines,
eprintln!is acceptable only for short-lived local debugging and must be removed before landing changes. The early-exit message on Line 42 is a committed user-facing diagnostic path in an example, so it belongs on stdout.📝 Proposed fix
- eprintln!("No cells in triangulation; nothing to demo."); + println!("No cells in triangulation; nothing to demo.");As per coding guidelines: "
eprintln!is acceptable only for short-lived local debugging; remove it before landing changes."🤖 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 `@examples/zero_allocation_iterator_demo.rs` around lines 42 - 43, The early-exit diagnostic in the example uses eprintln!("No cells in triangulation; nothing to demo."); which should be user-facing stdout; change that call to println! instead (in the example's main/early-exit branch where the message and subsequent return Ok(()) appear) so the message is printed to stdout per the coding guidelines.
80-98:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winMethod 1 and Method 2 count different quantities, making the
total_count == total_count_itercheck misleading.
total_count(Method 1, Lines 80–87): accumulated fromuuids.len(), which is the number of successfully resolved UUIDs (only reached onOk).total_count_iter(Method 2, Lines 92–98): accumulated from.count()onIterator<Item = Result<Uuid, _>>, which counts all items — bothOkandErrvariants — regardless of validity.For a well-formed triangulation the counts happen to agree, but the comment on Line 94 already acknowledges this discrepancy. If any item yields
Err, Line 111's "Counts match" will silently printfalsewithout surfacing the error, defeating the purpose of the typed-error refactor.Consider aligning Method 2 to count only successfully resolved UUIDs, consistent with Method 1:
📝 Proposed fix
for _ in 0..iterations { - // Note: count() on Iterator<Item = Result<Uuid, _>> counts all items (both Ok and Err). - // This is appropriate for performance comparison since errors should not occur here. - let count = cell.vertex_uuid_iter(dt.tds()).count(); // No allocation + let count = cell + .vertex_uuid_iter(dt.tds()) + .filter(|r| r.is_ok()) + .count(); // No allocation total_count_iter += black_box(count); }🤖 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 `@examples/zero_allocation_iterator_demo.rs` around lines 80 - 98, Method 2 is counting all Result items while Method 1 counts only successfully resolved UUIDs, so change the zero-allocation iterator usage in the benchmark to count only Ok values to match Method 1: locate cell.vertex_uuid_iter(dt.tds()) and replace the plain .count() with a filtered/counting approach that only considers successes (e.g., filter_map(Result::ok).count() or equivalent), and keep the totals in total_count_iter so total_count == total_count_iter reflects the same quantity.src/core/tds.rs (2)
2222-2255: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftDo not expose unchecked topology mutation through the public API.
cell_mut()andvertex_mut()hand out raw&mut Cell/&mut Vertex, so callers can change neighbor wiring, vertex buffers,incident_cell, or periodic offsets without any typed validation and without going through generation-aware mutation APIs. That reopens the invariant hole this PR is otherwise tightening.As per coding guidelines,
Every mutating operation must preserve invariants checked by Tds::is_valid (Level 1–3) and DelaunayTriangulation::is_valid (Level 4); operations must fail explicitly rather than leave the triangulation inconsistent.Also applies to: 2292-2320
🤖 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/core/tds.rs` around lines 2222 - 2255, cell_mut() (and vertex_mut()) currently expose raw &mut Cell/&mut Vertex allowing unchecked topology or offset mutations that can break Tds/Delaunay invariants; make these methods non-public (or remove direct &mut exposure) and replace them with a safe mutation API or a guarded proxy type that only allows content updates (not neighbor wiring, incident_cell, periodic offsets) and routes topology changes through existing generation-aware setters like set_neighbors_by_key(), ensuring every mutating operation preserves invariants checked by Tds::is_valid/DelaunayTriangulation::is_valid; update the matching vertex_mut implementation similarly and add tests to assert invalid topology cannot be created via the public API.
2999-3029:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftReturn a typed missing-cell error here instead of a synthetic boundary buffer.
When
cell_keyis stale, this API currently returns an all-Noneneighbor buffer, which is indistinguishable from a real boundary-only cell. That hides the root cause and can push callers down the wrong topological path. Since this PR is already making breaking API changes, this is a good place to switch toResult<NeighborBuffer<_>, TdsError>.Suggested direction
-pub fn find_neighbors_by_key(&self, cell_key: CellKey) -> NeighborBuffer<Option<CellKey>> { +pub fn find_neighbors_by_key( + &self, + cell_key: CellKey, +) -> Result<NeighborBuffer<Option<CellKey>>, TdsError> { let mut neighbors = NeighborBuffer::new(); neighbors.resize(D + 1, None); - let Some(cell) = self.cell(cell_key) else { - return neighbors; - }; + let cell = self.cell(cell_key).ok_or_else(|| TdsError::CellNotFound { + cell_key, + context: "find_neighbors_by_key".to_string(), + })?; if let Some(ref neighbors_from_cell) = cell.neighbors { for (slot, neighbor_key_opt) in neighbors.iter_mut().zip(neighbors_from_cell.iter()) { *slot = *neighbor_key_opt; } } - neighbors + Ok(neighbors) }As per coding guidelines,
Result<_, _Error> for every fallible operation. Panics are reserved for documented, debug-only precondition violations; library code in src/ must not panic on user input.src/core/algorithms/flips.rs (1)
441-457:⚠️ Potential issue | 🔴 Critical | 🏗️ Heavy liftMake flip application transactional across insertion/wiring failures.
If
insert_cell_with_mappingon Line 443 orwire_cavity_neighborson Line 451 fails, this function returnsErrafter mutating the liveTds: some replacement cells may already exist, external neighbors may already be partially rewired, andremoved_cellsare still present. That leaves callers with an inconsistent triangulation on the error path, which is exactly the state this PR is trying to rule out.Please stage the mutation transactionally: either compute/apply wiring against a temporary structure first, or add a rollback guard that restores any inserted cells and touched neighbor slots before returning an error. This also affects the k=1 wrapper, because removing the just-inserted vertex does not undo partial cell mutations from this helper.
As per coding guidelines
Every mutating operation must preserve invariants checked by Tds::is_valid (Level 1–3) and DelaunayTriangulation::is_valid (Level 4); operations must fail explicitly rather than leave the triangulation inconsistent.🤖 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/core/algorithms/flips.rs` around lines 441 - 457, The flip routine currently mutates the live Tds via insert_cell_with_mapping and wire_cavity_neighbors and can return Err leaving the Tds inconsistent; change it to perform the mutation transactionally by either applying wiring to a temporary Tds copy or installing a rollback guard that records inserted cell keys (new_cells), any modified neighbor slots (from wire_cavity_neighbors) and the removed_cells, and on any error (including FlipMutationError::CellInsertion or FlipNeighborWiringError) restores the Tds to its prior state by removing inserted cells, reinstating removed_cells, and rewiring neighbor pointers back to their saved values before returning the error; ensure this is used by the k=1 wrapper as well and that invariants verified by Tds::is_valid remain preserved on both success and failure.src/triangulation/delaunay.rs (2)
6991-7013:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftChecked
Tdsreconstruction is only as strong as the caller’s kernel.
try_from_tds*validates viacandidate.validate(), which means the Level-4 acceptance check runs on the suppliedK. The examples here passFastKernel, so near-degenerate or co-sphericalTdsstates can still be accepted or rejected incorrectly despite the new “checked reconstruction” contract. If reconstruction is meant to harden invariants, it needs a robust/exact validation pass that is independent of the runtime kernel, or the API/docs need to say that the guarantee is only approximate underFastKernel.Based on learnings "Performance is subordinate to correctness and stability; never trade correctness, stability, or clarity for speed."
Also applies to: 7039-7057, 7090-7135
🤖 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/triangulation/delaunay.rs` around lines 6991 - 7013, The Tds reconstruction currently relies on candidate.validate() (used by DelaunayTriangulation::try_from_tds) which runs the Level-4 acceptance checks using the caller-supplied kernel (e.g., FastKernel), so near-degenerate or co-spherical states may be mis-classified; change try_from_tds to perform a kernel-independent/exact validation pass (or call a new exact validator) when reconstructing from serialized Tds to ensure invariants are enforced regardless of runtime K, or clearly document in the API/docs that try_from_tds only provides best-effort validation when given a non-exact kernel like FastKernel and may accept borderline cases—update references to candidate.validate(), try_from_tds, and the examples to reflect the chosen approach.
2127-2137:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy lift
ConstructionStatisticsdrops work from earlier retry paths.These constructors now funnel through
with_options_and_statistics, but that path still returns only the last attempt’s summary. If balanced-simplex fallback or shuffled reconstruction retries run first,total_attempts,inserted,skipped_*, andskip_samplesno longer describe the full batch construction that just happened. Either merge attempt-level stats across retries/fallbacks or narrow the contract/docs to “final attempt only” before exposing this as the public aggregate-stats API.Also applies to: 2158-2164, 2695-2700
🤖 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/triangulation/delaunay.rs` around lines 2127 - 2137, The reported bug is that new_with_construction_statistics (and the other constructors funneling through with_options_and_statistics) only return the last attempt’s ConstructionStatistics, losing counts from earlier retries/fallbacks; update with_options_and_statistics to accumulate/merge attempt-level stats across all reconstruction attempts and fallback paths (summing total_attempts, inserted, skipped_* and skip_samples or other relevant counters) and return this aggregated ConstructionStatistics to callers like new_with_construction_statistics (and the other affected constructors around the noted ranges), or alternatively change the public contract to explicitly state it returns only the final-attempt stats — implement the aggregation approach by collecting each attempt’s ConstructionStatistics inside with_options_and_statistics and combining them before returning.
🧹 Nitpick comments (5)
examples/zero_allocation_iterator_demo.rs (1)
16-22: 💤 Low value
DemoErroris missing#[non_exhaustive].The coding guidelines require all error enums to carry
#[non_exhaustive]. WhileDemoErroris example-local and never part of a public crate API, applying the rule consistently avoids a pattern mismatch that could leak into library code during copy-paste.📝 Proposed fix
#[derive(Debug, thiserror::Error)] +#[non_exhaustive] enum DemoError {As per coding guidelines: "Error enums must be
#[non_exhaustive]."🤖 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 `@examples/zero_allocation_iterator_demo.rs` around lines 16 - 22, Add the #[non_exhaustive] attribute to the DemoError enum declaration to follow the coding guideline for error enums; locate the enum named DemoError (the #[derive(Debug, thiserror::Error)] enum DemoError { ... }) and prepend #[non_exhaustive] above it so the definition becomes non-exhaustive without changing its variants or derives.docs/dev/tooling-alignment.md (1)
86-90: ⚡ Quick winConsider programmatic verification of baseline counts.
The specific counts listed here will become stale as cleanup progresses. Consider adding a script or
justtarget that programmatically verifies these numbers to catch drift.Example verification approach
Add a
justrecipe that can verify or update these counts:#!/bin/bash # Count .expect(...) in examples echo "examples/**/*.rs: $(rg -t rust '\.expect\(' examples/ | wc -l) .expect(...) calls." # Count .expect(...) in benches echo "benches/**/*.rs: $(rg -t rust '\.expect\(' benches/ | wc -l) .expect(...) calls." # Count .expect(...) in src doc comments echo "src/**/*.rs docs: $(rg -t rust '^ *//[!/].*\.expect\(' src/ | wc -l) .expect(...) calls."This helps track progress and ensures the documentation stays current.
🤖 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 `@docs/dev/tooling-alignment.md` around lines 86 - 90, Add a small verification script / just recipe (e.g., verify-expect-counts) that programmatically counts occurrences of ".expect(" in the three reported buckets and fails or prints mismatches so the documented "Current baseline before that cleanup" counts stay accurate; implement it to run the three checks shown in the comment (examples/**/*.rs, benches/**/*.rs, and Rust doc-comments in src/**/*.rs using a regex that matches //.*/!.?\.expect\() and output or return a nonzero status when counts differ from the documented baseline so CI or maintainers can detect drift.src/core/tds.rs (1)
625-627: ⚡ Quick winAdd
#[must_use]to the new public wrapper type.
TdsMutationErroris a public wrapper introduced in this PR, but it is not annotated#[must_use]. That weakens the compiler nudge around dropped mutation failures right where the API is moving toward typed errors.Suggested change
#[derive(Clone, Debug, Error, PartialEq, Eq)] +#[must_use] #[error(transparent)] pub struct TdsMutationError(TdsError);As per coding guidelines,
Error enums must be #[non_exhaustive]; public wrapper types must be #[must_use].🤖 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/core/tds.rs` around lines 625 - 627, The new public wrapper type TdsMutationError is missing #[must_use]; add the #[must_use] attribute above the TdsMutationError definition (the struct wrapping TdsError) so consumers get a compiler warning if they drop the error; keep the existing derives and #[error(transparent)] intact and apply #[must_use] to the pub struct TdsMutationError(TdsError).src/triangulation/builder.rs (2)
579-586: ⚡ Quick winDoc claim about non-Euclidean explicit handling is misleading.
Both docstrings state that non-Euclidean explicit metadata is "validated through the currently topology-aware layers (Levels 1–3) and geometric nondegeneracy", which reads as if such inputs can succeed. In practice,
validate_explicit_delaunay_property_if_euclidean(Line 1394) unconditionally rejects every non-Euclidean explicit build, so the Levels 1–3 work is effectively unreachable from the user's perspective — they always getExplicitConstructionError::ValidationFailed { message: "Explicit non-Euclidean connectivity is not supported for construction; Level 4 validator required" }. The wording aligns with the explicitglobal_topologydoctest at Line 823 (assert!(result.is_err())) and theglobal_topologydoc at Lines 793–798 which already say outright that non-Euclidean is rejected.Suggest tightening both passages to match the rejection behavior so callers don't expect a topology-aware success path:
📝 Proposed doc tweak (illustrative)
- /// The explicit connectivity is still validated when - /// [`build`](Self::build) or [`build_with_kernel`](Self::build_with_kernel) - /// is called. Euclidean explicit meshes are checked at Levels 1–4, including - /// the Delaunay empty-circumsphere property; non-Euclidean explicit metadata - /// is validated through the currently topology-aware layers (Levels 1–3) - /// and geometric nondegeneracy. Invalid explicit connectivity is returned - /// as [`DelaunayTriangulationConstructionError::ExplicitConstruction`]. + /// The explicit connectivity is still validated when + /// [`build`](Self::build) or [`build_with_kernel`](Self::build_with_kernel) + /// is called. Euclidean explicit meshes are checked at Levels 1–4, + /// including the Delaunay empty-circumsphere property. Non-Euclidean + /// explicit connectivity (e.g. `GlobalTopology::Toroidal`) is currently + /// rejected at the Level 4 step until a quotient-aware validator exists. + /// Invalid explicit connectivity is returned as + /// [`DelaunayTriangulationConstructionError::ExplicitConstruction`].Also applies to: 1193-1198
🤖 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/triangulation/builder.rs` around lines 579 - 586, The docs incorrectly imply non-Euclidean explicit connectivity can pass topology-aware validators (Levels 1–3); in practice validate_explicit_delaunay_property_if_euclidean unconditionally rejects non‑Euclidean explicit builds and returns DelaunayTriangulationConstructionError::ExplicitConstruction, so update the docstrings for build and build_with_kernel (and the other occurrence) to clearly state that non‑Euclidean explicit connectivity is rejected and requires Level 4 handling (i.e., there is no successful Level 1–3 path for non‑Euclidean explicit inputs) so users won't expect a topology-aware success path.
1381-1407: 💤 Low valueHelper name implies conditional validation but actually rejects non-Euclidean.
validate_explicit_delaunay_property_if_euclideanreads as "do the Level 4 check when Euclidean, no-op otherwise", which matches the original (since-fixed) bypass behavior. The current implementation is stricter: non-Euclidean inputs always returnExplicitConstructionError::ValidationFailed. Consider renaming so the contract is obvious at the call site (Line 1376), e.g.enforce_explicit_delaunay_propertyorvalidate_or_reject_explicit_delaunay_property. While here, you could also short-circuit the non-Euclidean rejection earlier inbuild_explicitso callers don't pay for Levels 1–3 + nondegeneracy work that will always be discarded.🤖 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/triangulation/builder.rs` around lines 1381 - 1407, The helper validate_explicit_delaunay_property_if_euclidean currently rejects non-Euclidean topology instead of behaving as a conditional validator; rename it to enforce_explicit_delaunay_property (or validate_or_reject_explicit_delaunay_property) to make the contract explicit, update all call sites (e.g., the call from build_explicit) to use the new name, and move the non-Euclidean rejection check (the is_euclidean branch that returns ExplicitConstructionError::ValidationFailed) earlier in build_explicit so you short-circuit before performing Levels 1–3 and nondegeneracy work when topology is non-Euclidean; preserve the existing dt.is_valid() mapping for the Euclidean path.
🤖 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 `@examples/diagnostics.rs`:
- Around line 106-125: The loop currently accepts any trial where
trial.is_valid() returns Err, which can include low-level TDS/topology failures;
instead, after a successful flip_k2, call trial.is_valid() and pattern-match the
Result so you only return trials that are structurally valid but fail the
Delaunay check: require the structural invariants (Tds levels 1–3) to be Ok and
the Delaunay-specific validation to be the Err variant (e.g. match
Err(ValidationError::DelaunayViolation(_)) or the concrete variant used by
DelaunayTriangulation::is_valid); for any other Err or non-matching variant
continue the loop. Reference the loop using dt.cells(), FacetHandle::new(...),
trial.flip_k2(...), and trial.is_valid() when making this change.
In `@src/core/tds.rs`:
- Around line 5116-5140: The cell sort key currently built by
cell_uuid_sort_entries (CellUuidSortEntry) only collects vertex UUIDs, which
collapses distinct periodic/lifted copies that share the same UUID multiset;
update the key to include each vertex's periodic offset (or lifted coordinates)
together with its UUID (e.g., use a Vec of (Uuid, Offset) or otherwise encode
the offset per vertex) so the sort/uniformization distinguishes periodic copies,
sort the combined (Uuid, Offset) pairs, and return that composite key; apply the
same change to the analogous helper used at the 5197-5207 region so both helpers
include periodic offsets in their sort keys.
In `@src/core/util/delaunay_validation.rs`:
- Around line 542-569: first_offending_vertex currently calls robust_insphere
directly (checking Ok(InSphere::INSIDE)), which bypasses the D≥4 artifact
suppression used by validate_cell_delaunay; change first_offending_vertex to
reuse the same witness-classification path as validate_cell_delaunay (either by
calling the existing helper used there or by factoring that logic out into a
shared function) so the D≥4 suppression and any tie-breaking/robust heuristics
are applied before deciding to return test_vkey, instead of invoking
robust_insphere directly.
---
Outside diff comments:
In `@examples/zero_allocation_iterator_demo.rs`:
- Around line 42-43: The early-exit diagnostic in the example uses eprintln!("No
cells in triangulation; nothing to demo."); which should be user-facing stdout;
change that call to println! instead (in the example's main/early-exit branch
where the message and subsequent return Ok(()) appear) so the message is printed
to stdout per the coding guidelines.
- Around line 80-98: Method 2 is counting all Result items while Method 1 counts
only successfully resolved UUIDs, so change the zero-allocation iterator usage
in the benchmark to count only Ok values to match Method 1: locate
cell.vertex_uuid_iter(dt.tds()) and replace the plain .count() with a
filtered/counting approach that only considers successes (e.g.,
filter_map(Result::ok).count() or equivalent), and keep the totals in
total_count_iter so total_count == total_count_iter reflects the same quantity.
In `@src/core/algorithms/flips.rs`:
- Around line 441-457: The flip routine currently mutates the live Tds via
insert_cell_with_mapping and wire_cavity_neighbors and can return Err leaving
the Tds inconsistent; change it to perform the mutation transactionally by
either applying wiring to a temporary Tds copy or installing a rollback guard
that records inserted cell keys (new_cells), any modified neighbor slots (from
wire_cavity_neighbors) and the removed_cells, and on any error (including
FlipMutationError::CellInsertion or FlipNeighborWiringError) restores the Tds to
its prior state by removing inserted cells, reinstating removed_cells, and
rewiring neighbor pointers back to their saved values before returning the
error; ensure this is used by the k=1 wrapper as well and that invariants
verified by Tds::is_valid remain preserved on both success and failure.
In `@src/core/tds.rs`:
- Around line 2222-2255: cell_mut() (and vertex_mut()) currently expose raw &mut
Cell/&mut Vertex allowing unchecked topology or offset mutations that can break
Tds/Delaunay invariants; make these methods non-public (or remove direct &mut
exposure) and replace them with a safe mutation API or a guarded proxy type that
only allows content updates (not neighbor wiring, incident_cell, periodic
offsets) and routes topology changes through existing generation-aware setters
like set_neighbors_by_key(), ensuring every mutating operation preserves
invariants checked by Tds::is_valid/DelaunayTriangulation::is_valid; update the
matching vertex_mut implementation similarly and add tests to assert invalid
topology cannot be created via the public API.
In `@src/triangulation/delaunay.rs`:
- Around line 6991-7013: The Tds reconstruction currently relies on
candidate.validate() (used by DelaunayTriangulation::try_from_tds) which runs
the Level-4 acceptance checks using the caller-supplied kernel (e.g.,
FastKernel), so near-degenerate or co-spherical states may be mis-classified;
change try_from_tds to perform a kernel-independent/exact validation pass (or
call a new exact validator) when reconstructing from serialized Tds to ensure
invariants are enforced regardless of runtime K, or clearly document in the
API/docs that try_from_tds only provides best-effort validation when given a
non-exact kernel like FastKernel and may accept borderline cases—update
references to candidate.validate(), try_from_tds, and the examples to reflect
the chosen approach.
- Around line 2127-2137: The reported bug is that
new_with_construction_statistics (and the other constructors funneling through
with_options_and_statistics) only return the last attempt’s
ConstructionStatistics, losing counts from earlier retries/fallbacks; update
with_options_and_statistics to accumulate/merge attempt-level stats across all
reconstruction attempts and fallback paths (summing total_attempts, inserted,
skipped_* and skip_samples or other relevant counters) and return this
aggregated ConstructionStatistics to callers like
new_with_construction_statistics (and the other affected constructors around the
noted ranges), or alternatively change the public contract to explicitly state
it returns only the final-attempt stats — implement the aggregation approach by
collecting each attempt’s ConstructionStatistics inside
with_options_and_statistics and combining them before returning.
---
Nitpick comments:
In `@docs/dev/tooling-alignment.md`:
- Around line 86-90: Add a small verification script / just recipe (e.g.,
verify-expect-counts) that programmatically counts occurrences of ".expect(" in
the three reported buckets and fails or prints mismatches so the documented
"Current baseline before that cleanup" counts stay accurate; implement it to run
the three checks shown in the comment (examples/**/*.rs, benches/**/*.rs, and
Rust doc-comments in src/**/*.rs using a regex that matches //.*/!.?\.expect\()
and output or return a nonzero status when counts differ from the documented
baseline so CI or maintainers can detect drift.
In `@examples/zero_allocation_iterator_demo.rs`:
- Around line 16-22: Add the #[non_exhaustive] attribute to the DemoError enum
declaration to follow the coding guideline for error enums; locate the enum
named DemoError (the #[derive(Debug, thiserror::Error)] enum DemoError { ... })
and prepend #[non_exhaustive] above it so the definition becomes non-exhaustive
without changing its variants or derives.
In `@src/core/tds.rs`:
- Around line 625-627: The new public wrapper type TdsMutationError is missing
#[must_use]; add the #[must_use] attribute above the TdsMutationError definition
(the struct wrapping TdsError) so consumers get a compiler warning if they drop
the error; keep the existing derives and #[error(transparent)] intact and apply
#[must_use] to the pub struct TdsMutationError(TdsError).
In `@src/triangulation/builder.rs`:
- Around line 579-586: The docs incorrectly imply non-Euclidean explicit
connectivity can pass topology-aware validators (Levels 1–3); in practice
validate_explicit_delaunay_property_if_euclidean unconditionally rejects
non‑Euclidean explicit builds and returns
DelaunayTriangulationConstructionError::ExplicitConstruction, so update the
docstrings for build and build_with_kernel (and the other occurrence) to clearly
state that non‑Euclidean explicit connectivity is rejected and requires Level 4
handling (i.e., there is no successful Level 1–3 path for non‑Euclidean explicit
inputs) so users won't expect a topology-aware success path.
- Around line 1381-1407: The helper
validate_explicit_delaunay_property_if_euclidean currently rejects non-Euclidean
topology instead of behaving as a conditional validator; rename it to
enforce_explicit_delaunay_property (or
validate_or_reject_explicit_delaunay_property) to make the contract explicit,
update all call sites (e.g., the call from build_explicit) to use the new name,
and move the non-Euclidean rejection check (the is_euclidean branch that returns
ExplicitConstructionError::ValidationFailed) earlier in build_explicit so you
short-circuit before performing Levels 1–3 and nondegeneracy work when topology
is non-Euclidean; preserve the existing dt.is_valid() mapping for the Euclidean
path.
🪄 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: 7d8c89a2-7b8d-473f-8b39-38cc6f03b80e
📒 Files selected for processing (33)
README.mdbenches/topology_guarantee_construction.rsdocs/README.mddocs/dev/rust.mddocs/dev/tooling-alignment.mddocs/diagnostics.mddocs/numerical_robustness_guide.mdexamples/README.mdexamples/diagnostics.rsexamples/numerical_robustness.rsexamples/zero_allocation_iterator_demo.rsscripts/archive_changelog.pyscripts/tests/test_archive_changelog.pysemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/cell.rssrc/core/facet.rssrc/core/tds.rssrc/core/traits/facet_cache.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_utils.rssrc/core/vertex.rssrc/lib.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/flips.rstests/conflict_region_verification.rstests/delaunay_edge_cases.rstests/prelude_exports.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rs
✅ Files skipped from review due to trivial changes (10)
- src/core/traits/facet_cache.rs
- README.md
- docs/README.md
- docs/numerical_robustness_guide.md
- tests/conflict_region_verification.rs
- src/core/util/facet_utils.rs
- docs/dev/rust.md
- docs/diagnostics.md
- tests/semgrep/src/project_rules/rust_style.rs
- benches/topology_guarantee_construction.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- tests/prelude_exports.rs
- semgrep.yaml
- tests/delaunay_edge_cases.rs
- scripts/tests/test_archive_changelog.py
- src/core/facet.rs
- src/triangulation/flips.rs
- scripts/archive_changelog.py
- tests/regressions.rs
- src/core/algorithms/pl_manifold_repair.rs
- src/core/vertex.rs
- src/lib.rs
- src/core/algorithms/incremental_insertion.rs
- Make bistellar flips commit TDS mutations only after replacement insertion, neighbor wiring, and removals succeed. - Validate reconstructed Euclidean TDS values with the robust Delaunay-property path and aggregate construction statistics across retries and fallbacks. - Keep raw TDS mutation access crate-private while preserving safe data and topology setters, and make periodic cell identity keys include offsets. - Align diagnostics, examples, explicit-builder docs, and expect-count tooling with the updated validation behavior. BREAKING CHANGE: Tds::cell_mut and Tds::vertex_mut are no longer public; use safe data setters or topology-specific mutation APIs instead.
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 (2)
src/core/tds.rs (1)
3683-3699:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMake duplicate-cell removal fail-safe.
This commits the removals before
assign_neighbors()/assign_incident_cells()run. If either rebuild returnsErr, callers get a partially mutated TDS with stale neighbor orincident_cellstate. Reusingremove_cells_by_keys()here, or staging the change on a temporary TDS, would keep the operation from returning an error after half-applying the mutation.As per coding guidelines "Every mutating operation must preserve invariants checked by
Tds::is_valid(Level 1–3) andDelaunayTriangulation::is_valid(Level 4), or fail explicitly".🤖 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/core/tds.rs` around lines 3683 - 3699, The current removal loop mutates self.cells and uuid_to_cell_key before calling assign_neighbors() and assign_incident_cells(), which can leave the TDS partially mutated if either rebuild returns Err; make the operation fail-safe by staging removals and only committing them after successful rebuild: either reuse the existing remove_cells_by_keys() helper (which should perform removals atomically) or clone the TDS (or relevant maps) into a temporary TDS, apply the removals there, run assign_neighbors() and assign_incident_cells() on the temp, and on success swap the temp into self; ensure the chosen approach preserves the invariants checked by Tds::is_valid and DelaunayTriangulation::is_valid and leaves self unchanged on any Err from assign_neighbors()/assign_incident_cells().src/triangulation/delaunay.rs (1)
2942-2954:⚠️ Potential issue | 🟠 Major | ⚡ Quick winStop shuffled retries from erasing typed topology/TDS failures.
is_non_retryable_construction_error()still only short-circuits duplicate UUIDs and internal inconsistencies. That meansTdsvalidation failures plus the newInsertionTopologyValidation/FinalTopologyValidationbuckets will keep flowing through shuffled retries, and once retries are exhausted they get flattened into a genericGeometricDegeneracybybuild_with_shuffled_retries*. This loses the new matchable error variant precisely on the deterministic topology paths this PR is trying to expose.🤖 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/triangulation/delaunay.rs` around lines 2942 - 2954, The helper is_non_retryable_construction_error currently only marks DuplicateUuid and InternalInconsistency as non-retryable, so Tds validation failures and the new topology validation buckets get retried and later flattened; update is_non_retryable_construction_error to also return true for DelaunayConstructionFailure::Tds validation failures and for the new InsertionTopologyValidation and FinalTopologyValidation variants of DelaunayTriangulationConstructionError (so these deterministic topology errors are not routed through build_with_shuffled_retries* and preserved as their specific variants).
🧹 Nitpick comments (3)
src/core/algorithms/flips.rs (2)
1823-1845: ⚡ Quick winKeep unmatched
InsertionErrorvariants typed.The wildcard arm collapses additive
InsertionErrorvariants back intoUnexpected { message }, which makes downstream matching impossible again right at the flip boundary. Please preserve the unmatched source as a typed variant instead of stringifying it.As per coding guidelines,
src/**/*.rs: Error enums must be#[non_exhaustive]; public wrapper types must be#[must_use]; new variants are additive.🤖 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/core/algorithms/flips.rs` around lines 1823 - 1845, The From<InsertionError> impl currently collapses all unmatched InsertionError cases into Unexpected { message }, losing type information; add a new typed variant on FlipNeighborWiringError (e.g., OtherInsertion or Insertion { source: InsertionError }) and return that variant in the wildcard arm instead of stringifying, update the FlipNeighborWiringError enum to include that new variant and ensure the enum is marked #[non_exhaustive] and any public wrapper error types are #[must_use] per project guidelines so downstream matching can preserve the original InsertionError value.
461-468: ⚡ Quick winAssert full TDS validity before swapping in
trial.This path now commits all flip mutations through a cloned
Tds, but it only checks coherent orientation before*tds = trial. A debug-onlytrial.is_valid().is_ok()here would catch stale neighbor wiring or reciprocity drift at the mutation boundary.As per coding guidelines, `src/core/**/*.rs`: Every mutating operation must preserve invariants checked by `Tds::is_valid` (Level 1–3) and `DelaunayTriangulation::is_valid` (Level 4), or fail explicitly.Suggested guard
debug_assert!( trial.is_coherently_oriented(), "TDS coherent orientation invariant violated after bistellar flip (k={k_move}, direction={direction:?})", ); + debug_assert!( + trial.is_valid().is_ok(), + "TDS validity invariant violated after bistellar flip (k={k_move}, direction={direction:?})", + ); *tds = trial;🤖 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/core/algorithms/flips.rs` around lines 461 - 468, Before committing the cloned Tds back into place, run the full validity check and fail fast if it fails: after trial.remove_cells_by_keys(...) and before *tds = trial, call trial.is_valid() and assert its Ok result (e.g. debug_assert!(trial.is_valid().is_ok(), "Tds::is_valid failed after bistellar flip (k={k_move}, direction={direction:?}): {:?}", trial.is_valid().err())). This ensures invariants checked by Tds::is_valid (Levels 1–3) — and by extension DelaunayTriangulation::is_valid (Level 4) at mutation boundaries — are validated before swapping in the mutated trial.src/triangulation/delaunay.rs (1)
3399-3420: ⚡ Quick winDrop the duplicate completion-time topology validation pass.
build_with_kernel_inner*_seeded()already goes throughfinalize_bulk_construction(), and Lines 4611-4626 run this samerequires_vertex_links_at_completion() -> tri.validate()check with the sameFinalTopologyValidationerror. Re-running it again in both wrapper helpers adds another full O(cells) pass on every successful PL-manifold build without changing behavior.Also applies to: 3465-3489, 4611-4626
🤖 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/triangulation/delaunay.rs` around lines 3399 - 3420, The post-construction topology validation block is duplicated: remove the extra call that checks dt.tri.topology_guarantee.requires_vertex_links_at_completion() followed by dt.tri.validate() and returning TriangulationConstructionError::FinalTopologyValidation, because build_with_kernel_inner*_seeded() already invokes finalize_bulk_construction() which performs the same validate() check; locate and delete the redundant validation sections (the one around dt.tri.validate() at the shown snippet and the duplicates around lines referenced in the comment) so only the finalize_bulk_construction() / build_with_kernel_inner*_seeded() path performs the final topology validation.
🤖 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 `@src/core/tds.rs`:
- Around line 3683-3699: The current removal loop mutates self.cells and
uuid_to_cell_key before calling assign_neighbors() and assign_incident_cells(),
which can leave the TDS partially mutated if either rebuild returns Err; make
the operation fail-safe by staging removals and only committing them after
successful rebuild: either reuse the existing remove_cells_by_keys() helper
(which should perform removals atomically) or clone the TDS (or relevant maps)
into a temporary TDS, apply the removals there, run assign_neighbors() and
assign_incident_cells() on the temp, and on success swap the temp into self;
ensure the chosen approach preserves the invariants checked by Tds::is_valid and
DelaunayTriangulation::is_valid and leaves self unchanged on any Err from
assign_neighbors()/assign_incident_cells().
In `@src/triangulation/delaunay.rs`:
- Around line 2942-2954: The helper is_non_retryable_construction_error
currently only marks DuplicateUuid and InternalInconsistency as non-retryable,
so Tds validation failures and the new topology validation buckets get retried
and later flattened; update is_non_retryable_construction_error to also return
true for DelaunayConstructionFailure::Tds validation failures and for the new
InsertionTopologyValidation and FinalTopologyValidation variants of
DelaunayTriangulationConstructionError (so these deterministic topology errors
are not routed through build_with_shuffled_retries* and preserved as their
specific variants).
---
Nitpick comments:
In `@src/core/algorithms/flips.rs`:
- Around line 1823-1845: The From<InsertionError> impl currently collapses all
unmatched InsertionError cases into Unexpected { message }, losing type
information; add a new typed variant on FlipNeighborWiringError (e.g.,
OtherInsertion or Insertion { source: InsertionError }) and return that variant
in the wildcard arm instead of stringifying, update the FlipNeighborWiringError
enum to include that new variant and ensure the enum is marked #[non_exhaustive]
and any public wrapper error types are #[must_use] per project guidelines so
downstream matching can preserve the original InsertionError value.
- Around line 461-468: Before committing the cloned Tds back into place, run the
full validity check and fail fast if it fails: after
trial.remove_cells_by_keys(...) and before *tds = trial, call trial.is_valid()
and assert its Ok result (e.g. debug_assert!(trial.is_valid().is_ok(),
"Tds::is_valid failed after bistellar flip (k={k_move},
direction={direction:?}): {:?}", trial.is_valid().err())). This ensures
invariants checked by Tds::is_valid (Levels 1–3) — and by extension
DelaunayTriangulation::is_valid (Level 4) at mutation boundaries — are validated
before swapping in the mutated trial.
In `@src/triangulation/delaunay.rs`:
- Around line 3399-3420: The post-construction topology validation block is
duplicated: remove the extra call that checks
dt.tri.topology_guarantee.requires_vertex_links_at_completion() followed by
dt.tri.validate() and returning
TriangulationConstructionError::FinalTopologyValidation, because
build_with_kernel_inner*_seeded() already invokes finalize_bulk_construction()
which performs the same validate() check; locate and delete the redundant
validation sections (the one around dt.tri.validate() at the shown snippet and
the duplicates around lines referenced in the comment) so only the
finalize_bulk_construction() / build_with_kernel_inner*_seeded() path performs
the final topology validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: b283e15c-d31f-4470-8a77-1a9252bb0c46
📒 Files selected for processing (9)
docs/dev/tooling-alignment.mdexamples/diagnostics.rsexamples/zero_allocation_iterator_demo.rsjustfilesrc/core/algorithms/flips.rssrc/core/tds.rssrc/core/util/delaunay_validation.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rs
✅ Files skipped from review due to trivial changes (2)
- examples/diagnostics.rs
- docs/dev/tooling-alignment.md
🚧 Files skipped from review as they are similar to previous changes (4)
- examples/zero_allocation_iterator_demo.rs
- justfile
- src/triangulation/builder.rs
- src/core/util/delaunay_validation.rs
1 participant
BREAKING CHANGE: Delaunay construction and reconstruction now reject unchecked or non-Delaunay topology that could previously be wrapped as DelaunayTriangulation. The public API also removes the optional SlotMap backend/comparison path, renames legacy get_* accessors to noun-style accessors, narrows focused prelude exports, and changes insertion/construction error matching to typed variants.