Skip to content

Add graph-storage crate#4198

Draft
TrueDoctor wants to merge 10 commits into
masterfrom
upstream-graph-storage
Draft

Add graph-storage crate#4198
TrueDoctor wants to merge 10 commits into
masterfrom
upstream-graph-storage

Conversation

@TrueDoctor
Copy link
Copy Markdown
Member

No description provided.

Copy link
Copy Markdown
Contributor

@gemini-code-assist gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Comment thread document/graph-storage/src/lib.rs Outdated
Comment thread document/graph-storage/src/lib.rs Outdated
Comment thread node-graph/graph-craft/src/document/value.rs Outdated
@TrueDoctor
Copy link
Copy Markdown
Member Author

@cubic-ai-dev

@cubic-dev-ai
Copy link
Copy Markdown
Contributor

cubic-dev-ai Bot commented Jun 7, 2026

@cubic-ai-dev

@TrueDoctor I have started the AI code review. It will take a few minutes to complete.

Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.rs makes MemoHash equality ignore hash while Hash still 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 in document/graph-storage/src/from_runtime.rs, and silent truncation in document/graph-storage/src/to_runtime.rs when inputs lengths 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, and node-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

Comment thread node-graph/libraries/resources/src/lib.rs
Comment thread node-graph/libraries/vector-types/src/gradient.rs Outdated
Comment thread node-graph/libraries/core-types/src/memo.rs
Comment thread document/graph-storage/src/from_runtime.rs
Comment thread document/graph-storage/src/to_runtime.rs
Comment thread document/graph-storage/src/delta.rs
Comment thread document/graph-storage/src/delta.rs Outdated
Comment thread node-graph/graph-craft/src/document/value.rs Outdated
Comment thread document/graph-storage/src/from_runtime.rs Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Priority equality/ordering/hashing in document/graph-storage/src/attributes.rs, which can break HashMap/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 Rev hashing 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

Comment thread document/graph-storage/src/attributes.rs Outdated
Comment thread document/graph-storage/src/to_runtime.rs Outdated
Comment thread document/graph-storage/src/crdt.rs
Comment thread document/graph-storage/src/ids.rs
Comment thread document/graph-storage/src/session.rs Outdated
Comment thread document/graph-storage/src/model.rs
Comment thread document/graph-storage/src/model.rs
Comment thread document/graph-storage/Cargo.toml Outdated
Comment thread document/graph-storage/Cargo.toml Outdated
Comment thread document/graph-storage/Cargo.toml Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 from hot_log when unretired ops are present, which is user-facing state integrity impact.
  • document/graph-storage/src/session.rs also has a fallible path in commit_ops that currently uses assert! for missing parents, so malformed or unexpected input could panic the app instead of returning an error.
  • document/graph-storage/src/ids.rs deriving Default for LamportClock introduces an ambiguous/invalid construction path without an explicit PeerId; 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

Comment thread document/graph-storage/src/session.rs
Comment thread document/graph-storage/src/ids.rs Outdated
Comment thread document/graph-storage/src/session.rs Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 19 files

Confidence score: 2/5

  • There are concrete regression risks in core behavior: document/graph-storage/src/document.rs can 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.rs lacks 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 for Priority) and document/graph-storage/src/delta.rs (nondeterministic diffs from HashSet iteration) 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, and document/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

Comment thread document/graph-storage/src/document.rs Outdated
Comment thread document/graph-storage/src/to_runtime.rs
Comment thread document/graph-storage/src/attributes.rs Outdated
Comment thread document/graph-storage/src/delta.rs
Comment thread document/graph-storage/src/to_runtime.rs Outdated
…ross-network refs, make compute_deltas deterministic, harden Priority, index nodes by network
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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, the sources sorted-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.rs and document/graph-storage/src/delta.rs, timestamp tombstones for attribute deletes are dropped (allowing stale resurrection under out-of-order delivery) and inputs_attributes length 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, and document/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

Comment thread document/graph-storage/src/resources.rs
Comment thread document/graph-storage/src/crdt.rs
Comment thread document/graph-storage/src/delta.rs Outdated
Comment thread document/graph-storage/src/round_trip_tests.rs Outdated
Comment thread document/graph-storage/src/model.rs
…ts_attributes length as structural, dedupe demo-artwork loader
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

6 issues found across 19 files

Confidence score: 2/5

  • Current risk is high because Delta.id in document/graph-storage/src/crdt.rs is 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.rs and node-graph/libraries/resources/src/lib.rs both introduce concrete data-correctness risk: duplicate on-disk keys can break binary-search behavior, and 64-bit ResourceId truncation can collide distinct resources and merge mappings incorrectly.
  • There are also stability concerns in document/graph-storage/src/ids.rs where compute_rev and mint_node_id use expect, 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, and document/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

Comment thread document/graph-storage/src/crdt.rs
Comment thread document/graph-storage/src/resources.rs
Comment thread node-graph/libraries/resources/src/lib.rs
Comment thread document/graph-storage/src/ids.rs
Comment thread document/graph-storage/src/ids.rs
Comment thread document/graph-storage/src/metadata_source.rs Outdated
Copy link
Copy Markdown
Contributor

@cubic-dev-ai cubic-dev-ai Bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

5 issues found across 19 files

Confidence score: 3/5

  • There is a concrete regression risk in document/graph-storage/src/from_runtime.rs: dropping scope_injections during runtime→storage conversion can silently lose graph behavior on round trips, which is user-impacting and raises merge risk.
  • document/graph-storage/src/resources.rs has correctness and stability concerns (has_embedded_source using raw JSON equality and panicking expect(...) in ResourceEntry::embedded), which could lead to duplicate embedded fallbacks or editor crashes instead of handled errors.
  • document/graph-storage/src/crdt.rs and document/graph-storage/src/crdt_tests.rs show undo/test reliability gaps (is_gesture_end treating any marker as true, and a test not asserting is_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, and document/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

Comment thread document/graph-storage/src/from_runtime.rs
Comment thread document/graph-storage/src/resources.rs
Comment thread document/graph-storage/src/resources.rs
Comment thread document/graph-storage/src/crdt_tests.rs Outdated
Comment thread document/graph-storage/src/crdt.rs Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant