Skip to content

Make Resource aware of its own hash#4187

Merged
timon-schelling merged 4 commits into
masterfrom
resource-knows-hash
May 31, 2026
Merged

Make Resource aware of its own hash#4187
timon-schelling merged 4 commits into
masterfrom
resource-knows-hash

Conversation

@timon-schelling
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 the Resource struct by moving it from core-types to the graphene-resource library. The updated Resource struct now includes a precomputed ResourceHash field, and new constructors Resource::new and Resource::new_unchecked have been introduced. In the editor dispatcher, async_message_handler has been renamed to future_message_handler. Feedback on these changes suggests two key performance optimizations for the Resource struct: first, updating CacheHash to hash the precomputed self.hash instead of the entire byte slice to avoid performance bottlenecks on large resources; second, optimizing PartialEq to compare the precomputed hashes first to quickly short-circuit inequality checks.

Comment thread node-graph/libraries/resources/src/lib.rs
Comment thread node-graph/libraries/resources/src/lib.rs
@timon-schelling timon-schelling changed the title Add Resource::hash() Make Resource aware of its own hash May 31, 2026
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 11 files

Confidence score: 3/5

  • The most significant risk is in node-graph/graph-craft/src/application_io/resource/mmap.rs: using new_unchecked without validating file contents against the requested hash could allow on-disk corruption to be accepted as a valid resource.
  • The issues in node-graph/libraries/resources/src/lib.rs (missing Eq on Resource and redundant hash recomputation in Resource::new) look lower-severity API/performance concerns and are not strong merge blockers on their own.
  • Because there is one concrete, user-impacting integrity concern (7/10 severity) alongside minor issues, this lands at moderate merge risk.
  • Pay close attention to node-graph/graph-craft/src/application_io/resource/mmap.rs, node-graph/libraries/resources/src/lib.rs - integrity validation in mmap loading and resource equality/hash ergonomics need careful review.
Prompt for AI agents (unresolved issues)

Check if these issues are valid — if so, understand the root cause of each and fix them. If appropriate, use sub-agents to investigate and fix each issue separately.


<file name="node-graph/graph-craft/src/application_io/resource/mmap.rs">

<violation number="1" location="node-graph/graph-craft/src/application_io/resource/mmap.rs:56">
P1: `new_unchecked` is used in mmap loading without verifying file contents match the requested hash, which can mask on-disk corruption as a valid resource.</violation>
</file>

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

Re-trigger cubic

let path = self.path_for(hash);
let mmap = Self::open_mmap(&path)?;
let resource = Resource::new(MmappedBytes(mmap));
let resource = Resource::new_unchecked(MmappedBytes(mmap), *hash);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

P1: new_unchecked is used in mmap loading without verifying file contents match the requested hash, which can mask on-disk corruption as a valid resource.

Prompt for AI agents
Check if this issue is valid — if so, understand the root cause and fix it. At node-graph/graph-craft/src/application_io/resource/mmap.rs, line 56:

<comment>`new_unchecked` is used in mmap loading without verifying file contents match the requested hash, which can mask on-disk corruption as a valid resource.</comment>

<file context>
@@ -53,7 +53,7 @@ impl MmapResourceStorage {
 		let path = self.path_for(hash);
 		let mmap = Self::open_mmap(&path)?;
-		let resource = Resource::new(MmappedBytes(mmap));
+		let resource = Resource::new_unchecked(MmappedBytes(mmap), *hash);
 
 		self.cache.write().unwrap_or_else(|poisoned| poisoned.into_inner()).insert(*hash, resource.clone());
</file context>

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

We can think about this at some point, yes

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Thanks for considering it.

Comment thread node-graph/libraries/resources/src/lib.rs
Comment thread node-graph/libraries/resources/src/lib.rs
@timon-schelling timon-schelling added this pull request to the merge queue May 31, 2026
Merged via the queue into master with commit e32ff4b May 31, 2026
12 checks passed
@timon-schelling timon-schelling deleted the resource-knows-hash branch May 31, 2026 23:08
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