refactor(tds)!: hydrate TDS through validated UUID snapshots (#454) - #460
Merged
Conversation
- Route TDS serialization through a validated UUID snapshot boundary that carries vertex, simplex, neighbor, and periodic-offset relationships without storage-local slotmap handles. - Rebuild runtime TDS storage only from validated snapshots, with fresh slotmap keys and full topology validation before exposing hydrated state. - Keep standalone simplex records from becoming an alternate hydration path, so simplex connectivity is resolved only in the TDS snapshot context. - Update repository guards and documentation to describe snapshot-based persistence as the serialization boundary. BREAKING CHANGE: TDS JSON now uses the validated snapshot format, including serialized simplex neighbor UUID relationships, and no longer supports older storage-local/key-based hydration shapes. Closes #454
acgetchell
enabled auto-merge (squash)
June 15, 2026 14:53
Contributor
WalkthroughIntroduces ChangesTDS UUID Snapshot Persistence Boundary
Sequence Diagram(s)sequenceDiagram
participant User
participant TdsSerialize as Tds::serialize
participant TdsSnapshot
participant RawTdsSnapshot as codec
participant TdsDeserialize as Tds::deserialize
rect rgba(100, 149, 237, 0.5)
note over User, codec: Serialization path
User->>TdsSerialize: serialize(tds)
TdsSerialize->>TdsSnapshot: from_tds(tds) — extract UUID slots
TdsSnapshot->>RawTdsSnapshot: into_raw() — emit codec-friendly records
RawTdsSnapshot-->>User: JSON (UUIDs, no slotmap keys)
end
rect rgba(60, 179, 113, 0.5)
note over User, TdsDeserialize: Deserialization path
User->>TdsDeserialize: deserialize(data)
TdsDeserialize->>RawTdsSnapshot: decode JSON
RawTdsSnapshot->>TdsSnapshot: parse() — validate UUIDs, arity, offsets
TdsSnapshot->>TdsSnapshot: into_tds() — remap to fresh SlotMap keys
TdsSnapshot->>TdsSnapshot: set_neighbors_from_keys + validate
TdsSnapshot-->>User: Result~Tds, TdsSnapshotError~
end
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related issues
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 docstrings
🧪 Generate unit tests (beta)
Comment |
acgetchell
disabled auto-merge
June 15, 2026 14:55
coderabbitai
Bot
added
rust
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 215 |
🟢 Coverage 97.85% diff coverage · +0.10% coverage variation
Metric Results Coverage variation ✅ +0.10% coverage variation (-1.00%) Diff coverage ✅ 97.85% diff coverage Coverage variation details
Coverable lines Covered lines Coverage Common ancestor commit (739aba0) 66786 61125 91.52% Head commit (9aed5b9) 67814 (+1028) 62132 (+1007) 91.62% (+0.10%) 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 (#460) 1625 1590 97.85% 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 #460 +/- ##
==========================================
+ Coverage 91.49% 91.59% +0.09%
==========================================
Files 72 72
Lines 66574 67602 +1028
==========================================
+ Hits 60915 61922 +1007
- Misses 5659 5680 +21
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: 3
🤖 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/core/tds_snapshot.rs`:
- Around line 707-722: The from_tds() function unnecessarily requires Copy
bounds on type parameters U and V by eagerly copying vertex data with *vertex
dereference, whereas the serialization format RawTdsSnapshot only requires
DataSerialize (not Copy). Refactor from_tds() to avoid eager copying—either by
collecting vertex references directly without dereferencing, or by using a
borrowed serialization path—so that non-Copy types implementing DataSerialize
can be serialized through Tds. Ensure the refactored approach aligns with how
deserialization correctly avoids the Copy requirement.
- Around line 231-237: The three relationship map fields in RawTdsSnapshot
(simplex_vertices, simplex_neighbors, and simplex_vertex_offsets) deserialize
directly into FastHashMap without detecting duplicate simplex UUID keys during
deserialization, allowing serde_json to silently overwrite earlier values.
Create custom deserializers for these fields that explicitly error on duplicate
Uuid keys during deserialization, similar to the existing SnapshotSimplexVisitor
pattern, and apply these deserializers to simplex_vertices, simplex_neighbors,
and simplex_vertex_offsets using serde attributes. Alternatively, if the
serde_with crate is available, apply the maps_duplicate_key_is_error attribute
to these three fields instead.
- Around line 588-604: The deserialization of the data field in the Tds snapshot
is collapsing explicit None values by wrapping deserialized values in Some() and
then calling flatten(). This loses the distinction between a present JSON field
with null value and a missing field. After the deserialization loop for uuid and
data fields completes, find where the deserialized values are being finalized
and remove the flatten() call that collapses Some(None) to None. Store the
deserialized V value directly without the intermediate Some() wrapping so that
explicit null payloads in the JSON are preserved as Some(None) rather than being
flattened to None.
🪄 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: ddce8678-4212-4585-9da2-0f6c8676c0eb
📒 Files selected for processing (11)
docs/code_organization.mdsemgrep.yamlsrc/core/collections/key_maps.rssrc/core/collections/secondary_maps.rssrc/core/simplex.rssrc/core/tds.rssrc/core/tds_snapshot.rstests/README.mdtests/proptest_orientation.rstests/semgrep/src/project_rules/rust_style.rstests/serialization_vertex_preservation.rs
💤 Files with no reviewable changes (2)
- src/core/collections/secondary_maps.rs
- src/core/collections/key_maps.rs
acgetchell
enabled auto-merge (squash)
June 15, 2026 15:41
- Reject duplicate UUID relationship-map keys and storage-local simplex fields during snapshot deserialization. - Preserve explicit null simplex payloads and serialize non-Copy vertex and simplex payload data without eager copies. - Clarify FacetHandle, FacetView, and EdgeKey as runtime-local topology identities with handle/view conversion APIs. - Add Semgrep guardrails that keep snapshot internals private, require duplicate-key deserializers, and prevent runtime handle serde.
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
semgrep.yaml (1)
732-752: 💤 Low valueFragile negative lookbehind relies on exact whitespace formatting.
The patterns use negative lookbehinds that expect exact indentation (4 spaces) and line structure. While this works for rustfmt-consistent code, it may produce false positives if the serde attribute is split across lines or uses different indentation.
Consider documenting this formatting sensitivity in the rationale, or accepting the limitation since rustfmt enforces consistent formatting in this repository.
🤖 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 `@semgrep.yaml` around lines 732 - 752, The negative lookbehind patterns in the delaunay.rust.raw-tds-snapshot-uuid-maps-require-duplicate-key-deserializers rule rely on exact whitespace matching (4 spaces of indentation), which may cause false positives if code formatting changes. Add documentation to the rationale section acknowledging that these patterns depend on rustfmt-consistent formatting, explaining that the negative lookbehinds expect specific indentation and line structure for the serde attributes decorating simplex_vertices, simplex_neighbors, and simplex_vertex_offsets fields, and note that deviations from this formatting (such as different indentation or different line breaks) may result in false positives despite the presence of the required deserializer attributes.
🤖 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 `@semgrep.yaml`:
- Around line 732-752: The negative lookbehind patterns in the
delaunay.rust.raw-tds-snapshot-uuid-maps-require-duplicate-key-deserializers
rule rely on exact whitespace matching (4 spaces of indentation), which may
cause false positives if code formatting changes. Add documentation to the
rationale section acknowledging that these patterns depend on rustfmt-consistent
formatting, explaining that the negative lookbehinds expect specific indentation
and line structure for the serde attributes decorating simplex_vertices,
simplex_neighbors, and simplex_vertex_offsets fields, and note that deviations
from this formatting (such as different indentation or different line breaks)
may result in false positives despite the presence of the required deserializer
attributes.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: bd6e2a63-5064-4b75-8c7a-efc63d2823d3
📒 Files selected for processing (7)
docs/dev/tooling-alignment.mdsemgrep.yamlsrc/core/edge.rssrc/core/facet.rssrc/core/tds_snapshot.rstests/prelude_exports.rstests/semgrep/src/project_rules/rust_style.rs
✅ Files skipped from review due to trivial changes (2)
- src/core/edge.rs
- docs/dev/tooling-alignment.md
🚧 Files skipped from review as they are similar to previous changes (1)
- src/core/tds_snapshot.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
BREAKING CHANGE: TDS JSON now uses the validated snapshot format, including serialized simplex neighbor UUID relationships, and no longer supports older storage-local/key-based hydration shapes.
Closes #454