ci: quiet Codacy code scanning noise#356
Merged
acgetchell merged 5 commits intomainfrom Apr 30, 2026
Merged
Conversation
- Skip empty Codacy SARIF runs before uploading results to GitHub Code Scanning. - Limit Codacy code scanning uploads to repository-owned OpenGrep analysis. - Migrate triangulation doctests to focused prelude imports for #355. - Add Semgrep coverage to keep triangulation doctest imports on focused preludes. - Clarify vertex point-constructor docs and avoid UUID-bearing facet assertion output.
Contributor
|
Warning Rate limit exceeded
To keep reviews running without waiting, you can enable usage-based add-on for your organization. This allows additional reviews beyond the hourly cap. Account admins can enable it under billing. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (1)
WalkthroughUpdates crate prelude/re-exports and migrates many rustdoc/examples to focused Changes
Sequence Diagram(s)sequenceDiagram
participant Workflow as "GitHub Workflow"
participant Runner as "Matrix Job (opengrep)"
participant Splitter as "sarif_splitter.py (action)"
participant Codacy as "Codacy API"
Workflow->>Runner: run analysis (opengrep) → produce SARIF
Runner->>Splitter: provide SARIF file(s)
Splitter->>Splitter: validate `runs` (error if not a list)
Splitter->>Splitter: if `runs` empty → print message (no uploads)
Splitter->>Splitter: iterate runs, skip runs lacking `tool.driver.rules` and `results`
Splitter->>Splitter: write split SARIFs and count uploadable runs
alt uploadable runs > 0
Splitter->>Workflow: set CODACY_HAS_UPLOADABLE_SARIF=true
Workflow->>Codacy: upload split SARIFs with `category` (matrix tool)
Codacy-->>Workflow: upload ack/result
else no uploadable runs
Note right of Workflow: upload step skipped (flag false)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 28 minutes and 18 seconds.Comment |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
🟢 Coverage 100.00% diff coverage · 0.00% coverage variation
Metric Results Coverage variation ✅ 0.00% coverage variation (-1.00%) Diff coverage ✅ 100.00% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (9d51d30) 51562 46331 89.85% Head commit (f86a877) 51549 (-13) 46319 (-12) 89.85% (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 (#356) 2 2 100.00% Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified:
<covered lines added or modified>/<coverable lines added or modified> * 100%
NEW Get contextual insights on your PRs based on Codacy's metrics, along with PR and Jira context, without leaving GitHub. Enable AI reviewer
TIP This summary will be updated as you push new changes.
coderabbitai
Bot
added
documentation
![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: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
.github/workflows/codacy.yml (1)
121-123:⚠️ Potential issue | 🟠 Major | ⚡ Quick winHandle zero-run SARIF as a non-fatal case.
This branch still fails the workflow when
runsis empty. For the “quiet noise” objective, empty SARIF should skip upload and continue successfully.Proposed patch
- runs = sarif.get("runs") - if not isinstance(runs, list) or not runs: - sys.exit("Codacy SARIF did not contain any runs to upload") + runs = sarif.get("runs") + if not isinstance(runs, list): + sys.exit("Codacy SARIF did not contain a valid runs array")With this change, the existing
uploadable_count == 0path (Line 169-170) cleanly handles empty runs and keeps the job green.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In @.github/workflows/codacy.yml around lines 121 - 123, The current check exits the workflow when "runs" is empty; change it so a missing or non-list still errors but an empty list is treated as non-fatal: keep the isinstance check to sys.exit on non-list/nonexistent SARIF, but if runs is an empty list do not call sys.exit — instead log/print a message and return/continue so the rest of the script can hit the existing uploadable_count == 0 branch that cleanly skips upload and exits successfully; adjust the block that references the variable "runs" accordingly so empty "runs" flows through rather than terminating.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@semgrep.yaml`:
- Around line 383-386: The rule scope in semgrep.yaml currently restricts
doctest enforcement to "/src/triangulation/**/*.rs"; update the paths.include
entry to broaden the pattern to cover all Rust source files (e.g., change
"/src/triangulation/**/*.rs" to "/src/**/*.rs") so doctest checks apply
repository-wide while keeping the existing test include
("/tests/semgrep/src/project_rules/**/*.rs"); this ensures modules beyond
Triangulation are also covered.
- Around line 388-389: Update the semgrep rule in semgrep.yaml so the positive
regex (pattern-regex) also matches doctest hidden imports prefixed with "# "
(e.g. "/// # use ..."); specifically change the pattern-regex used in the diff
to allow an optional "#\s*" after the doc-comment marker (so it matches
'^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::(core|geometry|triangulation|topology)::'),
keep the pattern-not-regex (pattern-not-regex:
'^\s*//[/!]\s+use\s+delaunay::prelude::') as-is, and verify the updated rule
flags the hidden doctest imports (e.g., the instances in
src/geometry/algorithms/convex_hull.rs).
---
Outside diff comments:
In @.github/workflows/codacy.yml:
- Around line 121-123: The current check exits the workflow when "runs" is
empty; change it so a missing or non-list still errors but an empty list is
treated as non-fatal: keep the isinstance check to sys.exit on
non-list/nonexistent SARIF, but if runs is an empty list do not call sys.exit —
instead log/print a message and return/continue so the rest of the script can
hit the existing uploadable_count == 0 branch that cleanly skips upload and
exits successfully; adjust the block that references the variable "runs"
accordingly so empty "runs" flows through rather than terminating.
🪄 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: e5d7d76d-8fc5-4c8b-8581-9a981de5f972
📒 Files selected for processing (10)
.github/workflows/codacy.ymlCHANGELOG.mdsemgrep.yamlsrc/core/facet.rssrc/core/vertex.rssrc/lib.rssrc/triangulation/builder.rssrc/triangulation/delaunay.rssrc/triangulation/delaunayize.rstests/semgrep/src/project_rules/rust_style.rs
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #356 +/- ##
==========================================
- Coverage 89.83% 89.83% -0.01%
==========================================
Files 58 58
Lines 51367 51354 -13
==========================================
- Hits 46147 46135 -12
+ Misses 5220 5219 -1
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
- Add minimal, task-focused preludes for triangulation, TDS, geometry, queries, generators, ordering, collections, and topology workflows. - Update doctests, examples, integration tests, and benchmarks to use focused preludes instead of deep internal imports. - Expand the Semgrep doctest import rule to cover all Rust source files and hidden doctest imports. - Harden Codacy SARIF handling so empty analyses skip upload cleanly instead of creating noisy code-scanning results. - Document the new prelude structure and update guides to use public prelude imports.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
semgrep.yaml (1)
388-389:⚠️ Potential issue | 🟠 Major | ⚡ Quick winMirror the hidden-doctest prefix in the exclusion regex.
pattern-regexnow matches/// # use ..., butpattern-not-regexstill only exempts plain/// use delaunay::prelude::...lines. Hidden prelude imports will therefore be flagged as violations.Suggested fix
- - pattern-not-regex: '^\s*//[/!]\s+use\s+delaunay::prelude::' + - pattern-not-regex: '^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::prelude::'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@semgrep.yaml` around lines 388 - 389, The exclusion regex in pattern-not-regex must mirror the hidden-doctest prefix allowed by pattern-regex so hidden prelude imports like "/// # use delaunay::prelude::..." are exempted; update the pattern-not-regex to accept an optional "#\s*" after the comment slashes (i.e., the same '^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::prelude::' style) so it matches both plain and hidden doctest prelude import lines.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Around line 951-953: Replace profile-gated cfgs with the documented feature
flag: wherever lib.rs currently uses #[cfg(any(test, debug_assertions))] guard
around the public export of debug_print_first_delaunay_violation (and related
prelude exports), change it to #[cfg(any(test, feature = "test-debug"))]; and
wherever it uses #[cfg(debug_assertions)] for
verify_conflict_region_completeness (or similar debug-only exports), change that
to #[cfg(feature = "test-debug")]; ensure the pub use lines referencing
debug_print_first_delaunay_violation and verify_conflict_region_completeness
(and any other debug-only prelude exports) are updated so downstream API
visibility is controlled by the "test-debug" feature rather than build profile.
---
Duplicate comments:
In `@semgrep.yaml`:
- Around line 388-389: The exclusion regex in pattern-not-regex must mirror the
hidden-doctest prefix allowed by pattern-regex so hidden prelude imports like
"/// # use delaunay::prelude::..." are exempted; update the pattern-not-regex to
accept an optional "#\s*" after the comment slashes (i.e., the same
'^\s*//[/!]\s*(?:#\s*)?use\s+delaunay::prelude::' style) so it matches both
plain and hidden doctest prelude import lines.
🪄 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: c8a6770f-dccd-4bb8-9b19-b97a25a879f7
📒 Files selected for processing (65)
.github/workflows/codacy.ymlREADME.mddocs/api_design.mddocs/dev/rust.mddocs/numerical_robustness_guide.mddocs/workflows.mdexamples/memory_analysis.rsexamples/zero_allocation_iterator_demo.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/cell.rssrc/core/collections/aliases.rssrc/core/collections/buffers.rssrc/core/collections/helpers.rssrc/core/collections/key_maps.rssrc/core/collections/triangulation_maps.rssrc/core/edge.rssrc/core/facet.rssrc/core/operations.rssrc/core/tds.rssrc/core/traits/data_type.rssrc/core/traits/facet_cache.rssrc/core/triangulation.rssrc/core/util/deduplication.rssrc/core/util/delaunay_validation.rssrc/core/util/facet_keys.rssrc/core/util/facet_utils.rssrc/core/util/hashing.rssrc/core/util/jaccard.rssrc/core/util/measurement.rssrc/core/util/uuid.rssrc/core/vertex.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/kernel.rssrc/geometry/matrix.rssrc/geometry/point.rssrc/geometry/predicates.rssrc/geometry/quality.rssrc/geometry/robust_predicates.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/euclidean.rssrc/topology/spaces/spherical.rssrc/topology/spaces/toroidal.rssrc/topology/traits/topological_space.rssrc/triangulation/delaunay.rstests/euler_characteristic.rstests/large_scale_debug.rstests/proptest_triangulation.rstests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rstests/storage_backend_compatibility.rs
💤 Files with no reviewable changes (1)
- docs/api_design.md
✅ Files skipped from review due to trivial changes (57)
- examples/memory_analysis.rs
- src/core/traits/data_type.rs
- tests/serialization_vertex_preservation.rs
- tests/storage_backend_compatibility.rs
- docs/workflows.md
- examples/zero_allocation_iterator_demo.rs
- src/core/edge.rs
- docs/numerical_robustness_guide.md
- src/geometry/matrix.rs
- src/core/adjacency.rs
- tests/proptest_triangulation.rs
- src/topology/spaces/spherical.rs
- tests/euler_characteristic.rs
- src/topology/spaces/toroidal.rs
- README.md
- src/topology/spaces/euclidean.rs
- src/geometry/util/norms.rs
- src/core/traits/facet_cache.rs
- src/core/collections/buffers.rs
- src/topology/characteristics/validation.rs
- src/core/algorithms/pl_manifold_repair.rs
- src/geometry/point.rs
- src/core/util/measurement.rs
- src/core/util/hashing.rs
- src/core/util/facet_keys.rs
- src/core/util/delaunay_validation.rs
- src/geometry/quality.rs
- src/core/util/facet_utils.rs
- src/core/collections/key_maps.rs
- src/topology/traits/topological_space.rs
- src/geometry/util/measures.rs
- src/core/util/uuid.rs
- src/geometry/robust_predicates.rs
- docs/dev/rust.md
- src/geometry/util/conversions.rs
- src/core/collections/helpers.rs
- src/core/util/jaccard.rs
- src/core/collections/triangulation_maps.rs
- src/geometry/util/circumsphere.rs
- src/geometry/predicates.rs
- src/topology/manifold.rs
- src/core/collections/aliases.rs
- src/geometry/util/triangulation_generation.rs
- src/topology/characteristics/euler.rs
- src/geometry/kernel.rs
- src/core/operations.rs
- src/core/cell.rs
- src/core/algorithms/incremental_insertion.rs
- src/core/triangulation.rs
- src/geometry/util/point_generation.rs
- src/core/vertex.rs
- src/core/algorithms/flips.rs
- src/core/tds.rs
- src/geometry/traits/coordinate.rs
- src/core/util/deduplication.rs
- src/core/algorithms/locate.rs
- src/geometry/algorithms/convex_hull.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/semgrep/src/project_rules/rust_style.rs
- .github/workflows/codacy.yml
- Replace profile-based public diagnostic exports with the documented test-debug feature. - Keep debug-only helper definitions and diagnostic call sites aligned with feature-gated API visibility. - Allow hidden doctest prelude imports in the Semgrep prelude-import rule.
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)
tests/conflict_region_verification.rs (1)
68-91:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winDon't count non-verifying insertions as verified.
The
#[cfg(not(feature = "test-debug"))]branch still incrementsinsertions_checked, so the finalinsertions_checked > 0assertion can pass even whenverify_conflict_region_completeness(...)never ran. That makes this smoke test succeed on the non-diagnostic path.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/conflict_region_verification.rs` around lines 68 - 91, The test currently increments insertions_checked in both branches so the count can be non-zero even when verify_conflict_region_completeness(...) never ran; change the logic so insertions_checked is only incremented when verification runs: move the insertions_checked += 1 into the #[cfg(feature = "test-debug")] branch immediately after calling verify_conflict_region_completeness(...) (and remove the increment from the #[cfg(not(feature = "test-debug"))] branch), keeping the let _ = conflict_cells; there to avoid unused-variable warnings so the final assertion only succeeds when verify_conflict_region_completeness actually executed.
🧹 Nitpick comments (1)
src/core/triangulation.rs (1)
1267-1270: ⚡ Quick winFinish the doctest prelude migration in these two snippets
These examples still use deep
delaunay::geometry::kernel::FastKernelin the type path even after switching imports to focused preludes. Prefer the imported prelude symbol for consistency.Suggested doc-only cleanup
-/// let count = Triangulation::<delaunay::geometry::kernel::FastKernel<f64>, (), (), 3> +/// use delaunay::prelude::geometry::FastKernel; +/// let count = Triangulation::<FastKernel<f64>, (), (), 3> /// ::topology_safety_net_star_split_fallback_successes();-/// let metrics = Triangulation::<delaunay::geometry::kernel::FastKernel<f64>, (), (), 3> +/// use delaunay::prelude::geometry::FastKernel; +/// let metrics = Triangulation::<FastKernel<f64>, (), (), 3> /// ::duplicate_detection_metrics();Also applies to: 1287-1290
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/core/triangulation.rs` around lines 1267 - 1270, The doctest examples still reference the full kernel path; replace the deep path with the prelude-imported symbol so the examples use the focused prelude. Specifically, update the Triangulation instantiation in the examples that call Triangulation::<delaunay::geometry::kernel::FastKernel<f64>, (), (), 3>::topology_safety_net_star_split_fallback_successes() (and the corresponding snippet at the later location) to use the prelude-imported kernel type (the symbol brought in by the prelude import) instead of delaunay::geometry::kernel::FastKernel<f64>, and keep the rest of the generic args and method call unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/lib.rs`:
- Around line 1011-1014: The prelude::geometry change removed
previously-exported items (e.g., ConvexHull, generate_random_points_*,
RandomTriangulationBuilder) breaking downstream code; restore backward
compatibility by re-exporting the original symbols from the geometry module
(either re-export the full crate::geometry::* or explicitly include ConvexHull,
the generate_random_points_* functions, RandomTriangulationBuilder alongside the
new query and generators preludes) so that prelude::geometry still exposes the
prior public API while keeping the added query/generators exports.
---
Outside diff comments:
In `@tests/conflict_region_verification.rs`:
- Around line 68-91: The test currently increments insertions_checked in both
branches so the count can be non-zero even when
verify_conflict_region_completeness(...) never ran; change the logic so
insertions_checked is only incremented when verification runs: move the
insertions_checked += 1 into the #[cfg(feature = "test-debug")] branch
immediately after calling verify_conflict_region_completeness(...) (and remove
the increment from the #[cfg(not(feature = "test-debug"))] branch), keeping the
let _ = conflict_cells; there to avoid unused-variable warnings so the final
assertion only succeeds when verify_conflict_region_completeness actually
executed.
---
Nitpick comments:
In `@src/core/triangulation.rs`:
- Around line 1267-1270: The doctest examples still reference the full kernel
path; replace the deep path with the prelude-imported symbol so the examples use
the focused prelude. Specifically, update the Triangulation instantiation in the
examples that call Triangulation::<delaunay::geometry::kernel::FastKernel<f64>,
(), (), 3>::topology_safety_net_star_split_fallback_successes() (and the
corresponding snippet at the later location) to use the prelude-imported kernel
type (the symbol brought in by the prelude import) instead of
delaunay::geometry::kernel::FastKernel<f64>, and keep the rest of the generic
args and method call 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: 310536c6-b118-49eb-8782-0369872fff85
📒 Files selected for processing (9)
semgrep.yamlsrc/core/algorithms/locate.rssrc/core/triangulation.rssrc/core/util/delaunay_validation.rssrc/lib.rstests/conflict_region_verification.rstests/delaunay_edge_cases.rstests/regressions.rstests/semgrep/src/project_rules/rust_style.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- src/core/util/delaunay_validation.rs
- src/core/algorithms/locate.rs
Comment on lines
1011
to
+1014
| pub use crate::geometry::{ | ||
| algorithms::*, kernel::*, matrix::*, point::*, predicates::*, quality::*, | ||
| robust_predicates::*, traits::coordinate::*, util::*, | ||
| kernel::*, matrix::*, point::*, predicates::*, quality::*, robust_predicates::*, | ||
| traits::coordinate::*, util::circumsphere::*, util::conversions::*, util::measures::*, | ||
| util::norms::*, |
Contributor
There was a problem hiding this comment.
Keep prelude::geometry backward-compatible.
Narrowing this module to selected submodules silently removes existing public imports like ConvexHull, generate_random_points_*, and RandomTriangulationBuilder from delaunay::prelude::geometry::*. The new query and generators preludes are good additions, but this still breaks downstream code that already uses the old focused path.
♻️ Compatibility-safe re-export
pub mod geometry {
pub use crate::geometry::{
- kernel::*, matrix::*, point::*, predicates::*, quality::*, robust_predicates::*,
- traits::coordinate::*, util::circumsphere::*, util::conversions::*, util::measures::*,
- util::norms::*,
+ algorithms::*, kernel::*, matrix::*, point::*, predicates::*, quality::*,
+ robust_predicates::*, traits::coordinate::*, util::circumsphere::*,
+ util::conversions::*, util::measures::*, util::norms::*,
+ util::point_generation::*, util::triangulation_generation::*,
};
}As per coding guidelines "New functionality must be additive: use crate::prelude::* for ergonomic re-exports; never silently rename or remove a public item in Rust"
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@src/lib.rs` around lines 1011 - 1014, The prelude::geometry change removed
previously-exported items (e.g., ConvexHull, generate_random_points_*,
RandomTriangulationBuilder) breaking downstream code; restore backward
compatibility by re-exporting the original symbols from the geometry module
(either re-export the full crate::geometry::* or explicitly include ConvexHull,
the generate_random_points_* functions, RandomTriangulationBuilder alongside the
new query and generators preludes) so that prelude::geometry still exposes the
prior public API while keeping the added query/generators exports.
Owner
Author
There was a problem hiding this comment.
actually, no, we want to keep this narrow and orthogonal and not drag in unrelated functionality. I'll update the guidelines accordingly.
Contributor
There was a problem hiding this comment.
@acgetchell, understood — the narrow, orthogonal scope of prelude::geometry is intentional, with ConvexHull living in prelude::query and the random-generation helpers in prelude::generators. I'll note this design principle for future reviews.
✏️ Learnings added
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 356
File: src/lib.rs:1011-1014
Timestamp: 2026-04-30T15:45:03.086Z
Learning: In acgetchell/delaunay (Rust, src/lib.rs), `prelude::geometry` is intentionally narrow: it exports only geometric types, predicates, kernels, matrix utilities, coordinate traits, and circumsphere/conversion/measure/norm helpers. `ConvexHull` belongs in `prelude::query`; `generate_random_points_*`, `RandomTriangulationBuilder`, and related generators belong in `prelude::generators`. Do not flag the absence of these items from `prelude::geometry` as a backward-compatibility regression.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/**/*.rs : New functionality must be additive: use crate::prelude::* for ergonomic re-exports; never silently rename or remove a public item in Rust
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/core/**/*.rs : Use const-generic D on every core type (DelaunayTriangulation<K, U, V, D>, Tds<T, U, V, D>, Cell<T, U, V, D>, Vertex<T, U, D>, Point<T, D>); no runtime dimension in Rust
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to **/*.rs : Use type and function names matching textbook vocabulary: Triangulation, Vertex, Cell, Facet, Ridge, InSphere, Orientation, insphere, circumcenter, circumradius in Rust code
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 65
File: WARP.md:249-254
Timestamp: 2025-09-04T20:03:49.896Z
Learning: The delaunay repository contains all the specialized integration tests documented in WARP.md: circumsphere_debug_tools.rs, robust_predicates_comparison.rs, convex_hull_bowyer_watson_integration.rs, and allocation_api.rs, plus additional test files like robust_predicates_showcase.rs and coordinate_conversion_errors.rs.
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/**/*.rs : Per-simplex data must be stack-allocated ([T; D] coordinates, SmallBuffer<VertexKey, MAX_PRACTICAL_DIMENSION_SIZE>); triangulation topology stored in SlotMap in Rust
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/**/*.rs : Repair paths (repair_delaunay_with_flips, repair_facet_oversharing, delaunayize_by_flips) must bound work via explicit budgets (max_flips, max_iterations, max_cells_removed) and surface non-convergence as typed error, never by logging and proceeding in Rust
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 265
File: src/core/triangulation.rs:447-451
Timestamp: 2026-03-15T16:33:29.981Z
Learning: Repo acgetchell/delaunay (Rust): In src/core/triangulation.rs, impl From<ManifoldError> for TriangulationValidationError intentionally uses unreachable! for ManifoldError::Tds because TriangulationValidationError is Level 3–only and the companion impl From<ManifoldError> for InvariantError handles the Tds variant first. Do not flag this unreachable! as a panic path; it encodes a proven internal routing invariant.
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/geometry/predicates.rs : Geometric predicates (insphere, insphere_lifted, orientation) must use three-stage pattern: Stage 1 - provable f64 fast filter with Shewchuk-style errbound, Stage 2 - exact sign via Bareiss in la-stack, Stage 3 - deterministic BOUNDARY/DEGENERATE fallback for non-finite inputs
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to **/*.rs : Use const fn for pure-math helpers (sign_to_orientation, sign_to_insphere, coordinate conversions) where inputs allow in Rust code
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/topology/{manifold,characteristics}/**/*.rs : PL-manifold invariants: facets have multiplicity 1 (boundary) or 2 (interior); ridges linked consistently; Euler characteristic matches TopologyGuarantee; checks live in src/topology/manifold.rs and src/topology/characteristics/ in Rust
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The user corrected that finitecheck.rs, hashcoordinate.rs, and orderedeq.rs trait files do not exist in the current version of the delaunay repository under src/geometry/traits/. The current structure only contains coordinate.rs, which matches the documented structure in CONTRIBUTING.md.
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 261
File: src/core/algorithms/incremental_insertion.rs:296-304
Timestamp: 2026-03-14T14:02:19.451Z
Learning: In the `delaunay` crate (`src/core/triangulation.rs`), there are two distinct retry/fallback mechanisms that must not be conflated:
1. Hull extension in-place fallback (`should_fallback`, ~line 4590): operates on the already-mutated TDS within a single insertion attempt; only pre-mutation `ConflictError` variants are safe to retry here.
2. Perturbation retry (`is_retryable()`, ~line 3177): the TDS is rolled back to a snapshot before retrying; any geometrically-sensitive error (including `IsolatedVertex`) is valid to classify as retryable here.
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 55
File: CONTRIBUTING.md:152-167
Timestamp: 2025-08-30T02:40:22.032Z
Learning: The delaunay repository has stale documentation in WARP.md that references non-existent trait files (finitecheck.rs, hashcoordinate.rs, orderedeq.rs) in src/geometry/traits/, while the actual directory only contains coordinate.rs. The CONTRIBUTING.md documentation is accurate and reflects the current state.
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 283
File: benches/PERFORMANCE_RESULTS.md:32-35
Timestamp: 2026-03-24T21:38:39.729Z
Learning: In the `delaunay` crate (acgetchell/delaunay), `benches/PERFORMANCE_RESULTS.md` is auto-generated by running `just bench-perf-summary` against live Criterion output. The wide timing spread in the circumsphere benchmark table is intentional and reflects the implementation:
- `insphere` at 2 ns for boundary-vertex cases = short-circuit return (boundary vertex is a defining vertex of the simplex, no predicate computation needed).
- `insphere_distance` at ~25–85 ns = pure `f64` circumcenter + distance calculation, no exact arithmetic.
- `insphere` at tens of µs in 3D–5D = exact `BigRational` Bareiss fallback triggered by the SoS/exact pipeline (`AdaptiveKernel`).
Do not flag these timings as inconsistent or impossible — they are real Criterion measurements. The review sandbox does not clone `target/criterion`, so scripts checking for that directory will incorrectly conclude fallback data is in use.
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/geometry/{sos,predicates}.rs : Degenerate configurations are resolved by Simulation-of-Simplicity in src/geometry/sos.rs; callers can rely on predicates returning consistent total ordering even on degenerate input
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 273
File: src/core/delaunay_triangulation.rs:4914-4935
Timestamp: 2026-03-17T20:55:12.522Z
Learning: Repo acgetchell/delaunay (Rust): In src/core/delaunay_triangulation.rs, maybe_repair_after_insertion first runs repair_delaunay_with_flips_k2_k3 using K and only falls back to repair_delaunay_with_flips_robust on NonConvergent/PostconditionFailed; there is no pre-repair snapshot beyond the outer insertion-level transactional guard. This is intentional to avoid O(V+C) cloning and keep FastKernel usable; “Ok but wrong” outcomes with FastKernel are an accepted trade-off, and users can opt into DelaunayCheckPolicy::EveryN if they want extra assurance during incremental insertion.
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to src/**/*.rs : Every mutating operation must preserve invariants checked by Tds::is_valid (Level 1–3) and DelaunayTriangulation::is_valid (Level 4) in Rust; fail explicitly rather than leave triangulation in inconsistent state
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 300
File: src/core/triangulation_data_structure.rs:5853-5857
Timestamp: 2026-04-01T22:01:55.156Z
Learning: In this Rust repo, the Vertex type does not provide degree() or incident_cells() methods. When writing code changes or making review suggestions, avoid proposing or using those APIs. If you need the vertex’s “star size” in tests, compute it via the triangulation data structure, e.g. dt.as_triangulation().tds.find_cells_containing_vertex(vertex_key).len().
- Replace the test-debug feature with diagnostics for opt-in diagnostic helpers and verbose test diagnostics. - Add prelude::diagnostics for debug verification helpers while keeping focused preludes narrow and orthogonal. - Move diagnostic helper exports out of unrelated focused preludes and update cfg gates, tests, docs, and just recipes. - Document that focused preludes should favor precise taxonomy over backwards compatibility for unrelated re-exports. BREAKING CHANGE: The test-debug feature and just test-debug recipe were removed; use diagnostics and just test-diagnostics instead.
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@Cargo.toml`:
- Line 50: Add a compatibility alias feature named "test-debug" that forwards to
the existing "diagnostics" feature so downstream builds using --features
test-debug continue to work; update the Cargo.toml features section by adding a
feature entry test-debug = ["diagnostics"] (keeping the existing diagnostics =
[] intact) so "test-debug" enables the diagnostics feature.
In `@tests/conflict_region_verification.rs`:
- Around line 47-54: The test currently conditionally compiles the
mutability/updates and/or the call to verify_conflict_region_completeness behind
#[cfg(feature = "diagnostics")], causing insertions_checked and total_missed to
remain zero and the verification path to be skipped while an assertion later
still expects it to have run; either make the verification unconditional for
this ignored smoke test or gate the entire test on the diagnostics feature. To
fix, ensure insertions_checked and total_missed are declared mutable and updated
regardless of cfg (remove the #[cfg] variants that leave them
non-mutable/non-updated) and call verify_conflict_region_completeness(...)
unconditionally in the test body (or alternatively wrap the entire test function
with #[cfg(feature = "diagnostics")] so the test and its assertions only exist
when diagnostics is enabled); adjust the declarations and the call to
verify_conflict_region_completeness and any assertions that rely on those
counters accordingly.
🪄 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: 9444e12a-dde7-40b1-9955-a63fd57b68c2
📒 Files selected for processing (22)
AGENTS.mdCONTRIBUTING.mdCargo.tomldocs/code_organization.mddocs/dev/debug_env_vars.mddocs/dev/rust.mdjustfilesrc/core/algorithms/locate.rssrc/core/triangulation.rssrc/core/util/delaunay_validation.rssrc/geometry/algorithms/convex_hull.rssrc/geometry/util/measures.rssrc/lib.rstests/README.mdtests/conflict_region_verification.rstests/delaunay_edge_cases.rstests/delaunay_public_api_coverage.rstests/delaunay_repair_fallback.rstests/prelude_exports.rstests/proptest_delaunay_triangulation.rstests/regressions.rstests/storage_backend_compatibility.rs
✅ Files skipped from review due to trivial changes (4)
- docs/dev/debug_env_vars.md
- CONTRIBUTING.md
- tests/proptest_delaunay_triangulation.rs
- docs/dev/rust.md
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/regressions.rs
- src/geometry/util/measures.rs
| bench = [ ] # Enable benchmark-style tests and performance analysis | ||
| bench-logging = [ ] # Enable extra benchmark logging for CI diagnostics | ||
| test-debug = [ ] # Enable verbose debug output in tests for development | ||
| diagnostics = [ ] # Enable opt-in diagnostic helpers and verbose test diagnostics |
Contributor
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
echo "Feature declarations in Cargo.toml:"
rg -nP '^\s*(diagnostics|test-debug)\s*=' Cargo.toml || true
echo
echo "Remaining repository references to test-debug:"
rg -n --hidden --glob '!target/**' '\btest-debug\b' || trueRepository: acgetchell/delaunay
Length of output: 529
🏁 Script executed:
# Check if test-debug appears in any test or benchmark files
rg -t rs '\btest-debug\b|#\[cfg.*test-debug' || echo "No test-debug in code"
# Also check for any conditional compilation around diagnostics
rg -t rs '#\[cfg.*diagnostics' || echo "No diagnostics in code"
# View the full context of the diagnostics feature definition in Cargo.toml
cat -n Cargo.toml | sed -n '45,60p'Repository: acgetchell/delaunay
Length of output: 1057
🏁 Script executed:
# Check CHANGELOG context around test-debug to understand its usage history
cat -n CHANGELOG.md | sed -n '90,105p'
cat -n CHANGELOG.md | sed -n '225,235p'
# Check if there are any documentation files mentioning test-debug or feature flags
fd -t f -e md -e txt . | xargs rg -l 'feature|test-debug|diagnostics' 2>/dev/null | head -10
# Look at README or docs for feature flag documentation
ls -la | grep -i readme || true
cat README.md 2>/dev/null | grep -A 10 -i feature || trueRepository: acgetchell/delaunay
Length of output: 5437
Add test-debug compatibility alias to prevent downstream breakage.
The CHANGELOG documents test-debug as a real, user-facing feature gate (lines 97, 230) used to gate diagnostics tracing. The current diagnostics feature replaces it, but removing test-debug without an alias breaks downstream code and scripts still invoking --features test-debug. Additionally, established guidelines require test-debug as part of the feature set.
Required compatibility patch
diagnostics = [ ] # Enable opt-in diagnostic helpers and verbose test diagnostics
+test-debug = [ "diagnostics" ] # Deprecated compatibility alias; remove in next planned breaking release📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| diagnostics = [ ] # Enable opt-in diagnostic helpers and verbose test diagnostics | |
| diagnostics = [ ] # Enable opt-in diagnostic helpers and verbose test diagnostics | |
| test-debug = [ "diagnostics" ] # Deprecated compatibility alias; remove in next planned breaking release |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@Cargo.toml` at line 50, Add a compatibility alias feature named "test-debug"
that forwards to the existing "diagnostics" feature so downstream builds using
--features test-debug continue to work; update the Cargo.toml features section
by adding a feature entry test-debug = ["diagnostics"] (keeping the existing
diagnostics = [] intact) so "test-debug" enables the diagnostics feature.
Owner
Author
There was a problem hiding this comment.
No this is marked as a breaking change, I don't want an alias.
Contributor
There was a problem hiding this comment.
@acgetchell, understood! Since this is explicitly marked as a breaking change with proper notation, the alias is not needed and my concern doesn't apply here. I'll remember this for future reviews.
✏️ Learnings added
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 356
File: Cargo.toml:50-50
Timestamp: 2026-04-30T16:16:32.083Z
Learning: In acgetchell/delaunay (Rust, Cargo.toml), the rename of `test-debug` feature to `diagnostics` is an intentional breaking change (documented in CHANGELOG with breaking change notation). Do not flag the removal of `test-debug` or suggest a compatibility alias — there is no backward-compatibility requirement for this feature flag.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
🧠 Learnings used
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to {tests,benches}/**/*.rs : Gate non-essential test/benchmark diagnostics behind feature flags using test-debug for test diagnostics and bench-logging for benchmark diagnostics in Rust code
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to Cargo.toml : Feature flags must isolate optional dependency weight; default builds stay dep-minimal; use flags: dense-slotmap (default), count-allocations, bench, bench-logging, test-debug, slow-tests in Rust Cargo.toml
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to **/*.rs : Use tracing::{debug,info,warn,error}! for committed diagnostics across production code, tests, and benchmarks, especially for library/runtime code and non-trivial invariant debugging in Rust
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Applies to benches/**/*.rs : Never log inside hot benchmark loops or Criterion-measured closures; emit setup/summary diagnostics outside the measured path instead in Rust benchmarks
Learnt from: acgetchell
Repo: acgetchell/delaunay PR: 356
File: src/lib.rs:1011-1014
Timestamp: 2026-04-30T15:45:03.086Z
Learning: In acgetchell/delaunay (Rust, src/lib.rs), `prelude::geometry` is intentionally narrow: it exports only geometric types, predicates, kernels, matrix utilities, coordinate traits, and circumsphere/conversion/measure/norm helpers. `ConvexHull` belongs in `prelude::query`; `generate_random_points_*`, `RandomTriangulationBuilder`, and related generators belong in `prelude::generators`. Do not flag the absence of these items from `prelude::geometry` as a backward-compatibility regression.
Learnt from: CR
Repo: acgetchell/delaunay PR: 0
File: AGENTS.md:0-0
Timestamp: 2026-04-25T04:34:17.766Z
Learning: Breaking changes MUST use bang notation (feat!: description) or BREAKING CHANGE: <description> footer trailer for git-cliff detection in CHANGELOG.md
The 'verify_conflict_region_verification' test module is now entirely gated by the 'diagnostics' feature. This allows for the removal of redundant internal #[cfg(...)] blocks surrounding diagnostic helper calls, simplifying the test code. Updates the commented cargo test command. Refs: #355
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 #355