refactor(api)!: require validated coordinate topology API (#442) - #455
Merged
Conversation
- Store coordinates through validated finite-coordinate types and remove the public coordinate-scalar parameter from core geometry, TDS, hull, and triangulation APIs. - Replace macro and infallible raw constructors with explicit fallible smart constructors for points, vertices, simplices, edges, facets, and flip handles. - Serialize topology relationships through stable vertex and simplex UUIDs instead of process-local slotmap keys. - Add semgrep guardrails and update docs, examples, benches, and tests for the validated-coordinate API. BREAKING CHANGE: core geometry and topology types now use the validated f64-coordinate API, such as Point<D>, Vertex<U, D>, Simplex<V, D>, Tds<U, V, D>, and ConvexHull<U, V, D>, instead of exposing a coordinate scalar type parameter. BREAKING CHANGE: callers must use explicit fallible constructors such as Point::try_new, Vertex::try_new, Vertex::try_new_with_data, Simplex::try_new, EdgeKey::try_new, FacetView::try_new, and TriangleHandle::try_new instead of removed macro or infallible raw constructors. BREAKING CHANGE: serialized TDS topology no longer preserves slotmap keys and must be reconstructed from stable UUID relationships. Closes #442
Contributor
WalkthroughThis PR converts core storage and topology APIs to validated ChangesCore migration
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
acgetchell
enabled auto-merge (squash)
June 14, 2026 05:29
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 20 |
🟢 Coverage 98.52% diff coverage · +0.29% coverage variation
Metric Results Coverage variation ✅ +0.29% coverage variation (-1.00%) Diff coverage ✅ 98.52% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (2319f03) 64257 58571 91.15% Head commit (5e55640) 66469 (+2212) 60780 (+2209) 91.44% (+0.29%) 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 (#455) 11564 11393 98.52% 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 #455 +/- ##
==========================================
+ Coverage 91.12% 91.41% +0.29%
==========================================
Files 72 72
Lines 64045 66257 +2212
==========================================
+ Hits 58361 60570 +2209
- Misses 5684 5687 +3
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 15
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
src/core/traits/boundary_analysis.rs (1)
59-59:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winOutdated generic parameters in
boundary_facetsreturn-type documentation. Both the trait definition and its implementation documentBoundaryFacetsIter<'_, T, U, V, D>but the actual return type is nowBoundaryFacetsIter<'_, U, V, D>(theTparameter was removed).
src/core/traits/boundary_analysis.rs#L59-L59: Update doc comment to referenceBoundaryFacetsIter<'_, U, V, D>src/core/boundary.rs#L59-L59: Update doc comment to referenceBoundaryFacetsIter<'_, U, V, D>🤖 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/traits/boundary_analysis.rs` at line 59, The return-type documentation for `boundary_facets` contains an outdated generic parameter list that includes a `T` parameter that no longer exists in the actual type signature. In `src/core/traits/boundary_analysis.rs` at lines 59-59, update the doc comment that references `BoundaryFacetsIter<'_, T, U, V, D>` to instead reference `BoundaryFacetsIter<'_, U, V, D>`. Similarly, in `src/core/boundary.rs` at lines 59-59, apply the same update to the implementation's doc comment to remove the `T` parameter and maintain consistency with the actual return type.docs/validation.md (1)
99-112:⚠️ Potential issue | 🔴 CriticalDocumentation examples will not compile due to missing
CoordinateValidationErrorconversion.All snippets using
Vertex::<(), _>::try_new(...)?attempt to propagateCoordinateValidationError, but neitherValidationExampleErrornorDelaunayTriangulationConstructionError(the return types across all affected examples) implementFrom<CoordinateValidationError>. The?operator will fail at compile time.Either add a
From<CoordinateValidationError>implementation to the error types, or refactor the examples to avoid usingtry_newdirectly.Affects lines 99-112, 163-169, 315-321, 400-407, and 472-478.
🤖 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/validation.md` around lines 99 - 112, The documentation examples using `Vertex::<(), _>::try_new(...)?` will not compile because the error enum `ValidationExampleError` and its nested `DelaunayTriangulationConstructionError` do not implement `From<CoordinateValidationError>`, which is required for the `?` operator to propagate the error. Add a `From<CoordinateValidationError>` implementation to `ValidationExampleError` enum (using the `#[from]` attribute similar to the existing implementations), or alternatively refactor all affected example code snippets to handle `CoordinateValidationError` explicitly instead of using the `?` propagation operator.src/core/vertex.rs (1)
696-703:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winUpdate the
PartialOrddocs to reflect finite-only vertices.The comment here still describes NaN/infinity ordering semantics, but
Vertex::try_newandVertex::try_new_with_datanow reject non-finite coordinates. Leaving that text in place misstates the public contract.Based on the PR objective that the public API now enforces validated
f64coordinates at the boundary.🤖 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 696 - 703, The documentation comment for the partial_cmp method in the PartialOrd implementation for Vertex<U, D> describes NaN/infinity ordering semantics through OrderedFloat, but this is no longer accurate since the Vertex::try_new and Vertex::try_new_with_data constructors now reject non-finite coordinates. Update the doc comment to reflect that vertices are guaranteed to contain only finite coordinates, removing any references to special floating-point value handling (NaN, infinity) and OrderedFloat semantics, while preserving the core description that ordering is based on lexicographic order of coordinates.
🧹 Nitpick comments (3)
src/core/insertion.rs (1)
524-528: 💤 Low valueDead branches after f64-only refactor.
Since
F64_MANTISSA_DIGITSis 53 for f64, the<= 24condition is always false. This branch was likely for f32 support which is now removed. Consider simplifying to just:let epsilon_value: f64 = 1e-8;The same pattern exists in
duplicate_relative_tolerance()at lines 809-815.🤖 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/insertion.rs` around lines 524 - 528, Remove the dead conditional branch in the epsilon_value assignment at lines 524-528. Since F64_MANTISSA_DIGITS is always 53 for f64, the condition checking if it is <= 24 is always false, so simplify this code block to directly assign epsilon_value to 1e-8 without the if-else statement. Apply the same simplification to the duplicate_relative_tolerance() function at lines 809-815 where the identical pattern exists, removing its conditional check and directly assigning epsilon_value to 1e-8.src/core/algorithms/incremental_insertion.rs (1)
1659-1665: ⚡ Quick winAdd a regression test for
PerturbedCoordinateInvalid.This new public variant is now part of the caller-visible contract, but the updated tests do not pin its
InsertionErrorKindmapping or its non-retryable classification. A missed match arm here will still compile and silently change downstream retry behavior.As per coding guidelines, “Unit tests must cover known values, error paths, and dimension-generic correctness.”
🤖 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 1659 - 1665, Add a regression test for the `PerturbedCoordinateInvalid` error variant to ensure it is properly handled as a non-retryable error. The test should verify that when a `PerturbedCoordinateInvalid` error occurs, it correctly maps to the expected `InsertionErrorKind` value and is classified as non-retryable (meaning it should not trigger retry logic). Create a test case that constructs a `CoordinateValidationError`, wraps it in `PerturbedCoordinateInvalid`, and asserts both the error kind mapping and the non-retryable classification to prevent silent behavior changes in downstream retry handling.Source: Coding guidelines
src/core/triangulation.rs (1)
83-83: ⚡ Quick winDrop
constfrom this test constructor.Line 83 turns a heap-backed test helper into
const fn, but this helper is not one of the pure-math utilities the repo reservesconstfor. Keeping it non-const avoids unnecessary const-eval constraints on futureTds/Triangulationchanges.As per coding guidelines, "Use const fn for pure-math helpers (sign_to_orientation, sign_to_insphere, coordinate conversions) where inputs allow; do not twist mutating APIs into const fn for its own sake."
🤖 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` at line 83, The new_with_tds method in the Triangulation struct is marked as const fn, but it is a test helper constructor rather than a pure-math utility. Remove the const keyword from the function signature of new_with_tds to align with the repo's coding guidelines that reserve const fn only for pure-math helpers like sign_to_orientation, sign_to_insphere, and coordinate conversions.Source: Coding guidelines
🤖 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 `@benches/README.md`:
- Line 251: The profiling bullet point at line 251 was edited to remove explicit
mention of vertex iteration, but the benchmark documentation at line 270 still
references a queries/vertices filter. To resolve this documentation mismatch,
restore the explicit vertex-iteration mention in the profiling bullet at line
251 so that it aligns with the queries/vertices filter documented elsewhere in
the file. This ensures the benchmark-scope documentation is consistent and
complete.
In `@docs/api_design.md`:
- Around line 253-254: The bullet point description for the Inverse
(flip_k1_remove) operation is missing a noun, making the sentence grammatically
incorrect. In the line starting with "Remove a collapsing its star", add the
missing noun (vertex) to clarify the action being performed, so it reads
something like "Remove a vertex, collapsing its star" or similar phrasing that
makes the sentence grammatically complete and clear.
In `@docs/archive/invariant_validation_plan.md`:
- Around line 208-210: The insert_vertex method call within the loop iterating
over vertices is missing required parameters. The method currently only receives
&mut tds, but it needs the vertex key and coordinates as additional arguments.
Update the algorithm.insert_vertex call to pass the loop variable vertex along
with any required identifiers or coordinate data (such as vertex_key and
vertex.point().coords() or equivalent) based on the actual method signature, so
that the currently unused vertex parameter from the loop is properly utilized.
In `@docs/archive/optimization_recommendations_historical.md`:
- Around line 186-187: The method signature `find_bad_cells_cached` at line
186-187 uses invalid Rust syntax with the pattern `&self: &Vertex<T, U, D>`,
which cannot have both a self receiver and a type annotation. This same invalid
pattern appears at lines 437-438 and 536-537. Fix each occurrence by changing
the signature to use a normal `&self` receiver and adding a separate `vertex:
&Vertex<T, U, D>` parameter instead, removing the invalid `&self: &Vertex<...>`
pattern completely. Apply this correction to all three affected locations in the
archived documentation file.
- Around line 80-81: The format! macro call in the message construction contains
invalid syntax where .uuid() is being called on the string literal. Remove the
incorrect .uuid() method call from the string literal and instead use it as an
argument to the format! macro. Replace the current line that calls .uuid() on
the format string with proper format! macro syntax where the format string is
followed by a comma and then the uuid value or variable as an argument, matching
the placeholder {debug} in the format string.
In `@docs/archive/phase_3a_implementation_guide.md`:
- Line 543: The migration guide contains multiple invalid Rust code snippets
that need correction across all affected locations. At file
docs/archive/phase_3a_implementation_guide.md lines 543-543 (anchor), 675-680
(sibling), 1294-1299 (sibling), and 1483-1496 (sibling): fix the println! macro
calls that incorrectly invoke .point() as a method on string literals (should
use the format placeholder {:?} with the actual variable instead), correct
malformed function signatures like &self: &Vertex (should follow proper Rust
self parameter syntax), and remove or adjust any ? operator usage in code
snippets where the function does not return a Result or Option type. Ensure all
Rust examples are syntactically valid and mechanically accurate.
In `@docs/diagnostics.md`:
- Around line 72-77: The documentation snippets contain non-compilable code
because error type mismatches exist between what the main function signatures
declare as return types and what the Vertex::try_new calls actually return. In
both snippets, Vertex::try_new returns CoordinateConversionError but the first
snippet's main function returns Result<(),
DelaunayTriangulationConstructionError> without a From implementation to convert
between these error types, and the second snippet's DiagnosticsExampleError enum
defined in the docs is missing a CoordinateConversion variant needed to handle
errors from Vertex::try_new. Fix by adding the missing CoordinateConversion
variant to the DiagnosticsExampleError enum definition and ensuring
DelaunayTriangulationConstructionError has a From<CoordinateConversionError>
implementation, or alternatively replace all ? operators on try_new calls with
.expect() to avoid needing error conversion.
In `@docs/invariants.md`:
- Around line 232-233: Two phrases in the document have been incorrectly edited
and need to be restored. In the section at lines 232-233, fix "isolated-and
Euler characteristic checks" to read "isolated-vertex checks" and correct
"around" the abstracted away from the embedding space" to read "neighborhood
around the vertex". Apply the same corrections to the sibling occurrence at
lines 252-253, ensuring both locations use consistent and meaningful language
that preserves the original intent regarding isolated-vertex and
neighborhood-around-vertex concepts.
In `@docs/numerical_robustness_guide.md`:
- Around line 393-395: The sentence at line 394 in the guidance text contains a
grammatically malformed phrase "drop the perturb/rescale your point set" that
makes the intended decision options unclear. Revise this phrase to be
grammatically correct and ensure it clearly presents the two distinct
application-level options for handling the Skipped outcome (such as choosing
between one action versus another action, or between modifying the point set in
different ways versus re-running with a different kernel).
In `@docs/workflows.md`:
- Around line 329-330: The example code in the loop has two compilation errors
that need to be fixed. First, the loop pattern `for (_key) in dt.vertices()` is
dropping the vertex data instead of binding it—change the pattern to properly
capture the vertex (either rename `_key` to a meaningful variable name like
`vertex`, or adjust the pattern if dt.vertices() returns key-value pairs).
Second, the `.data()` method call is incorrectly placed on the string literal
`"data = {:?}"` instead of on the vertex object itself—move the `.data()` call
from the string to the actual vertex variable and ensure it is properly used
within the format string for println.
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 3301-3303: The repair_neighbor_pointers function must validate
simplex arity before performing any facet incidence rebuilding. Add a check at
the beginning of repair_neighbor_pointers that verifies each simplex has the
correct number of vertices (D+1 vertices for dimension D). If any simplex has
incorrect arity, immediately return a WrongSimplexArity error instead of
proceeding with the neighbor pointer repair, which would silently hide the
invariant violation and cause issues later during normalization/validation.
In `@src/core/algorithms/pl_manifold_repair.rs`:
- Around line 202-205: The repair_facet_oversharing function mutates tds but the
early returns for NoProgress and BudgetExhausted errors after line 289 bypass
the deferred neighbor/incidence rebuild at line 300, leaving tds in an invalid
state with stale topology links. Restructure the control flow to ensure the
topology rebuild operations execute before any error returns, guaranteeing that
tds remains valid according to Tds::is_valid invariants regardless of which
error condition is encountered during the repair process.
In `@src/core/collections/buffers.rs`:
- Around line 220-222: Correct the documentation comment for the periodic
simplex buffer to accurately describe its storage model. The comment currently
states that offsets are stored "per simplex," but the buffer actually stores
offsets per-vertex-slot. Update the comment text (starting around line 220 in
the buffers.rs file) to replace "per simplex" with "per-vertex-slot" to
accurately reflect the buffer's actual structure and avoid confusion about
slot-alignment invariants.
In `@src/core/util/canonical_points.rs`:
- Around line 52-56: The functions sorted_simplex_points (lines 52-56) and the
other public helper at lines 92-97 currently return Option, which discards error
information and requires generic error mapping at call sites. Change both
function signatures to return Result<SmallBuffer<Point<D>,
MAX_PRACTICAL_DIMENSION_SIZE>, CanonicalPointError> instead of Option. Define or
ensure CanonicalPointError is a non-exhaustive error type that captures failure
details. Update all call sites of these functions to handle the Result type
using proper error propagation patterns instead of Option-based handling.
In `@src/core/util/facet_keys.rs`:
- Around line 129-130: The doc comment starting at the line with "The facet
opposite `facet_index`" contains a grammatically unclear phrase "is selected
keys are sorted" that lacks proper punctuation or connective text. Reword this
comment to clarify the relationship between selecting the opposite facet and
sorting the keys, either by adding appropriate punctuation, splitting into
multiple sentences, or restructuring the phrasing so that it reads clearly and
conveys that keys are sorted for permutation invariance and offsets are
normalized.
---
Outside diff comments:
In `@docs/validation.md`:
- Around line 99-112: The documentation examples using `Vertex::<(),
_>::try_new(...)?` will not compile because the error enum
`ValidationExampleError` and its nested `DelaunayTriangulationConstructionError`
do not implement `From<CoordinateValidationError>`, which is required for the
`?` operator to propagate the error. Add a `From<CoordinateValidationError>`
implementation to `ValidationExampleError` enum (using the `#[from]` attribute
similar to the existing implementations), or alternatively refactor all affected
example code snippets to handle `CoordinateValidationError` explicitly instead
of using the `?` propagation operator.
In `@src/core/traits/boundary_analysis.rs`:
- Line 59: The return-type documentation for `boundary_facets` contains an
outdated generic parameter list that includes a `T` parameter that no longer
exists in the actual type signature. In `src/core/traits/boundary_analysis.rs`
at lines 59-59, update the doc comment that references `BoundaryFacetsIter<'_,
T, U, V, D>` to instead reference `BoundaryFacetsIter<'_, U, V, D>`. Similarly,
in `src/core/boundary.rs` at lines 59-59, apply the same update to the
implementation's doc comment to remove the `T` parameter and maintain
consistency with the actual return type.
In `@src/core/vertex.rs`:
- Around line 696-703: The documentation comment for the partial_cmp method in
the PartialOrd implementation for Vertex<U, D> describes NaN/infinity ordering
semantics through OrderedFloat, but this is no longer accurate since the
Vertex::try_new and Vertex::try_new_with_data constructors now reject non-finite
coordinates. Update the doc comment to reflect that vertices are guaranteed to
contain only finite coordinates, removing any references to special
floating-point value handling (NaN, infinity) and OrderedFloat semantics, while
preserving the core description that ordering is based on lexicographic order of
coordinates.
---
Nitpick comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 1659-1665: Add a regression test for the
`PerturbedCoordinateInvalid` error variant to ensure it is properly handled as a
non-retryable error. The test should verify that when a
`PerturbedCoordinateInvalid` error occurs, it correctly maps to the expected
`InsertionErrorKind` value and is classified as non-retryable (meaning it should
not trigger retry logic). Create a test case that constructs a
`CoordinateValidationError`, wraps it in `PerturbedCoordinateInvalid`, and
asserts both the error kind mapping and the non-retryable classification to
prevent silent behavior changes in downstream retry handling.
In `@src/core/insertion.rs`:
- Around line 524-528: Remove the dead conditional branch in the epsilon_value
assignment at lines 524-528. Since F64_MANTISSA_DIGITS is always 53 for f64, the
condition checking if it is <= 24 is always false, so simplify this code block
to directly assign epsilon_value to 1e-8 without the if-else statement. Apply
the same simplification to the duplicate_relative_tolerance() function at lines
809-815 where the identical pattern exists, removing its conditional check and
directly assigning epsilon_value to 1e-8.
In `@src/core/triangulation.rs`:
- Line 83: The new_with_tds method in the Triangulation struct is marked as
const fn, but it is a test helper constructor rather than a pure-math utility.
Remove the const keyword from the function signature of new_with_tds to align
with the repo's coding guidelines that reserve const fn only for pure-math
helpers like sign_to_orientation, sign_to_insphere, and coordinate conversions.
🪄 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: 2238540d-6a8b-4592-8cdb-74d5ada5ed12
📒 Files selected for processing (138)
README.mdbenches/README.mdbenches/allocation_hot_paths.rsbenches/boundary_uuid_iter.rsbenches/ci_performance_suite.rsbenches/circumsphere_containment.rsbenches/cold_path_predicates.rsbenches/common/flip_workflows.rsbenches/profiling_suite.rsbenches/remove_vertex.rsbenches/tds_clone.rsbenches/topology_guarantee_construction.rsdocs/ORIENTATION_SPEC.mddocs/api_design.mddocs/archive/fix-delaunay.mddocs/archive/invariant_validation_plan.mddocs/archive/optimization_recommendations_historical.mddocs/archive/phase2_bowyer_watson_optimization.mddocs/archive/phase_3a_implementation_guide.mddocs/archive/topology_integration_design_historical.mddocs/code_organization.mddocs/dev/rust.mddocs/dev/testing.mddocs/diagnostics.mddocs/invariants.mddocs/limitations.mddocs/numerical_robustness_guide.mddocs/topology.mddocs/validation.mddocs/workflows.mdexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/into_from_conversions.rsexamples/numerical_robustness.rsexamples/point_comparison_and_hashing.rsexamples/topology_editing.rsexamples/triangulation_and_hull.rssemgrep.yamlsrc/core/adjacency.rssrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/collections/aliases.rssrc/core/collections/buffers.rssrc/core/collections/key_maps.rssrc/core/collections/secondary_maps.rssrc/core/collections/spatial_hash_grid.rssrc/core/construction.rssrc/core/edge.rssrc/core/facet.rssrc/core/insertion.rssrc/core/orientation.rssrc/core/query.rssrc/core/repair.rssrc/core/simplex.rssrc/core/tds.rssrc/core/traits/boundary_analysis.rssrc/core/traits/facet_cache.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/deduplication.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/hilbert.rssrc/core/util/jaccard.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/locality.rssrc/delaunay/query.rssrc/delaunay/repair.rssrc/delaunay/serialization.rssrc/delaunay/triangulation.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/point.rssrc/geometry/predicates.rssrc/geometry/quality.rssrc/geometry/robust_predicates.rssrc/geometry/sos.rssrc/geometry/traits/coordinate.rssrc/geometry/util/circumsphere.rssrc/geometry/util/conversions.rssrc/geometry/util/measures.rssrc/geometry/util/norms.rssrc/geometry/util/point_generation.rssrc/geometry/util/triangulation_generation.rssrc/lib.rssrc/topology/characteristics/euler.rssrc/topology/characteristics/validation.rssrc/topology/manifold.rssrc/topology/spaces/toroidal.rssrc/topology/traits/global_topology_model.rssrc/topology/traits/topological_space.rstests/README.mdtests/circumsphere_debug_tools.rstests/coordinate_conversion_errors.rstests/dedup_batch_construction.rstests/delaunay_edge_cases.rstests/delaunay_incremental_insertion.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/example_workflows.rstests/insert_with_statistics.rstests/large_scale_debug.rstests/pachner_roundtrip.rstests/prelude_exports.rstests/proptest_convex_hull.rstests/proptest_delaunay_triangulation.rstests/proptest_euler_characteristic.rstests/proptest_facet.rstests/proptest_flips.rstests/proptest_geometry.rstests/proptest_orientation.rstests/proptest_point.rstests/proptest_predicates.rstests/proptest_serialization.rstests/proptest_simplex.rstests/proptest_sos.rstests/proptest_tds.rstests/proptest_triangulation.rstests/proptest_vertex.rstests/public_topology_api.rstests/regressions.rstests/semgrep/doctests/unwrap_expect.txttests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rstests/trait_bound_ergonomics.rstests/triangulation_builder.rs
- Return typed conflict-region errors for invalid simplex arity and missing simplex vertices. - Make canonical predicate point collection crate-private and fallible with simplex- and facet-specific errors. - Rebuild topology metadata on PL-manifold repair error returns without masking the primary non-convergence error. - Remove stale generic coordinate-scalar branches from f64 insertion tolerances and update docs around validated coordinates. BREAKING CHANGE: malformed simplex predicate inputs now surface `InvalidSimplexArity` and `MissingSimplexVertex` conflict errors instead of the generic `SimplexDataAccessFailed` path, and canonical predicate point helpers are no longer public helper APIs.
acgetchell
disabled auto-merge
June 14, 2026 14:06
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/algorithms/pl_manifold_repair.rs (1)
89-102: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMark
PlManifoldRepairStatsas#[must_use].Line 89 introduces a public wrapper result type for repair outcomes; without
#[must_use], callers can silently drop success/failure metadata and removal artifacts.Suggested fix
+#[must_use] #[derive(Debug, Clone)] pub struct PlManifoldRepairStats<U, V, const D: usize> {As per coding guidelines, “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/algorithms/pl_manifold_repair.rs` around lines 89 - 102, The struct PlManifoldRepairStats is a public wrapper type that contains important metadata about repair outcomes including the succeeded flag and removal artifacts, but it lacks the #[must_use] attribute. Add the #[must_use] attribute directly above the struct definition (before pub struct PlManifoldRepairStats) to enforce that callers must explicitly acknowledge or handle the repair results rather than silently dropping them.Source: Coding guidelines
🤖 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 `@docs/archive/phase_3a_implementation_guide.md`:
- Around line 679-680: Update the documentation at lines 679-680 and lines
1292-1299 to use the post-migration generic type shape for Tds. Change the Tds
generic parameter lists from the pre-migration forms (Tds<T, U, V, D> and
Tds<f64, ...>) to the post-migration form (Tds<U, V, D>) to align with the
post-migration constructors shown in the example bodies. This ensures the code
snippets are consistent and readers can accurately copy the examples without
confusion from mixed generic type signatures.
---
Outside diff comments:
In `@src/core/algorithms/pl_manifold_repair.rs`:
- Around line 89-102: The struct PlManifoldRepairStats is a public wrapper type
that contains important metadata about repair outcomes including the succeeded
flag and removal artifacts, but it lacks the #[must_use] attribute. Add the
#[must_use] attribute directly above the struct definition (before pub struct
PlManifoldRepairStats) to enforce that callers must explicitly acknowledge or
handle the repair results rather than silently dropping them.
🪄 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: fc70d998-03b5-4749-bc7c-35ab0111dc79
📒 Files selected for processing (21)
docs/api_design.mddocs/archive/invariant_validation_plan.mddocs/archive/optimization_recommendations_historical.mddocs/archive/phase_3a_implementation_guide.mddocs/diagnostics.mddocs/invariants.mddocs/numerical_robustness_guide.mddocs/validation.mddocs/workflows.mdsrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/collections/buffers.rssrc/core/facet.rssrc/core/insertion.rssrc/core/traits/boundary_analysis.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/facet_keys.rssrc/core/vertex.rs
✅ Files skipped from review due to trivial changes (8)
- src/core/collections/buffers.rs
- docs/numerical_robustness_guide.md
- docs/validation.md
- docs/invariants.md
- docs/api_design.md
- docs/diagnostics.md
- docs/archive/optimization_recommendations_historical.md
- docs/archive/invariant_validation_plan.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/traits/boundary_analysis.rs
- src/core/util/facet_keys.rs
- src/core/triangulation.rs
- src/core/boundary.rs
- src/core/insertion.rs
- src/core/facet.rs
- src/core/vertex.rs
- src/core/algorithms/incremental_insertion.rs
Contributor
There was a problem hiding this comment.
Caution
Inline review comments failed to post. This is likely due to GitHub's internal server error or limits when posting large numbers of comments. If you are seeing this consistently it is likely a permissions issue. Please check "Moderation" -> "Code review limits" under your organization settings.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/core/algorithms/pl_manifold_repair.rs (1)
89-102: 🛠️ Refactor suggestion | 🟠 Major | ⚡ Quick winMark
PlManifoldRepairStatsas#[must_use].Line 89 introduces a public wrapper result type for repair outcomes; without
#[must_use], callers can silently drop success/failure metadata and removal artifacts.Suggested fix
+#[must_use] #[derive(Debug, Clone)] pub struct PlManifoldRepairStats<U, V, const D: usize> {As per coding guidelines, “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/algorithms/pl_manifold_repair.rs` around lines 89 - 102, The struct PlManifoldRepairStats is a public wrapper type that contains important metadata about repair outcomes including the succeeded flag and removal artifacts, but it lacks the #[must_use] attribute. Add the #[must_use] attribute directly above the struct definition (before pub struct PlManifoldRepairStats) to enforce that callers must explicitly acknowledge or handle the repair results rather than silently dropping them.Source: Coding guidelines
🤖 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 `@docs/archive/phase_3a_implementation_guide.md`:
- Around line 679-680: Update the documentation at lines 679-680 and lines
1292-1299 to use the post-migration generic type shape for Tds. Change the Tds
generic parameter lists from the pre-migration forms (Tds<T, U, V, D> and
Tds<f64, ...>) to the post-migration form (Tds<U, V, D>) to align with the
post-migration constructors shown in the example bodies. This ensures the code
snippets are consistent and readers can accurately copy the examples without
confusion from mixed generic type signatures.
---
Outside diff comments:
In `@src/core/algorithms/pl_manifold_repair.rs`:
- Around line 89-102: The struct PlManifoldRepairStats is a public wrapper type
that contains important metadata about repair outcomes including the succeeded
flag and removal artifacts, but it lacks the #[must_use] attribute. Add the
#[must_use] attribute directly above the struct definition (before pub struct
PlManifoldRepairStats) to enforce that callers must explicitly acknowledge or
handle the repair results rather than silently dropping them.
🪄 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: fc70d998-03b5-4749-bc7c-35ab0111dc79
📒 Files selected for processing (21)
docs/api_design.mddocs/archive/invariant_validation_plan.mddocs/archive/optimization_recommendations_historical.mddocs/archive/phase_3a_implementation_guide.mddocs/diagnostics.mddocs/invariants.mddocs/numerical_robustness_guide.mddocs/validation.mddocs/workflows.mdsrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/collections/buffers.rssrc/core/facet.rssrc/core/insertion.rssrc/core/traits/boundary_analysis.rssrc/core/triangulation.rssrc/core/util/canonical_points.rssrc/core/util/facet_keys.rssrc/core/vertex.rs
✅ Files skipped from review due to trivial changes (8)
- src/core/collections/buffers.rs
- docs/numerical_robustness_guide.md
- docs/validation.md
- docs/invariants.md
- docs/api_design.md
- docs/diagnostics.md
- docs/archive/optimization_recommendations_historical.md
- docs/archive/invariant_validation_plan.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/traits/boundary_analysis.rs
- src/core/util/facet_keys.rs
- src/core/triangulation.rs
- src/core/boundary.rs
- src/core/insertion.rs
- src/core/facet.rs
- src/core/vertex.rs
- src/core/algorithms/incremental_insertion.rs
🛑 Comments failed to post (1)
docs/archive/phase_3a_implementation_guide.md (1)
679-680:
⚠️ Potential issue | 🟡 Minor | ⚡ Quick winAlign these snippets with the post-migration
Tds<U, V, D>type shape.Line 679 and Lines 1292-1299 still show pre-migration generic forms (
Tds<T, U, V, D>/Tds<f64, ...>) while the example body now uses post-migration constructors. That mixed shape is misleading for readers copying examples.Suggested doc fix
-pub fn contains_vertex(&self, vertex: &Vertex<T, U, D>, tds: &Tds<T, U, V, D>) -> bool { +pub fn contains_vertex(&self, vertex: &Vertex<U, D>, tds: &Tds<U, V, D>) -> bool {-fn create_test_tds_3d() -> Tds<f64, Option<()>, Option<()>, 3> { +fn create_test_tds_3d() -> Tds<Option<()>, Option<()>, 3> {Also applies to: 1292-1299
🤖 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/archive/phase_3a_implementation_guide.md` around lines 679 - 680, Update the documentation at lines 679-680 and lines 1292-1299 to use the post-migration generic type shape for Tds. Change the Tds generic parameter lists from the pre-migration forms (Tds<T, U, V, D> and Tds<f64, ...>) to the post-migration form (Tds<U, V, D>) to align with the post-migration constructors shown in the example bodies. This ensures the code snippets are consistent and readers can accurately copy the examples without confusion from mixed generic type signatures.
- Replace raw FacetHandle and RidgeHandle public constructors with try_new APIs that validate against a live TDS. - Require EdgeKey::try_new to prove endpoints form a real edge in live topology before storing the key. - Keep validated internal constructors crate-private and add semgrep guardrails against raw or topology-free handle construction. - Mark PL-manifold repair stats as must-use so repair outcomes are acknowledged. BREAKING CHANGE: FacetHandle::new and RidgeHandle::new are no longer public APIs, and EdgeKey::try_new now requires a live TDS argument instead of accepting only endpoint keys.
acgetchell
enabled auto-merge (squash)
June 14, 2026 15:03
Contributor
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/facet.rs (1)
1063-1081:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftDon’t silently drop facet errors during iteration.
Lines 1073-1081 and 1193-1199 turn
FacetView::try_new/facet_view.key()failures into skipped items. On a corrupted or partially reconstructed TDS, callers then get an incomplete facet stream instead of a typed failure, which can miscompute boundary extraction and hide invariant breaks as “no more facets”. Please surface a terminalFacetErrorhere instead of eliding the bad facet, even if that means changing the iterator contract.As per coding guidelines, “Every fallible operation must return Result<_, _Error>” and operations that cannot preserve invariants “must fail explicitly.”
Also applies to: 1189-1200
🤖 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/facet.rs` around lines 1063 - 1081, The FacetIterator::next method is silently skipping facets when FacetView::try_new fails instead of surfacing errors, which can cause incomplete facet streams on corrupted TDS. Change the iterator's return type from Option<Self::Item> to a Result-based approach that properly propagates FacetError when FacetView::try_new or the conversion operations fail. This change must be applied at the primary location in the next method (around the FacetView::try_new call where the Ok branch is checked) and also at the second location mentioned in the review (around lines 1189-1200, likely in a similar iterator method or continuation logic) so that both sites fail explicitly instead of eliding bad facets.Source: Coding guidelines
examples/topology_editing.rs (1)
578-589:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPropagate validated-handle failures instead of treating them as “not found”.
Lines 588, 606, and 631-633 collapse
FacetHandle::try_new/RidgeHandle::try_newerrors intoNone/continue. That makes a broken topology or stale candidate indistinguishable from “no interior facet/ridge exists”, even though this example now demonstrates live validated handle construction. ReturningResult<Option<_>, _>from these helpers and bubbling the constructor failure up throughExampleResultwould keep the demo honest when validation fails.Also applies to: 596-607, 614-633
🤖 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/topology_editing.rs` around lines 578 - 589, The find_interior_facet_2d function (and the similar functions at the consolidated sites in examples/topology_editing.rs at lines 596-607 and 614-633) currently collapse handle construction errors from FacetHandle::try_new and RidgeHandle::try_new into None using .ok(), making constructor failures indistinguishable from "not found" cases. Change the return types of these three functions from Option<FacetHandle> / Option<RidgeHandle> to Result<Option<FacetHandle>, Error> / Result<Option<RidgeHandle>, Error>, replace the .ok() calls that suppress errors with proper error propagation (using ? or similar), and update any callers to handle the Result type appropriately through ExampleResult so that validation failures bubble up as actual errors rather than being silently treated as missing facets/ridges.
🤖 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 `@examples/topology_editing.rs`:
- Around line 578-589: The find_interior_facet_2d function (and the similar
functions at the consolidated sites in examples/topology_editing.rs at lines
596-607 and 614-633) currently collapse handle construction errors from
FacetHandle::try_new and RidgeHandle::try_new into None using .ok(), making
constructor failures indistinguishable from "not found" cases. Change the return
types of these three functions from Option<FacetHandle> / Option<RidgeHandle> to
Result<Option<FacetHandle>, Error> / Result<Option<RidgeHandle>, Error>, replace
the .ok() calls that suppress errors with proper error propagation (using ? or
similar), and update any callers to handle the Result type appropriately through
ExampleResult so that validation failures bubble up as actual errors rather than
being silently treated as missing facets/ridges.
In `@src/core/facet.rs`:
- Around line 1063-1081: The FacetIterator::next method is silently skipping
facets when FacetView::try_new fails instead of surfacing errors, which can
cause incomplete facet streams on corrupted TDS. Change the iterator's return
type from Option<Self::Item> to a Result-based approach that properly propagates
FacetError when FacetView::try_new or the conversion operations fail. This
change must be applied at the primary location in the next method (around the
FacetView::try_new call where the Ok branch is checked) and also at the second
location mentioned in the review (around lines 1189-1200, likely in a similar
iterator method or continuation logic) so that both sites fail explicitly
instead of eliding bad facets.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: e126aaa0-c9d6-472f-941c-f44d1b421afe
📒 Files selected for processing (23)
benches/common/flip_workflows.rsexamples/delaunayize_repair.rsexamples/diagnostics.rsexamples/topology_editing.rssemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/algorithms/locate.rssrc/core/algorithms/pl_manifold_repair.rssrc/core/boundary.rssrc/core/edge.rssrc/core/facet.rssrc/core/insertion.rssrc/core/repair.rssrc/core/tds.rssrc/delaunay/flips.rssrc/geometry/algorithms/convex_hull.rssrc/topology/manifold.rstests/benchmark_flip_fixtures.rstests/delaunay_repair_fallback.rstests/delaunayize_workflow.rstests/pachner_roundtrip.rstests/semgrep/src/project_rules/rust_style.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- examples/diagnostics.rs
- examples/delaunayize_repair.rs
- src/core/boundary.rs
- semgrep.yaml
- src/core/repair.rs
- benches/common/flip_workflows.rs
- src/core/algorithms/locate.rs
- src/core/algorithms/pl_manifold_repair.rs
- src/core/algorithms/incremental_insertion.rs
This was referenced Jun 15, 2026
This was referenced Jun 24, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: core geometry and topology types now use the validated f64-coordinate API, such as Point, Vertex<U, D>, Simplex<V, D>, Tds<U, V, D>, and ConvexHull<U, V, D>, instead of exposing a coordinate scalar type parameter.
BREAKING CHANGE: callers must use explicit fallible constructors such as Point::try_new, Vertex::try_new, Vertex::try_new_with_data, Simplex::try_new, EdgeKey::try_new, FacetView::try_new, and TriangleHandle::try_new instead of removed macro or infallible raw constructors.
BREAKING CHANGE: serialized TDS topology no longer preserves slotmap keys and must be reconstructed from stable UUID relationships.
Closes #442