feat(contract): canonical SoA node (NodeGuid/EdgeBlock/NodeRow) — 12+4 edges, zero-fallback ladder#489
Conversation
|
Warning Review limit reached
More reviews will be available in 39 minutes and 11 seconds. Learn how PR review limits work. Your organization has used up its prepaid credits, and credit purchases are no longer available. Enable the review add-on in the billing tab to keep reviews running — you're only billed for reviews past your plan's rate limits ($0.25/file). ⌛ How to resolve this issue?After more reviews become available, 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 include higher PR review limits than trial, open-source, and free plans. In all cases, reviews become available again over time. During sustained high-volume PR review activity, CodeRabbit may temporarily slow when the next review becomes available. Please see our Fair Usage Limits Policy for further information. ℹ️ Review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThis PR introduces a new canonical binary addressing scheme for graph nodes. It defines three core data structures: ChangesGraph Node Canonical Binary Layout
🎯 3 (Moderate) | ⏱️ ~20 minutes
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
crates/lance-graph-contract/src/canonical_node.rs (2)
162-202: ⚡ Quick winAdd focused tests for the new mint-path uniqueness helper.
The tests are good for layout and fallback semantics, but there’s no direct test coverage for
debug_assert_identity_uniquebehavior in bootstrap vs non-bootstrap cases.🧪 Suggested tests
#[cfg(test)] mod tests { use super::*; @@ fn edge_block_is_twelve_plus_four() { let e = EdgeBlock::default(); assert_eq!(e.in_family.len(), 12); assert_eq!(e.out_family.len(), 4); assert_eq!(core::mem::size_of_val(&e), 16); } + + #[test] + fn uniqueness_guard_is_noop_outside_bootstrap() { + let g = NodeGuid::new(1, 0, 0, 0, 0, 1); + debug_assert_identity_unique(&g, true); + } + + #[cfg(debug_assertions)] + #[test] + #[should_panic(expected = "identity collision in default basin")] + fn uniqueness_guard_panics_on_bootstrap_collision() { + let g = NodeGuid::local(1); + debug_assert_identity_unique(&g, true); + } }As per coding guidelines,
crates/**/*.rs: “Add Rust unit tests alongside implementations via#[cfg(test)]modules; prefer focused scenarios over broad integration tests.”🤖 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 `@crates/lance-graph-contract/src/canonical_node.rs` around lines 162 - 202, Add focused unit tests for debug_assert_identity_unique inside the existing #[cfg(test)] mod: create one test (e.g., identity_unique_bootstrap) that constructs a bootstrap NodeGuid (use NodeGuid::local or NodeGuid::new with family==0 and bootstrap bits) and calls debug_assert_identity_unique to assert it does not abort, and another test (e.g., identity_unique_nonbootstrap) that constructs a non-bootstrap NodeGuid (family != 0 or non-bootstrap bits set) and verifies the expected behavior (panic or no-op per the helper's contract) when calling debug_assert_identity_unique; place both tests next to the other tests in canonical_node.rs so they run with the module and clearly name the cases (bootstrap vs non-bootstrap).Source: Coding guidelines
43-57: ⚡ Quick winMake 24-bit narrowing explicit in
NodeGuid::newto avoid accidental aliasing.Line 44 accepts
u32forfamily/identity, but Lines 56-57 persist only 3 bytes. Distinct 32-bit inputs can collapse to the same stored key; add explicit bounds checks (or a checked constructor) so truncation is never accidental.🛡️ Suggested guard
pub const fn new(classid: u32, heel: u16, hip: u16, twig: u16, family: u32, identity: u32) -> Self { + assert!(family <= 0x00FF_FFFF, "family must fit in 24 bits"); + assert!(identity <= 0x00FF_FFFF, "identity must fit in 24 bits"); let c = classid.to_le_bytes(); let h = heel.to_le_bytes(); let p = hip.to_le_bytes(); let t = twig.to_le_bytes();🤖 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 `@crates/lance-graph-contract/src/canonical_node.rs` around lines 43 - 57, The constructor new currently accepts family: u32 and identity: u32 but silently drops the high byte (keeps only 3 bytes) which can cause accidental aliasing; modify the API so truncation is explicit and guarded: keep pub const fn new(...) but add debug_assert!(family <= 0xFF_FFFF) and debug_assert!(identity <= 0xFF_FFFF) (or panic/assert for non-debug builds if you prefer strict failure), and add a fallible constructor pub fn new_checked(classid: u32, heel: u16, hip: u16, twig: u16, family: u32, identity: u32) -> Result<Self, Error> (or pub fn try_new returning Option<Self>) that returns an error when family or identity exceed 0xFFFFFF; update callers to use new_checked where inputs may be untrusted and add tests for boundary values; reference the existing function new in canonical_node.rs when implementing these 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 `@crates/lance-graph-contract/src/canonical_node.rs`:
- Around line 147-155: The free function debug_assert_identity_unique(guid:
&NodeGuid, already_present: bool) should be moved onto the NodeGuid type as an
inherent method; remove the standalone function and add an impl NodeGuid { pub
fn debug_assert_identity_unique(&self, already_present: bool) { ... } } with the
same body (replace `guid` usages with `self`), and update all call sites to use
guid.debug_assert_identity_unique(already_present) instead of the free-function
form.
---
Nitpick comments:
In `@crates/lance-graph-contract/src/canonical_node.rs`:
- Around line 162-202: Add focused unit tests for debug_assert_identity_unique
inside the existing #[cfg(test)] mod: create one test (e.g.,
identity_unique_bootstrap) that constructs a bootstrap NodeGuid (use
NodeGuid::local or NodeGuid::new with family==0 and bootstrap bits) and calls
debug_assert_identity_unique to assert it does not abort, and another test
(e.g., identity_unique_nonbootstrap) that constructs a non-bootstrap NodeGuid
(family != 0 or non-bootstrap bits set) and verifies the expected behavior
(panic or no-op per the helper's contract) when calling
debug_assert_identity_unique; place both tests next to the other tests in
canonical_node.rs so they run with the module and clearly name the cases
(bootstrap vs non-bootstrap).
- Around line 43-57: The constructor new currently accepts family: u32 and
identity: u32 but silently drops the high byte (keeps only 3 bytes) which can
cause accidental aliasing; modify the API so truncation is explicit and guarded:
keep pub const fn new(...) but add debug_assert!(family <= 0xFF_FFFF) and
debug_assert!(identity <= 0xFF_FFFF) (or panic/assert for non-debug builds if
you prefer strict failure), and add a fallible constructor pub fn
new_checked(classid: u32, heel: u16, hip: u16, twig: u16, family: u32, identity:
u32) -> Result<Self, Error> (or pub fn try_new returning Option<Self>) that
returns an error when family or identity exceed 0xFFFFFF; update callers to use
new_checked where inputs may be untrusted and add tests for boundary values;
reference the existing function new in canonical_node.rs when implementing these
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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: e1c662ae-f463-4a9b-ba3f-923326690bf6
📒 Files selected for processing (1)
crates/lance-graph-contract/src/canonical_node.rs
- Move debug_assert_identity_unique from free fn to method on NodeGuid (CLAUDE.md litmus test: free function on carrier's state -> reject; method on carrier -> accept). - Add 24-bit guard in NodeGuid::new for family/identity (catches the silent-truncation footgun where distinct u32 inputs would collapse to the same stored key). - Add focused tests: bootstrap-collision panics (debug only), no-op outside bootstrap, family/identity overflow asserts both fire. Module stays out of lib.rs per the PR's standalone-for-review intent. Verified the new tests by transient mod wire + cargo test -- canonical_node: 8 passed, 0 failed (lib.rs reverted after). https://claude.ai/code/session_01PBTGaPCSnnt6u3pjXpbLwY
feat(contract): canonicalise #489 — wire-in + self-describing Display + retire Phase-A wrapper
2 participants
Makes the canon locked in
CLAUDE.md(commit 4ea6ac9) visible as code.Node = 4096 bit = 512 byte:
key(16) | edges(16) | value(480).NodeGuid([u8;16])— LE;classid(4B)·HEEL(2B)·HIP(2B)·TWIG(2B)·family(3B)·identity(3B).family+identityare the contiguous trailing 6 bytes ⇒local_key()is one masked load after the trie binds the prefix.EdgeBlock { in_family:[u8;12], out_family:[u8;4] }— canonical, not mandatory; always reserved, never shrunk.classid==0⇒ default class (no prefix routing);family==0⇒ default basin (no grouping);identity(24 bit) alone discriminates in the bootstrap state. Reserve-don't-reclaim: zero = not consulted, never compacted away.NodeGuidfeat(contract): NodeGuid — structured 128-bit instance identity (identity-architecture Phase A) #480 is audited against this canon.const _size asserts at 16/16/512; guardsis_default_class/is_unbasined/is_bootstrap_address;debug_assert_identity_uniquefor the mint path. Unit tests cover the ladder + trailing-6-byte invariant.Not yet wired into
lib.rsor theSoaEnvelopedescriptors — deliberately standalone so the layout can be reviewed in isolation before theENVELOPE_LAYOUT_VERSIONbump.Summary by CodeRabbit