Refactor/443 329 typed validation errors#465
Merged
Merged
Conversation
- Replace string-only Level 4 verification failures with DelaunayVerificationError sources that distinguish flip-predicate and empty-circumsphere validation paths. - Surface compact verification source kinds through explicit validation summaries and public preludes. - Fix validation guide examples to handle coordinate conversion errors alongside construction errors. - Adopt stable assert_matches! in eligible tests and move the #329 roadmap item into v0.8.0. BREAKING CHANGE: DelaunayTriangulationValidationError::VerificationFailed now carries a Box<DelaunayVerificationError> source instead of a message String. Closes #329
- Parse explicit simplex specs before builder storage with fallible explicit-construction APIs. - Preserve construction, insertion, validation, and flip repair failures as typed variants, summaries, and source-bearing errors instead of parseable strings. - Route temporary triangulation assembly through candidate/proof types and add guardrails for constructor naming, debug assertions, unchecked constructors, and erased errors. BREAKING CHANGE: Explicit connectivity construction now uses `try_from_vertices_and_simplices*` and can fail before `build`; legacy string-only repair/catch-all error variants and unchecked construction paths were removed. Closes #443
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (5)
WalkthroughTyped error payloads replace string-based construction, repair, and validation messages. Explicit simplex construction becomes fallible and candidate-driven, snapshot and vertex constructors use validated naming, exports and docs are updated, Semgrep guardrails are added, and tests move to typed assertions and ChangesTyped validation, repair, and builder refactor
Sequence Diagram(s)sequenceDiagram
participant User
participant DelaunayTriangulationBuilder
participant ValidatedExplicitSimplices
participant DelaunayTriangulationCandidate
participant DelaunayTriangulation
User->>DelaunayTriangulationBuilder: try_from_vertices_and_simplices(vertices, simplices)
DelaunayTriangulationBuilder->>ValidatedExplicitSimplices: validate_explicit_simplex_specs()
ValidatedExplicitSimplices-->>DelaunayTriangulationBuilder: validated specs
User->>DelaunayTriangulationBuilder: build()
DelaunayTriangulationBuilder->>DelaunayTriangulationCandidate: assemble()
DelaunayTriangulationCandidate->>DelaunayTriangulationCandidate: staged validations
DelaunayTriangulationCandidate->>DelaunayTriangulation: into_validated_delaunay(proof)
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
Possibly related PRs
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 28 |
🟢 Coverage 89.21% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 89.21% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (0fb6607) 68283 62599 91.68% Head commit (b772486) 69019 (+736) 63271 (+672) 91.67% (0.00%) 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 (#465) 1298 1158 89.21% 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 #465 +/- ##
==========================================
- Coverage 91.65% 91.64% -0.01%
==========================================
Files 72 72
Lines 68063 68799 +736
==========================================
+ Hits 62381 63053 +672
- Misses 5682 5746 +64
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: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
src/core/algorithms/incremental_insertion.rs (2)
763-769:⚠️ Potential issue | 🟠 Major | ⚡ Quick winPreserve the typed validation context during construction-error conversion.
Both match arms discard the new typed context with
.., so callers receivingInitialSimplexConstructionError::UnexpectedInsertionStagecannot recover whether the failure came from post-insertion, local repair, final validation, etc. Carry the context into the correspondingInitialSimplexUnexpectedInsertionStagevariants instead of dropping it at this boundary.Also applies to: 777-783
🤖 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 763 - 769, In the match arms that convert TriangulationConstructionError variants to InitialSimplexConstructionError::UnexpectedInsertionStage (around lines 763-769 and 777-783), replace the `..` pattern that discards fields with explicit capture of the context field. Instead of dropping the validation context, extract it from the TriangulationConstructionError and pass it into the corresponding InitialSimplexUnexpectedInsertionStage variant so that callers can recover whether the failure originated from post-insertion validation, local repair, final validation, or other stages.
72-91:⚠️ Potential issue | 🟠 Major | ⚡ Quick winExpose hull-extension reasons through the error source chain.
HullExtensionReasonnow stores structuredCoordinateConversionError/TdsError, but it is onlyDisplay, and the wrapper fields are not#[source]. Callers usingstd::error::Error::source()still lose the predicate/TDS cause even though the typed payload exists.Proposed fix
-#[derive(Debug, Clone, PartialEq)] +#[derive(Debug, Clone, thiserror::Error, PartialEq)] #[non_exhaustive] pub enum HullExtensionReason { /// No visible boundary facets (coplanar with hull surface). + #[error("No visible boundary facets found for exterior vertex (may be coplanar with hull surface)")] NoVisibleFacets, /// Visible facets form an invalid patch. + #[error("Visible boundary facets are not a valid patch: {details}")] InvalidPatch { /// Details about why the patch was invalid. details: String, }, @@ - PredicateFailed(CoordinateConversionError), + #[error("Geometric predicate failed: {0}")] + PredicateFailed(#[source] CoordinateConversionError), @@ - Tds(TdsError), + #[error("TDS error: {0}")] + Tds(#[source] TdsError), } -impl fmt::Display for HullExtensionReason { - fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { - match self { - Self::NoVisibleFacets => f.write_str( - "No visible boundary facets found for exterior vertex (may be coplanar with hull surface)", - ), - Self::InvalidPatch { details } => write!( - f, - "Visible boundary facets are not a valid patch: {details}" - ), - Self::PredicateFailed(source) => { - write!(f, "Geometric predicate failed: {source}") - } - Self::Tds(source) => write!(f, "TDS error: {source}"), - } - } -}HullExtension { /// Structured hull-extension failure reason. + #[source] reason: HullExtensionReason, }, @@ HullExtension { /// Structured reason for failure. + #[source] reason: HullExtensionReason, },Also applies to: 537-542, 1626-1631
🤖 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 72 - 91, The HullExtensionReason enum stores structured errors in the PredicateFailed variant (containing CoordinateConversionError) and the Tds variant (containing TdsError), but these fields lack the #[source] attribute. This prevents the error source chain from being properly exposed when callers use std::error::Error::source(). Add #[source] attributes to the CoordinateConversionError field in the PredicateFailed variant and the TdsError field in the Tds variant to ensure the underlying causes are accessible through the standard error source chain.src/core/orientation.rs (1)
615-623:⚠️ Potential issue | 🟡 MinorMigrate
assert!(matches!(...))toassert_matches!macro.Line 615 contains the only remaining
assert!(matches!(...))pattern in this file. Convert it to use the standard-libraryassert_matches!macro for consistency with the rest of the codebase:Suggested conversion
- assert!( - matches!( - &err, - TdsError::Geometric(GeometricError::NegativeOrientation { message }) - if message.contains("negative geometric orientation") - && message.contains("vertices") - ), - "Error should contain vertex keys: {err}" + assert_matches!( + &err, + TdsError::Geometric(GeometricError::NegativeOrientation { message }) + if message.contains("negative geometric orientation") + && message.contains("vertices"), + "Error should contain vertex keys: {err}" );🤖 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/orientation.rs` around lines 615 - 623, Replace the assert!(matches!(...)) pattern in the assertion block that checks for TdsError::Geometric(GeometricError::NegativeOrientation { message }) with the standard-library assert_matches! macro. The pattern guard conditions checking for "negative geometric orientation" and "vertices" in the message should be preserved, and the error message parameter should be moved to the second argument of assert_matches! macro for consistency with the rest of the codebase.src/core/algorithms/flips.rs (1)
11185-11532:⚠️ Potential issue | 🟡 MinorMigrate remaining
assert!(matches!(...))calls toassert_matches!in this section.The test module at lines 11185–11532 and 15552–15562 still uses legacy
assert!(matches!(...))patterns. The file already usesassert_matches!elsewhere (e.g., lines 12047, 12728, 12761), and Rust 1.96.0 supports the stabilized macro. Complete the migration by converting these remaining blocks as shown in the example pattern.🤖 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 11185 - 11532, Replace all instances of the legacy `assert!(matches!(...))` pattern with the stabilized `assert_matches!` macro throughout the test functions in this section, including those in test_orient_replacement_simplices_rejects_conflicting_periodic_external_facet_translations, test_orient_replacement_simplices_rejects_periodic_offset_count_mismatch, test_orient_replacement_simplices_rejects_missing_replacement_periodic_offsets, test_orient_replacement_simplices_rejects_replacement_periodic_offset_length_mismatch, test_orient_replacement_simplices_rejects_missing_external_simplex, test_replacement_orientation_helpers_cover_error_paths, test_set_flip_assignment_rejects_conflicts_and_invalid_indices, test_orient_replacement_simplices_aligns_external_and_internal_facets, and test_validate_replacement_orientation_rejects_bad_geometry. Remove the `assert!(` wrapper and closing parenthesis from each `assert!(matches!(...))` call, converting it to `assert_matches!(...)` format with the matches pattern as the primary argument.
🧹 Nitpick comments (2)
src/core/tds.rs (1)
8076-8079: Complete theassert_matches!migration in this module.This file has 6 remaining
assert!(matches!(...))call sites at lines 8076–8077, 8097–8098, 11157–11158, 11208–11209, 11225–11226, and 11236–11237 that should be converted toassert_matches!for consistency with the ~60 existing uses of the macro in this file.🤖 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 8076 - 8079, The assert!(matches!(...)) pattern used in multiple locations (lines 8076-8077, 8097-8098, 11157-11158, 11208-11209, 11225-11226, and 11236-11237) should be converted to the assert_matches! macro for consistency with the existing ~60 uses in this file. For each occurrence where you have assert!(matches!(result, pattern), message), replace it with assert_matches!(result, pattern, message) to use the dedicated macro instead of the generic assert with matches pattern matching.src/delaunay/construction.rs (1)
5055-5067: ⚡ Quick winRemove the ignored
messageparameter from the synthetic fixture.
synthetic_delaunay_verification_error("...")suggests the string affects the produced typed error, but line 5058 discards it and every fixture becomes the samePostconditionFailed::Disconnectedcase. Dropping the parameter, or accepting a typed fixture kind instead, keeps these typed-payload tests honest.🤖 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/delaunay/construction.rs` around lines 5055 - 5067, The synthetic_delaunay_verification_error function accepts a message parameter but immediately discards it with let _ = message;, which is misleading since callers will think their input affects the returned error. Remove the unused message parameter from the function signature of synthetic_delaunay_verification_error and update all call sites to not pass a string argument.
🤖 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/validation.md`:
- Around line 602-610: Update the prose narrative text in the validation layers
section of the documentation to correctly describe that
Triangulation::is_valid() returns InvariantError as the public API return type,
rather than TriangulationValidationError. Clarify that InvariantError is a
wrapper enum containing variants for all validation levels (1, 2, 3, and 4),
with TriangulationValidationError being an internal variant specifically for
Level 3 failures. Ensure the narrative text aligns with the correct API usage
shown in the Pattern 2 and Pattern 3 code examples.
In `@semgrep.yaml`:
- Around line 629-631: The regex pattern in the pattern-regex field contains
`(?:new|from_[A-Za-z0-9_]+)` which matches `from_str` functions, but the rule
message explicitly permits `FromStr` trait implementations as a recommended
alternative. This creates a contradiction where valid `FromStr` implementations
would be flagged as violations. To fix this, modify the alternation pattern to
use a negative lookahead assertion that excludes `from_str` specifically, such
that the pattern matches functions named `new` or `from_*` but explicitly
excludes `from_str`. This will allow the regex to match constructor functions
while correctly permitting `FromStr` trait implementations.
In `@src/core/algorithms/flips.rs`:
- Around line 3169-3172: The PostconditionFailed variant in DelaunayRepairError
does not properly expose the nested DelaunayRepairPostconditionFailure through
the standard error source chain. Add the #[source] attribute to the reason field
in the PostconditionFailed variant to mark it as the source of the error, and
implement the std::error::Error trait for DelaunayRepairPostconditionFailure so
it can be properly discovered and accessed by error reporters through the source
chain, making the typed postcondition failure details discoverable to callers.
In `@src/core/construction.rs`:
- Around line 37-38: The PeriodicQuotientDelaunay variant is a geometric
validation phase being placed in the FinalTopologyValidation enum which carries
InvariantErrorSummary, violating the typed error contract. Remove the
PeriodicQuotientDelaunay variant from this topology-only enum, and if periodic
quotient final Delaunay validation needs wrapping, create a separate error
handling mechanism or variant whose source type properly reflects the Delaunay
validation phase rather than topology validation. Apply the same correction to
the related code at lines 52-54 and 208-214 to ensure all validation phases
remain at their correct layers and error types match their validation contexts.
In `@src/delaunay/builder.rs`:
- Around line 175-178: The `build_explicit` function and related code paths in
the range 2180-2204 currently use `into_validated_delaunay()` after calling
`is_valid()`, but this skips the Euclidean empty-circumsphere check required for
raw-TDS reconstruction validation. Replace the `into_validated_delaunay()` calls
with `try_into_validated_delaunay()` from the candidate API, which properly
executes the reconstruction validator to ensure the Level 4 proof is performed
before returning a public DelaunayTriangulation.
In `@src/delaunay/validation.rs`:
- Around line 107-143: The function `validate_delaunay_property()` only
validates Level 4 via `is_valid()`, but the proof it mints is then accepted by
`into_validated_delaunay()` without requiring Levels 1-3 validation (Tds,
topology, geometric). Refactor `validate_delaunay_property()` to perform the
same comprehensive multi-layer validation as currently exists in
`try_into_validated_delaunay()`: call `self.candidate.tri.validate()` to check
TDS and triangulation invariants, then perform conditional geometric validation
based on topology (call `is_delaunay_property_only()` for euclidean or
`is_valid()` otherwise). This ensures the minted proof represents full
validation across all layers, preventing callers from accidentally skipping
Levels 1-3 when using `into_validated_delaunay()`.
---
Outside diff comments:
In `@src/core/algorithms/flips.rs`:
- Around line 11185-11532: Replace all instances of the legacy
`assert!(matches!(...))` pattern with the stabilized `assert_matches!` macro
throughout the test functions in this section, including those in
test_orient_replacement_simplices_rejects_conflicting_periodic_external_facet_translations,
test_orient_replacement_simplices_rejects_periodic_offset_count_mismatch,
test_orient_replacement_simplices_rejects_missing_replacement_periodic_offsets,
test_orient_replacement_simplices_rejects_replacement_periodic_offset_length_mismatch,
test_orient_replacement_simplices_rejects_missing_external_simplex,
test_replacement_orientation_helpers_cover_error_paths,
test_set_flip_assignment_rejects_conflicts_and_invalid_indices,
test_orient_replacement_simplices_aligns_external_and_internal_facets, and
test_validate_replacement_orientation_rejects_bad_geometry. Remove the
`assert!(` wrapper and closing parenthesis from each `assert!(matches!(...))`
call, converting it to `assert_matches!(...)` format with the matches pattern as
the primary argument.
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 763-769: In the match arms that convert
TriangulationConstructionError variants to
InitialSimplexConstructionError::UnexpectedInsertionStage (around lines 763-769
and 777-783), replace the `..` pattern that discards fields with explicit
capture of the context field. Instead of dropping the validation context,
extract it from the TriangulationConstructionError and pass it into the
corresponding InitialSimplexUnexpectedInsertionStage variant so that callers can
recover whether the failure originated from post-insertion validation, local
repair, final validation, or other stages.
- Around line 72-91: The HullExtensionReason enum stores structured errors in
the PredicateFailed variant (containing CoordinateConversionError) and the Tds
variant (containing TdsError), but these fields lack the #[source] attribute.
This prevents the error source chain from being properly exposed when callers
use std::error::Error::source(). Add #[source] attributes to the
CoordinateConversionError field in the PredicateFailed variant and the TdsError
field in the Tds variant to ensure the underlying causes are accessible through
the standard error source chain.
In `@src/core/orientation.rs`:
- Around line 615-623: Replace the assert!(matches!(...)) pattern in the
assertion block that checks for
TdsError::Geometric(GeometricError::NegativeOrientation { message }) with the
standard-library assert_matches! macro. The pattern guard conditions checking
for "negative geometric orientation" and "vertices" in the message should be
preserved, and the error message parameter should be moved to the second
argument of assert_matches! macro for consistency with the rest of the codebase.
---
Nitpick comments:
In `@src/core/tds.rs`:
- Around line 8076-8079: The assert!(matches!(...)) pattern used in multiple
locations (lines 8076-8077, 8097-8098, 11157-11158, 11208-11209, 11225-11226,
and 11236-11237) should be converted to the assert_matches! macro for
consistency with the existing ~60 uses in this file. For each occurrence where
you have assert!(matches!(result, pattern), message), replace it with
assert_matches!(result, pattern, message) to use the dedicated macro instead of
the generic assert with matches pattern matching.
In `@src/delaunay/construction.rs`:
- Around line 5055-5067: The synthetic_delaunay_verification_error function
accepts a message parameter but immediately discards it with let _ = message;,
which is misleading since callers will think their input affects the returned
error. Remove the unused message parameter from the function signature of
synthetic_delaunay_verification_error and update all call sites to not pass a
string argument.
🪄 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: 183d1efa-805e-40a2-8f72-140edc22bead
📒 Files selected for processing (33)
docs/ORIENTATION_SPEC.mddocs/dev/rust.mddocs/roadmap.mddocs/validation.mdsemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/collections/spatial_hash_grid.rssrc/core/construction.rssrc/core/insertion.rssrc/core/orientation.rssrc/core/repair.rssrc/core/tds.rssrc/core/tds_snapshot.rssrc/core/util/canonical_points.rssrc/core/validation.rssrc/core/vertex.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/delaunayize.rssrc/delaunay/flips.rssrc/delaunay/insertion.rssrc/delaunay/repair.rssrc/delaunay/validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/point.rssrc/geometry/util/triangulation_generation.rssrc/lib.rstests/delaunayize_workflow.rstests/euler_characteristic.rstests/prelude_exports.rstests/semgrep/src/project_rules/rust_style.rstests/triangulation_builder.rs
Closes #443 - Split final topology and final Delaunay validation contexts so callers can handle Level 4 failures without parsing display text. - Preserve typed source chains for construction, insertion, hull-extension, and repair postcondition failures. - Require checked TDS reconstruction to consume validation proofs before returning Delaunay wrappers. - Align examples and developer guidance with fallible vertex construction and typed error propagation. BREAKING CHANGE: final Delaunay validation failures now use FinalDelaunayValidationContext instead of the topology-validation context, and affected construction error variants expose additional typed context/source structure.
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 (2)
src/core/algorithms/incremental_insertion.rs (2)
83-87: 🛠️ Refactor suggestion | 🟠 Major | 🏗️ Heavy liftKeep
InvalidPatchpayloads typed too.
InvalidPatch { details: String }still forces callers to parse deterministic cases like boundary-edge split counts and visible-patch counters. Since this PR is removing stringly error surfaces, split these into variant-specific payloads such asBoundaryEdgeSplitFacetCount { expected, actual },MultipleBoundaryEdgeSplitFacets, andDisconnectedVisiblePatch { boundary_ridges, ridge_fans, components, boundary_components, boundary_subface_nonmanifold }.Possible shape for the typed payload
pub enum HullExtensionReason { NoVisibleFacets, - InvalidPatch { - /// Details about why the patch was invalid. - details: String, - }, + InvalidPatch { + /// Structured reason the visible boundary patch was invalid. + reason: InvalidHullPatchReason, + }, PredicateFailed(#[source] CoordinateConversionError), Tds(#[source] TdsError), } + +#[derive(Debug, Clone, thiserror::Error, PartialEq, Eq)] +#[non_exhaustive] +pub enum InvalidHullPatchReason { + #[error("2D boundary edge split expected {expected} facets, got {actual}")] + BoundaryEdgeSplitFacetCount { expected: usize, actual: usize }, + #[error("2D boundary edge split matched multiple facets")] + MultipleBoundaryEdgeSplitFacets, + #[error( + "invalid visible patch: boundary_ridges={boundary_ridges}, ridge_fans={ridge_fans}, components={components}, boundary_components={boundary_components}, boundary_subface_nonmanifold={boundary_subface_nonmanifold}" + )] + DisconnectedVisiblePatch { + boundary_ridges: usize, + ridge_fans: usize, + components: usize, + boundary_components: usize, + boundary_subface_nonmanifold: usize, + }, +}Also applies to: 3903-3906, 4541-4546
🤖 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 83 - 87, The InvalidPatch error variant in the error enum currently uses a generic details: String field that forces callers to parse error messages as strings for deterministic cases. Replace the current InvalidPatch variant with multiple, more specific typed variants: BoundaryEdgeSplitFacetCount with expected and actual fields for split count mismatches, MultipleBoundaryEdgeSplitFacets for multiple boundary edge split scenarios, and DisconnectedVisiblePatch with typed fields for boundary_ridges, ridge_fans, components, boundary_components, and boundary_subface_nonmanifold to provide structured error information. Update all callers that currently construct or match on InvalidPatch to use these new typed variants instead of string-based error details.
4856-4864:⚠️ Potential issue | 🟡 MinorFinish the
assert_matches!migration here.This remaining
assert!(matches!(...))call at lines 4856–4864 should use the importedassert_matches!macro directly, consistent with the other tests in this module (lines 4723, 4889, 4918, etc.) and the PR objective.Proposed test cleanup
- assert!( - matches!( - result, - Err(InsertionError::CavityFilling { - reason: CavityFillingError::MissingInsertedVertex { .. }, - }) - ), - "Expected CavityFilling error, got: {result:?}" - ); + assert_matches!( + result, + Err(InsertionError::CavityFilling { + reason: CavityFillingError::MissingInsertedVertex { .. }, + }) + );🤖 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 4856 - 4864, Replace the assert!(matches!(...)) pattern at lines 4856–4864 with the assert_matches! macro directly. The current code uses a verbose nested assert and matches approach, but the module already imports and uses assert_matches! macro in other tests (such as at lines 4723, 4889, and 4918). Convert this to use the assert_matches! macro with the same error pattern matching logic for InsertionError::CavityFilling with CavityFillingError::MissingInsertedVertex to maintain consistency across the test suite.
🧹 Nitpick comments (1)
src/core/validation.rs (1)
1302-1307: ⚡ Quick winUpdate the conversion comment to match the typed source payload.
Line 1306 still documents the old string payload, while the code now preserves
source: dt_err.Proposed cleanup
- /// - `InvariantError::Delaunay(e)` → `InsertionError::DelaunayValidationFailed { message }` + /// - `InvariantError::Delaunay(e)` → `InsertionError::DelaunayValidationFailed { source: e }`🤖 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/validation.rs` around lines 1302 - 1307, Update the documentation comment for the invariant_error_to_insertion_error function to accurately reflect the current conversion behavior. The comment on line 1306 currently describes the Delaunay error conversion as using a message field, but the actual implementation now uses a typed source field to preserve the error object. Revise the comment to show that InvariantError::Delaunay(e) converts to InsertionError::DelaunayValidationFailed with source: e, matching the actual typed source payload used in the function implementation.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/delaunay/validation.rs`:
- Around line 89-95: The method `into_structurally_valid_delaunay()` currently
accepts only structural validation proof (`TdsStructureValidationProof`) while
returning the fully finalized `DelaunayTriangulation` wrapper, which
misrepresents the validation level and invites misuse. Either modify the method
to require the full `DelaunayTriangulationValidationProof` parameter instead of
`TdsStructureValidationProof` to match the finalized return type, or change the
return type to return an intermediate candidate or repair-only type that
represents only structural validity. Alternatively, add clear documentation
marking this method as internal-only repair machinery not intended for general
use.
---
Outside diff comments:
In `@src/core/algorithms/incremental_insertion.rs`:
- Around line 83-87: The InvalidPatch error variant in the error enum currently
uses a generic details: String field that forces callers to parse error messages
as strings for deterministic cases. Replace the current InvalidPatch variant
with multiple, more specific typed variants: BoundaryEdgeSplitFacetCount with
expected and actual fields for split count mismatches,
MultipleBoundaryEdgeSplitFacets for multiple boundary edge split scenarios, and
DisconnectedVisiblePatch with typed fields for boundary_ridges, ridge_fans,
components, boundary_components, and boundary_subface_nonmanifold to provide
structured error information. Update all callers that currently construct or
match on InvalidPatch to use these new typed variants instead of string-based
error details.
- Around line 4856-4864: Replace the assert!(matches!(...)) pattern at lines
4856–4864 with the assert_matches! macro directly. The current code uses a
verbose nested assert and matches approach, but the module already imports and
uses assert_matches! macro in other tests (such as at lines 4723, 4889, and
4918). Convert this to use the assert_matches! macro with the same error pattern
matching logic for InsertionError::CavityFilling with
CavityFillingError::MissingInsertedVertex to maintain consistency across the
test suite.
---
Nitpick comments:
In `@src/core/validation.rs`:
- Around line 1302-1307: Update the documentation comment for the
invariant_error_to_insertion_error function to accurately reflect the current
conversion behavior. The comment on line 1306 currently describes the Delaunay
error conversion as using a message field, but the actual implementation now
uses a typed source field to preserve the error object. Revise the comment to
show that InvariantError::Delaunay(e) converts to
InsertionError::DelaunayValidationFailed with source: e, matching the actual
typed source payload used in the function implementation.
🪄 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: 2f4521ce-1e1b-4fb8-8c9b-12ff44668f8b
📒 Files selected for processing (23)
README.mddocs/api_design.mddocs/dev/rust.mddocs/diagnostics.mddocs/numerical_robustness_guide.mddocs/topology.mddocs/validation.mddocs/workflows.mdsemgrep.yamlsrc/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/construction.rssrc/core/orientation.rssrc/core/query.rssrc/core/tds.rssrc/core/validation.rssrc/delaunay/builder.rssrc/delaunay/construction.rssrc/delaunay/query.rssrc/delaunay/validation.rssrc/lib.rstests/prelude_exports.rstests/semgrep/src/project_rules/rust_style.rs
✅ Files skipped from review due to trivial changes (5)
- docs/topology.md
- README.md
- docs/numerical_robustness_guide.md
- docs/diagnostics.md
- docs/dev/rust.md
🚧 Files skipped from review as they are similar to previous changes (8)
- src/core/orientation.rs
- semgrep.yaml
- tests/prelude_exports.rs
- tests/semgrep/src/project_rules/rust_style.rs
- docs/validation.md
- src/delaunay/construction.rs
- src/delaunay/builder.rs
- src/core/tds.rs
- Replace string-backed hull-extension patch failures with orthogonal typed
variants for boundary-edge split counts, duplicate split facets, and
disconnected visible patches.
- Preserve those categories through flip-neighbor repair diagnostics and export
the summary types through the public flips facade and repair prelude.
- Clarify that structural Delaunay conversion is internal repair machinery until
full validation proof types can model topology-specific wrappers.
BREAKING CHANGE: `HullExtensionReason::InvalidPatch { details }` is replaced by `BoundaryEdgeSplitFacetCount`, `MultipleBoundaryEdgeSplitFacets`, and `DisconnectedVisiblePatch`.
BREAKING CHANGE: `FlipNeighborHullExtensionFailureKind::InvalidPatch` is replaced by typed summary variants for boundary-edge split counts, multiple split facets, and disconnected visible patches.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
src/core/algorithms/flips.rs (1)
14821-14834: ⚡ Quick winCover the other new hull-extension mappings too.
This test only exercises
DisconnectedVisiblePatch, but lines 3057-3063 added three new conversion arms. Add assertions forBoundaryEdgeSplitFacetCountandMultipleBoundaryEdgeSplitFacetshere so regressions in the typed boundary-edge summaries fail close to the conversion.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/flips.rs` around lines 14821 - 14834, The current test only validates the conversion of the DisconnectedVisiblePatch variant in the FlipNeighborHullExtensionFailureKind::from conversion, but three new conversion arms were added for HullExtensionReason (specifically BoundaryEdgeSplitFacetCount and MultipleBoundaryEdgeSplitFacets). Add two additional test cases following the same pattern as the existing DisconnectedVisiblePatch test: create instances of HullExtensionReason with appropriate values for each of these variants, call FlipNeighborHullExtensionFailureKind::from on them, and assert both that the resulting hull_kind matches the expected enum variant and that the to_string() output produces the correct string representation.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.
Nitpick comments:
In `@src/core/algorithms/flips.rs`:
- Around line 14821-14834: The current test only validates the conversion of the
DisconnectedVisiblePatch variant in the
FlipNeighborHullExtensionFailureKind::from conversion, but three new conversion
arms were added for HullExtensionReason (specifically
BoundaryEdgeSplitFacetCount and MultipleBoundaryEdgeSplitFacets). Add two
additional test cases following the same pattern as the existing
DisconnectedVisiblePatch test: create instances of HullExtensionReason with
appropriate values for each of these variants, call
FlipNeighborHullExtensionFailureKind::from on them, and assert both that the
resulting hull_kind matches the expected enum variant and that the to_string()
output produces the correct string representation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 61bbd56e-c803-414b-98b4-1354136980f4
📒 Files selected for processing (6)
src/core/algorithms/flips.rssrc/core/algorithms/incremental_insertion.rssrc/core/validation.rssrc/delaunay/flips.rssrc/delaunay/validation.rssrc/lib.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- src/delaunay/flips.rs
- src/lib.rs
- src/delaunay/validation.rs
- Cover all hull-extension repair failure kind conversions with typed variant assertions. - Keep explicit-construction doctests panic-free while matching structured error variants. - Document concrete Delaunay repair error variants for flip repair APIs. - Allow idiomatic fallible `from_str` parser names in constructor-name guardrails. Closes #443
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes #443
CLOSES #329