Skip to content

Drop MemoHash's std Hash impl and route cache-identity hashing through CacheHash#4209

Merged
TrueDoctor merged 1 commit into
masterfrom
memohash-cachehash
Jun 7, 2026
Merged

Drop MemoHash's std Hash impl and route cache-identity hashing through CacheHash#4209
TrueDoctor merged 1 commit into
masterfrom
memohash-cachehash

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 refactors hashing and comparison behavior for node inputs and memoized hashes. Specifically, it removes the standard Hash derivation from NodeInput and MemoHash, and manually implements PartialEq, Eq, PartialOrd, and Ord for MemoHash to compare the underlying value rather than the cache hash. Additionally, it updates ConstructionArgs to use cache_hash and fixes a documentation code block format. The review feedback suggests optimizing the manual PartialOrd and Ord implementations for MemoHash by checking Arc::ptr_eq first to avoid expensive value comparisons when the underlying Arc pointers are identical.

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 node-graph/libraries/core-types/src/memo.rs
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.

1 issue found across 4 files

Confidence score: 3/5

  • There is a concrete regression risk in node-graph/graph-craft/src/proto.rs: ConstructionArgs now uses cache identity for hashing while equality remains value-based, which violates the Eq/Hash contract.
  • This mismatch can cause incorrect behavior in hash-based collections (e.g., failed lookups or inconsistent deduplication), so the impact is user-facing if ConstructionArgs is used as a key.
  • Given the high severity/confidence on this issue, this is not a low-risk merge until hash/equality semantics are aligned.
  • Pay close attention to node-graph/graph-craft/src/proto.rs - ConstructionArgs hash and equality semantics are inconsistent and may trigger subtle logic errors.

Reply with feedback, questions, or to request a fix.

Re-trigger cubic

Comment thread node-graph/graph-craft/src/proto.rs
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.

No issues found across 4 files

Confidence score: 5/5

  • Automated review surfaced no issues in the provided summaries.
  • No files require special attention.

Re-trigger cubic

Comment thread node-graph/libraries/core-types/src/memo.rs Outdated
Comment thread node-graph/libraries/core-types/src/transform.rs Outdated
@TrueDoctor TrueDoctor force-pushed the memohash-cachehash branch from 126a0fa to 74ed44b Compare June 7, 2026 17:41
@TrueDoctor TrueDoctor enabled auto-merge June 7, 2026 17:43
@TrueDoctor TrueDoctor added this pull request to the merge queue Jun 7, 2026
Merged via the queue into master with commit d5c67ee Jun 7, 2026
10 checks passed
@TrueDoctor TrueDoctor deleted the memohash-cachehash branch June 7, 2026 17:50
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.

2 participants