Add graph-storage crate#4198
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces the graph-storage crate, which provides a delta-based graph representation for the Graphite file format, including CRDT/delta computation, conversion utilities, and round-trip tests. It also cleans up legacy migrations in graph-craft and updates MemoHash to compare values instead of hashes. The review feedback highlights a potential DoS/OOM vulnerability when resizing the exports vector with unvalidated slot indices, suggests optimizing the O(N^2) pairwise comparison in order_consistent to O(N log N) by sorting, and recommends using into_iter().next() instead of drain(..).next() for idiomatic element extraction.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
21ab02a to
2350854
Compare
2350854 to
6dda108
Compare
6811f03 to
9b01dd2
Compare
|
@cubic-ai-dev |
@TrueDoctor I have started the AI code review. It will take a few minutes to complete. |
There was a problem hiding this comment.
9 issues found across 26 files
Confidence score: 2/5
- There are multiple high-confidence, user-impacting regressions (sev 7–8) across hashing, graph diffing, and serialization, so merge risk is currently high rather than routine.
- Most severe:
node-graph/libraries/core-types/src/memo.rsmakesMemoHashequality ignorehashwhileHashstill includes it, which can violate hash collection invariants and cause incorrect map/set behavior. - Several storage/runtime paths risk dropped or corrupted data: missed network/export diffs in
document/graph-storage/src/delta.rs, omitted export-only resources indocument/graph-storage/src/from_runtime.rs, and silent truncation indocument/graph-storage/src/to_runtime.rswheninputslengths differ. - Pay close attention to
node-graph/libraries/core-types/src/memo.rs,document/graph-storage/src/delta.rs,document/graph-storage/src/from_runtime.rs,document/graph-storage/src/to_runtime.rs, andnode-graph/libraries/resources/src/lib.rs- they contain the highest-likelihood correctness and round-trip integrity risks.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
49f9dee to
05848ee
Compare
There was a problem hiding this comment.
11 issues found across 19 files
Confidence score: 2/5
- Multiple high-severity, high-confidence data-integrity risks make this PR risky to merge as-is, especially in core graph storage/CRDT paths (
document/graph-storage/src/attributes.rs,document/graph-storage/src/to_runtime.rs,document/graph-storage/src/crdt.rs,document/graph-storage/src/ids.rs). - The most severe issue is inconsistent
Priorityequality/ordering/hashing indocument/graph-storage/src/attributes.rs, which can breakHashMap/BTree*behavior and lead to hard-to-debug corruption-like behavior. - There are additional concrete regression risks affecting user-visible state correctness: duplicate node IDs can silently collapse nodes during runtime reconstruction, deletes can be resurrected without tombstones, and non-canonical
Revhashing can make identical deltas produce different IDs. - Pay close attention to
document/graph-storage/src/attributes.rs,document/graph-storage/src/to_runtime.rs,document/graph-storage/src/crdt.rs,document/graph-storage/src/ids.rs,document/graph-storage/src/session.rs,document/graph-storage/src/delta.rs,document/graph-storage/src/model.rs- core invariants and history/diff behavior can diverge from expected graph state.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
1fcc880 to
02ff474
Compare
There was a problem hiding this comment.
3 issues found across 19 files
Confidence score: 3/5
- There is meaningful regression risk in
document/graph-storage/src/session.rs: the unconditional registry reset can drop visible hot-zone edits and desynchronize working state fromhot_logwhen unretired ops are present, which is user-facing state integrity impact. document/graph-storage/src/session.rsalso has a fallible path incommit_opsthat currently usesassert!for missing parents, so malformed or unexpected input could panic the app instead of returning an error.document/graph-storage/src/ids.rsderivingDefaultforLamportClockintroduces an ambiguous/invalid construction path without an explicitPeerId; this is moderate API correctness risk rather than an immediate blocker.- Pay close attention to
document/graph-storage/src/session.rs,document/graph-storage/src/ids.rs- state rebuild correctness, panic-free error handling, and explicit clock initialization need validation before merge.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…corruption, drop LamportClock Default
There was a problem hiding this comment.
5 issues found across 19 files
Confidence score: 2/5
- There are concrete regression risks in core behavior:
document/graph-storage/src/document.rscan drop the root delta in history iteration, which can break history-based restoration when a removal happens at the root commit. document/graph-storage/src/to_runtime.rslacks same-network validation during node-reference remapping, so cross-network references may be converted into wrong or dangling runtime IDs instead of failing fast.- Additional medium-severity correctness issues in
document/graph-storage/src/attributes.rs(finite-value invariant bypass forPriority) anddocument/graph-storage/src/delta.rs(nondeterministic diffs fromHashSetiteration) increase the chance of inconsistent state and revision behavior. - Pay close attention to
document/graph-storage/src/document.rs,document/graph-storage/src/to_runtime.rs,document/graph-storage/src/attributes.rs, anddocument/graph-storage/src/delta.rs- they contain the highest-impact correctness and determinism risks before merge.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ross-network refs, make compute_deltas deterministic, harden Priority, index nodes by network
There was a problem hiding this comment.
5 issues found across 19 files
Confidence score: 2/5
- There are concrete correctness risks in core data behavior, so this looks higher risk than a typical merge despite being fixable.
- In
document/graph-storage/src/resources.rs, thesourcessorted-order invariant is assumed by binary-search-based operations but not enforced, which can silently corrupt source-chain behavior for unsorted external/deserialized data. - In
document/graph-storage/src/crdt.rsanddocument/graph-storage/src/delta.rs, timestamp tombstones for attribute deletes are dropped (allowing stale resurrection under out-of-order delivery) andinputs_attributeslength changes are missed in structural diffing, both of which can cause convergence or delta-loss regressions. - Pay close attention to
document/graph-storage/src/resources.rs,document/graph-storage/src/crdt.rs, anddocument/graph-storage/src/delta.rs- they contain the user-impacting correctness paths (ordering invariants, LWW tombstones, and diff completeness).
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…ts_attributes length as structural, dedupe demo-artwork loader
There was a problem hiding this comment.
6 issues found across 19 files
Confidence score: 2/5
- Current risk is high because
Delta.idindocument/graph-storage/src/crdt.rsis trusted during deserialization without validating against the content hash, which can undermine revision integrity via crafted history-key spoofing/collision. document/graph-storage/src/resources.rsandnode-graph/libraries/resources/src/lib.rsboth introduce concrete data-correctness risk: duplicate on-disk keys can break binary-search behavior, and 64-bitResourceIdtruncation can collide distinct resources and merge mappings incorrectly.- There are also stability concerns in
document/graph-storage/src/ids.rswherecompute_revandmint_node_iduseexpect, so recoverable failures may become process crashes; this is important but likely straightforward to harden. - Pay close attention to
document/graph-storage/src/crdt.rs,document/graph-storage/src/resources.rs,node-graph/libraries/resources/src/lib.rs, anddocument/graph-storage/src/ids.rs- integrity validation, key invariants, ID collision handling, and panic paths are the main merge blockers.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…e, fix doc grammar
There was a problem hiding this comment.
5 issues found across 19 files
Confidence score: 3/5
- There is a concrete regression risk in
document/graph-storage/src/from_runtime.rs: droppingscope_injectionsduring runtime→storage conversion can silently lose graph behavior on round trips, which is user-impacting and raises merge risk. document/graph-storage/src/resources.rshas correctness and stability concerns (has_embedded_sourceusing raw JSON equality and panickingexpect(...)inResourceEntry::embedded), which could lead to duplicate embedded fallbacks or editor crashes instead of handled errors.document/graph-storage/src/crdt.rsanddocument/graph-storage/src/crdt_tests.rsshow undo/test reliability gaps (is_gesture_endtreating any marker as true, and a test not assertingis_err()), so behavior around gesture boundaries and error-path coverage is less certain.- Pay close attention to
document/graph-storage/src/from_runtime.rs,document/graph-storage/src/resources.rs,document/graph-storage/src/crdt.rs, anddocument/graph-storage/src/crdt_tests.rs- data-loss-on-conversion, embedded-source detection, panic handling, and gesture/error-path correctness need validation before merge.
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…dded-source checks
No description provided.