fix(deps): prevent stale vcpkg - #142
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
WalkthroughThe change adds incidence-based causal metadata persistence for coincident vertices. It also adds an exact-baseline vcpkg tool-pin synchronizer, workflow integration, tests, documentation, and Dependabot configuration. ChangesCausal metadata persistence
vcpkg tool-pin synchronization
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Mesh
participant Serializer
participant IndexedParser
Mesh->>Serializer: serialize indexed causal metadata
Serializer->>IndexedParser: provide versioned payload
IndexedParser->>IndexedParser: validate indices
IndexedParser->>Mesh: restore metadata in container order
sequenceDiagram
participant CLI
participant VcpkgMetadata
participant Bootstrap
CLI->>VcpkgMetadata: read exact manifest baseline
VcpkgMetadata->>CLI: return release metadata and Windows assets
CLI->>CLI: validate and hash assets
CLI->>Bootstrap: atomically write tool pins
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Add a Just command that derives the vcpkg tool release from the exact manifest baseline and refreshes trusted Windows asset hashes atomically.\n\nKeep existing pins unchanged when metadata or asset downloads cannot be completed and validated.\n\nCo-Authored-By: Oz <oz-agent@warp.dev>
Persist vertex and cell metadata by payload index instead of coordinate-derived keys. Make topology fingerprints incidence-aware when distinct vertices share coordinates, and reject invalid v2 metadata indices. BREAKING CHANGE: v1 causal-data trailers are no longer readable; persisted causal metadata now uses the v2 payload-indexed format.
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 (1)
include/Utilities.hpp (1)
427-443: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument that the two branches produce non-comparable digests.
The coincident branch fingerprints refined incidence signatures. The default branch fingerprints point-keyed vertex and cell records. The two record sets share no format, so the same abstract triangulation yields a completely different digest depending on which branch runs.
The round-trip paths stay self-consistent, because the payload is written with
max_digits10precision at Line 1438 andpoint_keyformats with{:.17g}, so coordinates round-trip exactly andhas_coincident_verticesreturns the same answer for the original and the parsed triangulation.validate_serialized_payloadandvalidate_persistence_metadatatherefore compare like with like.The risk is for a future reader or caller who assumes any two
topology.fnv1a64values are directly comparable. A short comment stating that the digest is branch-dependent, and that the branch is selected by coordinate coincidence, prevents that misreading.📝 Proposed clarifying comment
template <typename TriangulationType> [[nodiscard]] auto canonical_topology_fingerprint( TriangulationType const& triangulation) -> std::uint64_t { + // Two record formats, so two digest families. Coincident coordinates + // make point keys ambiguous, so those states are fingerprinted from + // refined vertex-cell incidence instead. Payloads round-trip + // coordinates exactly, so a given state always selects the same + // branch and validation compares matching formats. Digests from + // different branches are not comparable. if (has_coincident_vertices(triangulation))🤖 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 `@include/Utilities.hpp` around lines 427 - 443, Add a concise comment immediately before the coincident-vertex branch explaining that topology.fnv1a64 is branch-dependent: coincident coordinates use refined incidence signatures, while the default path uses point-keyed vertex and cell records, so digests from the two branches are not directly comparable.
🤖 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/reproducibility.md`:
- Around line 98-101: Add a sentence to the trailer compatibility discussion in
docs/reproducibility.md stating that payloads using the older
cdt-plusplus-causal-info-v1 trailer are rejected entirely by read_causal_info,
rather than loaded with their causal metadata discarded. Keep the existing
behavior for streams with no trailer unchanged.
In `@include/Utilities.hpp`:
- Around line 765-778: Use checked access when assigning restored metadata in
the vertex and cell loops: replace unchecked indexing of vertex_info and
cell_info with the container’s bounds-checked accessor. Preserve the existing
index increments and trailing equality checks so malformed payloads throw
through the existing read_file/write_payload error path and leftover records
remain validated.
- Around line 350-393: Update both refined_signature call sites in the vertex
and cell refinement loops to use only the current color and sorted neighboring
colors, removing base from per-iteration signatures. After refinement completes,
retain the original vertex_bases and cell_bases in the final returned records by
prepending each base before the final sort, preserving distinctions between
different coordinates or info values.
In `@README.md`:
- Around line 359-365: Update the README description of the synchronization
workflow to state that it writes only when the rendered pin assignments differ,
rather than relying on the tool release matching. Correct the Windows invocation
to use python.exe scripts\sync_vcpkg_tool_pins.py, while preserving the existing
behavior description for fetching, validation, and atomic updates.
In `@scripts/sync_vcpkg_tool_pins.py`:
- Around line 118-133: Preserve the bootstrap file’s original line endings
throughout pin updates. Update the file-reading flow to disable newline
translation, then make _render_bootstrap detect the source line ending and use
it for both replacement assignments and rendered trailing lines; add a CRLF
fixture verifying unrelated bytes remain unchanged.
In `@tests/Utilities_test.cpp`:
- Around line 331-381: Harden the coincident-vertices scenario by requiring the
first vertex iterator to be valid before calling std::next or dereferencing it,
then directly assert that the modified vertices share the same point to ensure
the coincidence branch is exercised. Split the existing THEN assertions into
separate descriptively named blocks for topology fingerprint stability and
vertex/cell metadata preservation.
---
Outside diff comments:
In `@include/Utilities.hpp`:
- Around line 427-443: Add a concise comment immediately before the
coincident-vertex branch explaining that topology.fnv1a64 is branch-dependent:
coincident coordinates use refined incidence signatures, while the default path
uses point-keyed vertex and cell records, so digests from the two branches are
not directly comparable.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 8026e8c2-e256-4330-8aa6-15eb2c364ee0
📒 Files selected for processing (7)
JustfileREADME.mddocs/reproducibility.mdinclude/Utilities.hppscripts/sync_vcpkg_tool_pins.pyscripts/tests/test_sync_vcpkg_tool_pins.pytests/Utilities_test.cpp
Keep vertex and cell attributes out of incidence-refinement cycles while retaining them in final topology records, and use checked metadata restoration. Preserve bootstrap line endings during vcpkg pin updates and clarify the render-based update and Windows invocation contracts.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@include/Utilities.hpp`:
- Around line 393-405: Replace the records construction and sorting in the
incidence-graph validation path with a canonical bipartite-incidence encoding
that includes individual vertex-to-cell connectivity, not only
vertex_bases/vertex_signatures and cell_bases/cell_signatures. Ensure the
encoding is invariant under equivalent vertex/cell ordering while distinguishing
non-isomorphic degree-equivalent graphs, and add a regression case covering two
such incidence structures.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: a102857b-19dd-44af-a9ed-32253bf172d5
📒 Files selected for processing (6)
README.mddocs/reproducibility.mdinclude/Utilities.hppscripts/sync_vcpkg_tool_pins.pyscripts/tests/test_sync_vcpkg_tool_pins.pytests/Utilities_test.cpp
Encode individual vertex-to-cell connectivity for coincident triangulations so degree-equivalent, non-isomorphic topologies produce distinct fingerprints. Preserve ordering invariance by resolving ambiguous incidence colors before selecting the canonical encoding.
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
include/Utilities.hpp (1)
516-538: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftDo not rely on undocumented CGAL ordering.
CGAL 6.2 does not guarantee that
operator<<andoperator>>preserve the order offinite_vertex_handles()orfinite_cell_handles(). Replace positional metadata with stable identifiers, or add an explicit order-independent mapping and regression test.🤖 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 `@include/Utilities.hpp` around lines 516 - 538, Replace the positional vertex_index and cell_index metadata in the triangulation serialization logic with stable identifiers or an explicit order-independent mapping that survives CGAL stream round-trips. Update the corresponding deserialization path and add a regression test verifying metadata remains associated with the correct vertices and cells when finite handle iteration order changes.
🤖 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 `@include/Utilities.hpp`:
- Around line 323-353: Bound the recursive individualization work in
canonical_incidence_search by threading an explicit shared budget through every
recursive call and seeding it from canonical_bipartite_incidence_records.
Decrement the budget for each visited search node or refinement operation, and
when exhausted, stop the unbounded search using a diagnosable error or the
established cheap coarser-record fallback; ensure the coincident-vertex
fingerprint path has a predictable cost ceiling.
- Around line 288-297: Document the discrete-coloring precondition at the
incidence_records_for_coloring interface: colors must assign each color class to
exactly one node, making colors an inverse permutation with every color index
valid and unique. Add a brief precondition note or an Expects-style validation
near the function declaration, while preserving the existing .at()-based
behavior and caller logic.
- Around line 355-382: Update canonical_bipartite_incidence_records to validate
every vertex index in cell_vertices against vertex_bases.size() before building
adjacency, rejecting indices that do not identify a vertex rather than allowing
cell-node references. Move the bases.empty() early return before the adjacency
construction loop, while preserving the existing cell_bases/cell_vertices size
validation and canonicalization flow.
In `@tests/Utilities_test.cpp`:
- Around line 258-303: Extend the tests for
canonical_bipartite_incidence_records with separate scenarios covering
mismatched cell_bases and cell_vertices sizes, expecting std::invalid_argument,
and empty input, expecting an empty vector. Split the existing cycle equality
and component inequality checks into distinct THEN blocks with descriptive names
so each property is reported independently, while preserving the current
isomorphism and connectivity cases.
---
Outside diff comments:
In `@include/Utilities.hpp`:
- Around line 516-538: Replace the positional vertex_index and cell_index
metadata in the triangulation serialization logic with stable identifiers or an
explicit order-independent mapping that survives CGAL stream round-trips. Update
the corresponding deserialization path and add a regression test verifying
metadata remains associated with the correct vertices and cells when finite
handle iteration order changes.
🪄 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.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: 95378c7d-fe74-4178-8712-7eb7c4982c64
📒 Files selected for processing (2)
include/Utilities.hpptests/Utilities_test.cpp
- Cap recursive individualization with a shared, diagnosable work budget. - Reject malformed cell incidence indices before constructing adjacency. - Handle empty incidence inputs explicitly.
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)
include/Utilities.hpp (2)
862-873: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winRestore the trailing count check dropped from the checked-indexing fix.
vertex_info.at(vertex_index)andcell_info.at(cell_index)correctly convert an out-of-bounds write into a thrown exception, addressing the prior "use checked indexing" review. However, that fix only guards againstfinite_vertex_handles()/finite_cell_handles()yielding more elements than expected. If they yield fewer (a scenario the prior review already flagged as possible for triangulations whosedimension()is less than 3, and this function runs beforetds().is_valid()), the loop ends early and the remaining entries invertex_info/cell_infoare silently dropped instead of raising an error.Add the trailing equality check the prior review suggested, so a partially consumed index range is treated as malformed metadata.
🛡️ Proposed trailing validation
std::size_t vertex_index{}; for (auto const vertex : triangulation.finite_vertex_handles()) { vertex->info() = vertex_info.at(vertex_index); ++vertex_index; } + if (vertex_index != vertex_info.size()) + { + throw std::filesystem::filesystem_error( + "Causal vertex metadata count does not match triangulation", path, + std::make_error_code(std::errc::illegal_byte_sequence)); + } std::size_t cell_index{}; for (auto const cell : triangulation.finite_cell_handles()) { cell->info() = cell_info.at(cell_index); ++cell_index; } + if (cell_index != cell_info.size()) + { + throw std::filesystem::filesystem_error( + "Causal cell metadata count does not match triangulation", path, + std::make_error_code(std::errc::illegal_byte_sequence)); + }🤖 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 `@include/Utilities.hpp` around lines 862 - 873, After each finite handle loop in the metadata assignment flow, validate that the consumed index count equals the corresponding vector size: compare vertex_index with vertex_info.size() and cell_index with cell_info.size(), and raise the existing malformed-metadata error when either count differs. Keep the checked at() access and apply the trailing equality checks separately to vertices and cells.
267-300: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound recursion depth, not only total work, in the individualization search.
The work budget in
consume_canonical_incidence_workbounds the total number of refinement/search steps, but it does not bound how deepcanonical_incidence_searchcan recurse. Each recursive level consumes at least a small constant amount of budget (one unit for the call itself, at least one more insiderefine_incidence_colors), and the node count stays fixed across the whole recursion tree because individualizing one node only recolors that node. For a highly symmetric, moderately sized incidence graph, this lets the recursion go tens of thousands of frames deep before the budget is exhausted.
canonical_incidence_searchruns on the read path, on a triangulation freshly parsed from a file, before thetds().is_valid()check. A crafted or corrupted checkpoint with many coincident vertices could drive deep recursion and crash the process with a stack overflow rather than raising the intendedstd::runtime_error.Add an explicit recursion-depth cap alongside the work budget.
🛡️ Proposed depth cap
+ inline constexpr std::size_t CANONICAL_INCIDENCE_MAX_DEPTH{2'000}; + [[nodiscard]] inline auto canonical_incidence_search( std::vector<std::string> const& bases, std::vector<std::vector<std::size_t>> const& adjacency, - std::vector<std::size_t> colors, std::size_t& budget) + std::vector<std::size_t> colors, std::size_t& budget, + std::size_t const depth = 0) -> std::vector<std::string> { + if (depth >= CANONICAL_INCIDENCE_MAX_DEPTH) + { + throw std::runtime_error{ + "Canonical incidence search exceeded its recursion depth limit"}; + } consume_canonical_incidence_work(budget); colors = refine_incidence_colors(adjacency, std::move(colors), budget); ... - auto candidate = canonical_incidence_search( - bases, adjacency, std::move(individualized), budget); + auto candidate = canonical_incidence_search( + bases, adjacency, std::move(individualized), budget, depth + 1);Also applies to: 339-371
🤖 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 `@include/Utilities.hpp` around lines 267 - 300, Bound recursion depth in canonical_incidence_search independently of the work budget by passing and checking an explicit depth counter or remaining-depth limit at every recursive call. Reject searches that exceed the cap with the existing intended std::runtime_error path, while preserving the current budget accounting and refinement behavior in refine_incidence_colors.
🤖 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 `@include/Utilities.hpp`:
- Around line 862-873: After each finite handle loop in the metadata assignment
flow, validate that the consumed index count equals the corresponding vector
size: compare vertex_index with vertex_info.size() and cell_index with
cell_info.size(), and raise the existing malformed-metadata error when either
count differs. Keep the checked at() access and apply the trailing equality
checks separately to vertices and cells.
- Around line 267-300: Bound recursion depth in canonical_incidence_search
independently of the work budget by passing and checking an explicit depth
counter or remaining-depth limit at every recursive call. Reject searches that
exceed the cap with the existing intended std::runtime_error path, while
preserving the current budget accounting and refinement behavior in
refine_incidence_colors.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
Run ID: ddce3d1d-3a7b-463a-9659-cc6d6538574b
📒 Files selected for processing (2)
include/Utilities.hpptests/Utilities_test.cpp
1 participant
Summary by CodeRabbit
New Features
just sync-vcpkg-tool-pinsto synchronize and validate vcpkg tool release metadata and Windows integrity pins.Bug Fixes
Documentation
Chores