feat(storage): file-backed graph generations (#338) - #487
Merged
Conversation
WalkthroughThe PR adds file-backed graph generation storage. It validates inventories and graph trees, stages them in project generations, supports pinned or materialized opens, updates rollback and checkpoint flows, preserves legacy snapshots, and adds integration and contract coverage. ChangesFile-backed graph generations
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant GraphForge
participant ProjectPublication
participant ProjectGeneration
participant GraphWorkspace
GraphForge->>ProjectPublication: capture and stage graph files
ProjectPublication->>ProjectGeneration: validate and publish graph tree
GraphForge->>ProjectGeneration: resolve pinned generation
ProjectGeneration->>GraphWorkspace: pin or materialize verified tree
GraphWorkspace-->>GraphForge: graph workspace and open evidence
Possibly related issues
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
core
This comment has been minimized.
This comment has been minimized.
cursor
Bot
force-pushed
the
cursor/338-file-backed-generations-c6a4
branch
from
August 9, 2026 23:46
5b3410f to
0d6b4d3
Compare
DecisionNerd
marked this pull request as ready for review
August 10, 2026 00:10
Introduce the versioned graph/files inventory contract and generation-owned graph/ tree staging under CURRENT authority, with parent auto-carry for unchanged trees. Legacy graph/snapshot remains untouched. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Switch GraphForge publication to the graph/files inventory + generation graph/ tree path so large workspaces are not assembled into one Arrow snapshot. Preserve legacy snapshot hydrate, record open evidence, document scale/project-format guidance, and add a deterministic CI fixture. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Add mid-graph-tree staging failpoint coverage with recovery cleanup, PinnedInPlace checkpoint assertions, portable unsupported export checks, and an ignored >2GiB sparse file-backed proof. Refresh M4 matrix/docs that still claimed the snapshot envelope blocked the public path. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Commit the measured sparse oversize evidence artifact, point M4 docs/matrix at it, and apply cargo fmt after the P1/P0 test follow-through. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Public open-evidence accessors are introspection helpers for #338 file-backed generation observability and must be inventoried before Repository Policy. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Satisfy format_collect and include //crates/graphforge-api:file_backed_graph_generation in api_integration_tests so //:ci_rust_tests reaches the #338 fixture. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Checkpoint revert was staging graph/files via the parent (CURRENT) tree, so restored inventories failed length/digest checks after post-checkpoint mutations. Pass the pinned source generation's graph/ tree instead, update the corruption matrix for files participants, and apply cargo fmt. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
Collapse the unused lock-and-tree helper into stage_project_generation_with_lock so revert and ordinary exclusive staging share one entry point and clippy stays clean under -D warnings. Co-authored-by: David Spencer <DecisionNerd@users.noreply.github.com>
cursor
Bot
force-pushed
the
cursor/338-file-backed-generations-c6a4
branch
from
August 10, 2026 00:18
0d6b4d3 to
2c61635
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/graphforge-api/src/lib.rs (1)
913-928: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winRestore failure hides the original publication error.
Line 923 uses
?, so ifrematerialize_graph_workspacefails the caller receives aGfError::Storagefrom the restore instead oferror, the publication failure that actually caused the rollback.
publish_composite_attemptincrates/graphforge-api/src/composite_publish.rshandles the same situation deliberately and documents it at lines 264-266: recovery is best-effort and the stable publication error is preserved. This path contradicts that rule.🐛 Proposed fix
if let Err(error) = self.publish_graph_mutation(receipt) { let still_prior = *self .current_generation_uuid .lock() .expect("generation UUID lock poisoned") == expected_generation_before_write; if still_prior { - rematerialize_graph_workspace(rollback_generation, &self.dir)?; - self.adjacency_provider.invalidate(); + // Best-effort recovery: preserve the stable publication error. + let _ = rematerialize_graph_workspace(rollback_generation, &self.dir); + self.adjacency_provider.invalidate(); } return Err(error); }🤖 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/graphforge-api/src/lib.rs` around lines 913 - 928, Update the error branch in the write path around publish_graph_mutation so rematerialize_graph_workspace recovery is best-effort: attempt restoration and invalidate the adjacency provider on success, but do not propagate a restoration failure. Always return the original publication error from publish_graph_mutation, matching the behavior documented in publish_composite_attempt.crates/graphforge-api/src/composite_publish.rs (1)
267-280: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftRollback and reconciliation can leave the workspace partially cleared.
Both sites call
crate::rematerialize_graph_workspace, which deletes the contents ofself.dirbefore repopulating it (seecrates/graphforge-api/src/lib.rslines 3194-3226). The clear is destructive and the operation is not atomic.At line 269 the result is discarded with
.is_ok(). If the call fails after the clear,prior_catalogis not restored, the original publication error is returned, and the facade keeps serving queries against a truncated workspace.The root cause is in
rematerialize_graph_workspace; see the comment oncrates/graphforge-api/src/lib.rslines 3190-3239.Also applies to: 514-519
🤖 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/graphforge-api/src/composite_publish.rs` around lines 267 - 280, Make rematerialize_graph_workspace atomic so failures never leave self.dir cleared or partially repopulated: materialize into a temporary workspace and replace the destination only after success, or restore the original contents on failure. Update the rollback and reconciliation call sites in the composite publish flow to propagate or handle the rematerialization error before updating runtime_catalog, preserving the prior workspace and catalog when it fails.
🧹 Nitpick comments (6)
crates/graphforge-storage/src/project_publication.rs (1)
793-798: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winUse the bounded participant read instead of
std::fs::read.Line 796 reads the staged participant file with an unbounded
std::fs::read. Every other participant read in this crate is bounded by the declared length, for exampleread_exact_participantincrates/graphforge-storage/src/project_generation.rsline 353 andread_bounded_regular_file.
verify_participant_fileruns over the participant set before both call sites, so the length is checked in practice today. Using the bounded read keeps this consistent with the crate's hardening pattern and removes the dependence on call ordering.
StagedParticipantcarries the declared byte length; pass it to the bounded reader.🤖 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/graphforge-storage/src/project_publication.rs` around lines 793 - 798, Update the participant read in verify_participant_file to use the crate’s bounded reader instead of std::fs::read, passing the declared byte length from StagedParticipant. Preserve the existing publication_io error mapping and inventory decoding flow.crates/graphforge-storage/src/graph_files.rs (3)
356-386: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument the verification precondition and drop the redundant match arm.
Two small items:
pinned_open_evidencereportsfiles_validatedandbytes_validatedbut performs no validation. Both current callers runverify_graph_treefirst. Since the function is public, state that precondition in the doc comment so a future caller does not publish unearned evidence.- Line 379 matches
"runtime_catalog.parquet"and line 380 matches any first component that starts withruntime_catalog. The second arm already covers the first, and both returnGraphFileRole::Catalog. Remove the explicit arm.♻️ Proposed change
-/// Open evidence for a read-only pin directly onto the generation tree. +/// Open evidence for a read-only pin directly onto the generation tree. +/// +/// Callers must run [`verify_graph_tree`] against the same tree and inventory +/// before recording this evidence. #[must_use] pub fn pinned_open_evidence(inventory: &GraphFilesInventory) -> GraphFilesOpenEvidence { @@ "indexes" | "index" => GraphFileRole::Index, - "runtime_catalog.parquet" => GraphFileRole::Catalog, _ if first.starts_with("runtime_catalog") => GraphFileRole::Catalog,🤖 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/graphforge-storage/src/graph_files.rs` around lines 356 - 386, Update the documentation for pinned_open_evidence to state that callers must run verify_graph_tree before using its evidence fields, then remove the redundant "runtime_catalog.parquet" match arm from infer_role while preserving the existing runtime_catalog prefix handling.
487-516: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueBound the collected path count during traversal.
collect_source_filesappends every discovered file before any limit applies.build_inventorychecksMAX_GRAPH_FILESonly after the full traversal returns. A workspace with far more than 100,000 files allocates everyPathBufbefore the rejection.collect_regular_filesat line 548 already enforces the limit inside its loop.Apply the same in-loop check here for symmetry and bounded memory.
♻️ Proposed change
if name.ends_with(".lock") || name.starts_with(".gf-stage-") { continue; } paths.push(path); + if paths.len() > MAX_GRAPH_FILES { + return Err(resource_limit("graph files count exceeds limit")); + }🤖 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/graphforge-storage/src/graph_files.rs` around lines 487 - 516, Update collect_source_files to enforce MAX_GRAPH_FILES immediately before or after appending each regular file, returning the existing validation error once the limit is reached. Keep traversal behavior unchanged for directories and excluded entries, and retain build_inventory’s final check as applicable.
794-807: 🔒 Security & Privacy | 🔵 Trivial | ⚡ Quick winAdd a negative test for a traversal path in decoded inventory JSON.
decode_inventoryis the trust boundary for inventory bytes read from a generation.validate_relative_pathguards it, but no test feeds a hostilerelative_paththroughdecode_inventory. Add a case with"relative_path": "../escape"and one with an absolute path, and assert both are rejected.A second useful case: an inventory whose
total_byte_lengthdisagrees with the sum of entries, to lock in the aggregate check at line 479.🤖 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/graphforge-storage/src/graph_files.rs` around lines 794 - 807, Extend the inventory decoding tests around decode_inventory with hostile entries using "../escape" and an absolute relative_path, asserting both are rejected. Add a separate decoded-inventory case whose total_byte_length differs from the entry sum and assert validation fails, preserving the existing valid and unsupported-version coverage.crates/graphforge-api/src/lib.rs (2)
343-349: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winThe workspace guard is a dummy in the pinned read-only case.
hydrate_graph_workspacereturns the generation tree asdirfor a read-only open, plus a freshly created emptyTempDirpurely to satisfy this non-optional field (lines 3134-3146). That temp directory guards nothing. The real lifetime guarantee for a pinned tree comes from theArc<GenerationLease>insideresolved_generation.The doc on line 346 states the field keeps the private mutable workspace alive. In the pinned case there is no private workspace, so the doc is misleading and every read-only open creates and deletes an unused directory.
Change the field to
Option<Arc<tempfile::TempDir>>and returnNonefor the pinned path, then update the doc to say the guard is present only for materialized workspaces.🤖 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/graphforge-api/src/lib.rs` around lines 343 - 349, Change the workspace_guard field on the graph workspace struct to Option<Arc<tempfile::TempDir>> and update its documentation to state it is only present for materialized workspaces. In hydrate_graph_workspace, return None for the pinned read-only generation path instead of creating an unused TempDir, while retaining Some(...) for private mutable workspaces and updating all affected callers to handle the optional guard.
3133-3147: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winEnforce the pinned tree as read-only at the internal write boundary.
diris the immutable generation tree in this branch. PublicCheckpointViewmethods currently reject writes, butGraphForge::run_planand publication helpers do not checkread_only. Add a centralized guard and document this invariant to prevent future paths from modifying the digest-covered tree.🤖 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/graphforge-api/src/lib.rs` around lines 3133 - 3147, Enforce the read-only invariant at the centralized internal write boundary used by GraphForge::run_plan and publication helpers, rejecting writes whenever the pinned tree is selected by the read_only branch. Ensure every path that could mutate or publish the digest-covered generation tree passes through this guard, and document that the pinned tree must never be modified.
🤖 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/graphforge-api/src/bulk_construction.rs`:
- Around line 495-497: Bind rollback and publication handling to the validated
parent generation rather than only normalized.source_generation_uuid. After
resolve_project_generation, compare its UUID with expected_parent before
writing; on publication failure, re-resolve CURRENT and reconcile the workspace
to the durable generation whenever the parent changed, including the still_prior
paths. Add a two-writer regression test covering an intervening publication and
ensuring no newer generation is materialized into an instance reporting the
older parent.
In `@crates/graphforge-api/src/checkpoints.rs`:
- Around line 1023-1025: Update the file-backed branch guarded by the graph
capability and “snapshot”/“files” record families so revert validation does not
unboundedly collect all fingerprints in logical_records. Prefer streaming
validation; otherwise enforce a documented maximum and return a structured
oversized-revert error before allocation exceeds the limit. Add a test covering
an oversized file-backed revert through validate_revert_source.
In `@crates/graphforge-api/src/lib.rs`:
- Around line 3194-3226: Update rematerialize_graph_workspace in
crates/graphforge-api/src/lib.rs:3194-3226 to resolve and validate the inventory
or legacy snapshot before clearing anything, materialize into a sibling
directory, then atomically rename it over the target; preserve the existing
workspace when no valid source exists or validation fails. In
crates/graphforge-api/src/composite_publish.rs:267-280, retain the .is_ok()
best-effort contract and document that atomic failure preserves the workspace.
In crates/graphforge-api/src/composite_publish.rs:514-519, verify
reconcile_workspace_to callers can still serve reads after ? propagates a failed
reconcile. In crates/graphforge-api/src/lib.rs:913-928, use the atomic
rematerialization path for rollback and preserve the original error instead of
replacing it.
In `@crates/graphforge-storage/src/graph_files.rs`:
- Around line 577-587: Make copy_regular_file accept a durable boolean and
invoke sync_file(destination) only when true. In
crates/graphforge-storage/src/graph_files.rs:235-241, have stage_graph_tree pass
true and remove its redundant sync_file call; in
crates/graphforge-storage/src/graph_files.rs:340-351, have
materialize_graph_tree pass false for its temporary workspace.
- Around line 389-403: Update the file collection flow around
collect_source_files and build_inventory to derive each file’s validated
relative path string before sorting, then iterate in that string order so
emitted inventory paths satisfy validate_inventory_contract. Preserve the
existing path validation and limits, and add a unit test covering
topology/nodes.parquet alongside topology-meta.json to verify the canonical
ordering.
- Around line 638-642: Update sync_directory in graph_files.rs to reuse the
platform-aware crate::project_publication::sync_directory implementation, or
move that implementation into a shared helper, instead of unconditionally
opening the directory with File::open. Ensure stage_graph_tree publication
succeeds on Windows, and add a Windows-specific graph-files publication test
covering the directory sync path.
- Around line 708-710: Update resource_limit to construct the structured GfError
variant that maps code() to GF_RESOURCE_LIMIT instead of GfError::Execution.
Remove the unnecessary GF_RESOURCE_LIMIT message prefix while preserving the
supplied message content.
- Around line 644-662: Update path_text to return slash-normalized path text,
and ensure inventory observed keys use this normalized representation. Rebuild
filesystem paths by splitting the normalized `/`-separated components rather
than relying on platform-native separators. In validate_relative_path, remove
the redundant ParentDir and RootDir matches while preserving rejection of
non-normal components and absolute or empty paths.
In `@crates/graphforge-storage/src/project_publication.rs`:
- Around line 763-781: Eliminate redundant graph-tree verification across the
publication and generation flow. In
crates/graphforge-storage/src/project_publication.rs lines 763-781, verify the
carried-forward parent tree once and pass that result into stage_graph_tree; in
crates/graphforge-storage/src/graph_files.rs lines 256-258, remove or debug-gate
stage_graph_tree’s trailing full verify_graph_tree call. At
crates/graphforge-storage/src/project_publication.rs lines 880 and 1095, retain
verification at only one durability boundary. In
crates/graphforge-storage/src/project_generation.rs lines 146-168, split
inventory decoding into verifying and non-verifying variants so
rematerialize_graph_workspace avoids re-verifying before materialize_graph_tree.
In `@tests/contracts/m4-entry-matrix.json`:
- Around line 217-219: The evidence artifact path in the m4 entry must match the
path produced by the documented regeneration command in
file_backed_graph_generation.rs::oversize_file_backed_generation_exceeds_legacy_snapshot_envelope.
Align the contract’s evidence_artifact value and notes with
build/file-backed-oversize-evidence.json, or explicitly document the environment
variable required to generate the docs/development path.
---
Outside diff comments:
In `@crates/graphforge-api/src/composite_publish.rs`:
- Around line 267-280: Make rematerialize_graph_workspace atomic so failures
never leave self.dir cleared or partially repopulated: materialize into a
temporary workspace and replace the destination only after success, or restore
the original contents on failure. Update the rollback and reconciliation call
sites in the composite publish flow to propagate or handle the rematerialization
error before updating runtime_catalog, preserving the prior workspace and
catalog when it fails.
In `@crates/graphforge-api/src/lib.rs`:
- Around line 913-928: Update the error branch in the write path around
publish_graph_mutation so rematerialize_graph_workspace recovery is best-effort:
attempt restoration and invalidate the adjacency provider on success, but do not
propagate a restoration failure. Always return the original publication error
from publish_graph_mutation, matching the behavior documented in
publish_composite_attempt.
---
Nitpick comments:
In `@crates/graphforge-api/src/lib.rs`:
- Around line 343-349: Change the workspace_guard field on the graph workspace
struct to Option<Arc<tempfile::TempDir>> and update its documentation to state
it is only present for materialized workspaces. In hydrate_graph_workspace,
return None for the pinned read-only generation path instead of creating an
unused TempDir, while retaining Some(...) for private mutable workspaces and
updating all affected callers to handle the optional guard.
- Around line 3133-3147: Enforce the read-only invariant at the centralized
internal write boundary used by GraphForge::run_plan and publication helpers,
rejecting writes whenever the pinned tree is selected by the read_only branch.
Ensure every path that could mutate or publish the digest-covered generation
tree passes through this guard, and document that the pinned tree must never be
modified.
In `@crates/graphforge-storage/src/graph_files.rs`:
- Around line 356-386: Update the documentation for pinned_open_evidence to
state that callers must run verify_graph_tree before using its evidence fields,
then remove the redundant "runtime_catalog.parquet" match arm from infer_role
while preserving the existing runtime_catalog prefix handling.
- Around line 487-516: Update collect_source_files to enforce MAX_GRAPH_FILES
immediately before or after appending each regular file, returning the existing
validation error once the limit is reached. Keep traversal behavior unchanged
for directories and excluded entries, and retain build_inventory’s final check
as applicable.
- Around line 794-807: Extend the inventory decoding tests around
decode_inventory with hostile entries using "../escape" and an absolute
relative_path, asserting both are rejected. Add a separate decoded-inventory
case whose total_byte_length differs from the entry sum and assert validation
fails, preserving the existing valid and unsupported-version coverage.
In `@crates/graphforge-storage/src/project_publication.rs`:
- Around line 793-798: Update the participant read in verify_participant_file to
use the crate’s bounded reader instead of std::fs::read, passing the declared
byte length from StagedParticipant. Preserve the existing publication_io error
mapping and inventory decoding flow.
🪄 Autofix
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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 85358817-58ff-4d10-a692-95a26ca691df
⛔ Files ignored due to path filters (7)
benchmarks/m4_entry_baseline.mdis excluded by!**/*.mddocs/book/architecture/project-format-compatibility.mdis excluded by!**/*.md,!**/docs/**docs/book/architecture/storage.mdis excluded by!**/*.md,!**/docs/**docs/development/bazel-migration-ledger.mdis excluded by!**/*.md,!**/docs/**docs/development/file-backed-oversize-evidence.jsonis excluded by!**/docs/**docs/development/m4-entry-baseline.mdis excluded by!**/*.md,!**/docs/**docs/reference/scale-limits.mdis excluded by!**/*.md,!**/docs/**
📒 Files selected for processing (20)
crates/graphforge-api/BUILD.bazelcrates/graphforge-api/src/bulk_construction.rscrates/graphforge-api/src/checkpoints.rscrates/graphforge-api/src/composite_publish.rscrates/graphforge-api/src/embedding_refresh.rscrates/graphforge-api/src/lib.rscrates/graphforge-api/src/provenance.rscrates/graphforge-api/tests/file_backed_graph_generation.rscrates/graphforge-api/tests/strict_runtime_properties.rscrates/graphforge-bindings-py/tests/non_cypher_release.pycrates/graphforge-storage/src/graph_files.rscrates/graphforge-storage/src/lib.rscrates/graphforge-storage/src/project_checkpoints.rscrates/graphforge-storage/src/project_generation.rscrates/graphforge-storage/src/project_portable.rscrates/graphforge-storage/src/project_publication.rsscripts/ci/test-non-cypher-surface-gate.pytests/contracts/m4-entry-matrix.jsontests/contracts/non-cypher-rust-surface.jsontools/bazel/parity/migration_target_map.json
Comment on lines
+495
to
+497
| let prior_generation = graphforge_storage::resolve_project_generation( | ||
| self.resolved_generation.container_root(), | ||
| )?; |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Bind rollback to the validated parent generation.
normalized.source_generation_uuid comes from the local instance, but resolve_project_generation reads the current durable CURRENT record. If another writer publishes between these operations, prior_generation can have a different UUID. Publication then rejects the stale parent, but still_prior compares only the stale local UUID. Lines 563 and 849 can materialize the newer generation into an instance that still reports the older generation.
Compare the resolved generation UUID with expected_parent before writing. On publication failure, re-resolve CURRENT and reconcile the workspace to the durable generation when the parent changed. Add a two-writer regression test.
Also applies to: 563-563, 768-770, 849-849
🤖 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/graphforge-api/src/bulk_construction.rs` around lines 495 - 497, Bind
rollback and publication handling to the validated parent generation rather than
only normalized.source_generation_uuid. After resolve_project_generation,
compare its UUID with expected_parent before writing; on publication failure,
re-resolve CURRENT and reconcile the workspace to the durable generation
whenever the parent changed, including the still_prior paths. Add a two-writer
regression test covering an intervening publication and ensuring no newer
generation is materialized into an instance reporting the older parent.
Comment on lines
+1023
to
+1025
| if descriptor.capability_id == "graph" | ||
| && matches!(descriptor.record_family_id.as_str(), "snapshot" | "files") | ||
| { |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Keep file-backed revert validation bounded.
The files branch extracts all logical node and edge records and inserts every fingerprint into logical_records's BTreeMap. validate_revert_source calls this at Line 1317 before the storage layer stages the restore. A file-backed generation can exceed the legacy snapshot envelope, so revert validation can still allocate O(N) memory and fail before the revert starts.
Stream the validation or enforce a documented resource limit with a structured error. Add an oversized file-backed revert test.
🤖 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/graphforge-api/src/checkpoints.rs` around lines 1023 - 1025, Update
the file-backed branch guarded by the graph capability and “snapshot”/“files”
record families so revert validation does not unboundedly collect all
fingerprints in logical_records. Prefer streaming validation; otherwise enforce
a documented maximum and return a structured oversized-revert error before
allocation exceeds the limit. Add a test covering an oversized file-backed
revert through validate_revert_source.
Comment on lines
+3194
to
+3226
| if target.exists() { | ||
| for entry in std::fs::read_dir(target).map_err(|error| { | ||
| GfError::Storage(format!( | ||
| "failed to read graph workspace for restore: {error}" | ||
| )) | ||
| })? { | ||
| let entry = entry.map_err(|error| { | ||
| GfError::Storage(format!("failed to read graph workspace entry: {error}")) | ||
| })?; | ||
| let path = entry.path(); | ||
| let file_type = entry.file_type().map_err(|error| { | ||
| GfError::Storage(format!("failed to inspect graph workspace entry: {error}")) | ||
| })?; | ||
| if file_type.is_dir() { | ||
| std::fs::remove_dir_all(&path).map_err(|error| { | ||
| GfError::Storage(format!( | ||
| "failed to clear graph workspace directory: {error}" | ||
| )) | ||
| })?; | ||
| } else { | ||
| std::fs::remove_file(&path).map_err(|error| { | ||
| GfError::Storage(format!("failed to clear graph workspace file: {error}")) | ||
| })?; | ||
| } | ||
| } | ||
| } | ||
| Ok(workspace) | ||
| if let Some(inventory) = generation.graph_files_inventory()? { | ||
| graphforge_storage::materialize_graph_tree( | ||
| &generation.graph_tree_root(), | ||
| &inventory, | ||
| target, | ||
| )?; | ||
| return Ok(()); |
There was a problem hiding this comment.
🩺 Stability & Availability | 🔴 Critical | 🏗️ Heavy lift
rematerialize_graph_workspace empties the live workspace before it establishes that the source generation can refill it, so every caller can be left pointing at an empty directory. The clear loop runs first; graph_files_inventory() and its verify_graph_tree run afterward and can fail on a corrupt or mismatched tree. A generation declaring neither graph/files nor graph/snapshot also returns Ok(()) with the workspace wiped.
crates/graphforge-api/src/lib.rs#L3194-L3226: resolve and validate the inventory or the legacy snapshot participant before the clear loop, and materialize into a sibling directory that you rename over the target so the operation is atomic.crates/graphforge-api/src/composite_publish.rs#L267-L280: once the call is atomic, a failure leaves the prior workspace intact; keep the.is_ok()best-effort contract but add a comment noting that failure no longer damages the workspace.crates/graphforge-api/src/composite_publish.rs#L514-L519:reconcile_workspace_topropagates the error with?; confirm the caller can still serve reads after a failed reconcile once the workspace is preserved.crates/graphforge-api/src/lib.rs#L913-L928: this write-path rollback both loses the original publication error and depends on the same destructive clear; apply the atomic version and preserveerror.
📍 Affects 2 files
crates/graphforge-api/src/lib.rs#L3194-L3226(this comment)crates/graphforge-api/src/composite_publish.rs#L267-L280crates/graphforge-api/src/composite_publish.rs#L514-L519crates/graphforge-api/src/lib.rs#L913-L928
🤖 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/graphforge-api/src/lib.rs` around lines 3194 - 3226, Update
rematerialize_graph_workspace in crates/graphforge-api/src/lib.rs:3194-3226 to
resolve and validate the inventory or legacy snapshot before clearing anything,
materialize into a sibling directory, then atomically rename it over the target;
preserve the existing workspace when no valid source exists or validation fails.
In crates/graphforge-api/src/composite_publish.rs:267-280, retain the .is_ok()
best-effort contract and document that atomic failure preserves the workspace.
In crates/graphforge-api/src/composite_publish.rs:514-519, verify
reconcile_workspace_to callers can still serve reads after ? propagates a failed
reconcile. In crates/graphforge-api/src/lib.rs:913-928, use the atomic
rematerialization path for rollback and preserve the original error instead of
replacing it.
Comment on lines
+389
to
+403
| let mut paths = Vec::new(); | ||
| collect_source_files(source_root, &mut paths)?; | ||
| paths.sort(); | ||
| if paths.len() > MAX_GRAPH_FILES { | ||
| return Err(resource_limit("graph files count exceeds limit")); | ||
| } | ||
| let mut files = Vec::with_capacity(paths.len()); | ||
| let mut total = 0_u64; | ||
| let mut seen = HashSet::new(); | ||
| for path in paths { | ||
| let relative = path | ||
| .strip_prefix(source_root) | ||
| .map_err(|_| validation("graph file path escaped workspace"))?; | ||
| validate_relative_path(relative)?; | ||
| let relative_text = path_text(relative)?; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Sort by the normalized relative path string, not by PathBuf.
paths.sort() orders PathBuf values component-by-component. validate_inventory_contract at line 455 requires strict ascending order of the joined relative_path strings. These two orders are not the same, because component comparison never sees the / separator byte.
Example: a workspace with the directory topology/ and a sibling file topology-meta.json. Component order puts topology/nodes.parquet first, because "topology" < "topology-meta.json". String order puts "topology-meta.json" first, because - (0x2D) is less than / (0x2F). build_inventory then emits entries that its own contract check rejects with "graph files inventory paths are duplicate or non-canonical", so capture_graph_files fails on a valid workspace.
Build the relative path strings first and sort those, so the emitted order matches the validated order by construction.
🐛 Proposed fix
fn build_inventory(source_root: &Path) -> Result<GraphFilesInventory, GfError> {
let mut paths = Vec::new();
collect_source_files(source_root, &mut paths)?;
- paths.sort();
if paths.len() > MAX_GRAPH_FILES {
return Err(resource_limit("graph files count exceeds limit"));
}
+ let mut entries = Vec::with_capacity(paths.len());
+ for path in paths {
+ let relative = path
+ .strip_prefix(source_root)
+ .map_err(|_| validation("graph file path escaped workspace"))?;
+ validate_relative_path(relative)?;
+ entries.push((path_text(relative)?, path.clone(), infer_role(relative)));
+ }
+ entries.sort_by(|left, right| left.0.cmp(&right.0));
let mut files = Vec::with_capacity(paths.len());
let mut total = 0_u64;
let mut seen = HashSet::new();
- for path in paths {
- let relative = path
- .strip_prefix(source_root)
- .map_err(|_| validation("graph file path escaped workspace"))?;
- validate_relative_path(relative)?;
- let relative_text = path_text(relative)?;
+ for (relative_text, path, role) in entries {Add a unit test that captures a workspace containing both a directory and a sibling file whose name sorts between them, for example topology/nodes.parquet and topology-meta.json.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| let mut paths = Vec::new(); | |
| collect_source_files(source_root, &mut paths)?; | |
| paths.sort(); | |
| if paths.len() > MAX_GRAPH_FILES { | |
| return Err(resource_limit("graph files count exceeds limit")); | |
| } | |
| let mut files = Vec::with_capacity(paths.len()); | |
| let mut total = 0_u64; | |
| let mut seen = HashSet::new(); | |
| for path in paths { | |
| let relative = path | |
| .strip_prefix(source_root) | |
| .map_err(|_| validation("graph file path escaped workspace"))?; | |
| validate_relative_path(relative)?; | |
| let relative_text = path_text(relative)?; | |
| let mut paths = Vec::new(); | |
| collect_source_files(source_root, &mut paths)?; | |
| if paths.len() > MAX_GRAPH_FILES { | |
| return Err(resource_limit("graph files count exceeds limit")); | |
| } | |
| let mut entries = Vec::with_capacity(paths.len()); | |
| for path in paths { | |
| let relative = path | |
| .strip_prefix(source_root) | |
| .map_err(|_| validation("graph file path escaped workspace"))?; | |
| validate_relative_path(relative)?; | |
| entries.push((path_text(relative)?, path.clone(), infer_role(relative))); | |
| } | |
| entries.sort_by(|left, right| left.0.cmp(&right.0)); | |
| let mut files = Vec::with_capacity(paths.len()); | |
| let mut total = 0_u64; | |
| let mut seen = HashSet::new(); | |
| for (relative_text, path, role) in entries { |
🤖 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/graphforge-storage/src/graph_files.rs` around lines 389 - 403, Update
the file collection flow around collect_source_files and build_inventory to
derive each file’s validated relative path string before sorting, then iterate
in that string order so emitted inventory paths satisfy
validate_inventory_contract. Preserve the existing path validation and limits,
and add a unit test covering topology/nodes.parquet alongside topology-meta.json
to verify the canonical ordering.
Comment on lines
+577
to
+587
| fn copy_regular_file(source: &Path, destination: &Path) -> Result<[u8; 32], GfError> { | ||
| reject_link(source)?; | ||
| // Prefer filesystem copy so sparse/holey sources stay sparse when the OS | ||
| // supports it (Linux copy_file_range). Digest the destination so staged | ||
| // bytes remain verified without assembling them into one buffer. | ||
| fs::copy(source, destination) | ||
| .map_err(|error| storage("copy graph source file", destination, error))?; | ||
| let digest = hash_file(destination)?; | ||
| sync_file(destination)?; | ||
| Ok(digest) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | ⚡ Quick win
One unconditional fsync in copy_regular_file causes both a duplicate sync during staging and an unnecessary sync during private materialization. copy_regular_file always calls sync_file(destination), so its two callers cannot choose the durability they need: stage_graph_tree needs it and adds a second redundant call, while materialize_graph_tree writes into a discarded temp workspace and needs none.
crates/graphforge-storage/src/graph_files.rs#L577-L587: add adurable: boolparameter and callsync_fileonly when it is set.crates/graphforge-storage/src/graph_files.rs#L235-L241: passtrueand delete the now-redundantsync_file(&destination)?on line 241.crates/graphforge-storage/src/graph_files.rs#L340-L351: passfalse, because the target is a caller-ownedtempfile::TempDirand not a durability boundary.
📍 Affects 1 file
crates/graphforge-storage/src/graph_files.rs#L577-L587(this comment)crates/graphforge-storage/src/graph_files.rs#L235-L241crates/graphforge-storage/src/graph_files.rs#L340-L351
🤖 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/graphforge-storage/src/graph_files.rs` around lines 577 - 587, Make
copy_regular_file accept a durable boolean and invoke sync_file(destination)
only when true. In crates/graphforge-storage/src/graph_files.rs:235-241, have
stage_graph_tree pass true and remove its redundant sync_file call; in
crates/graphforge-storage/src/graph_files.rs:340-351, have
materialize_graph_tree pass false for its temporary workspace.
Comment on lines
+638
to
+642
| fn sync_directory(path: &Path) -> Result<(), GfError> { | ||
| let file = File::open(path).map_err(|error| storage("open graph directory", path, error))?; | ||
| file.sync_all() | ||
| .map_err(|error| storage("fsync graph directory", path, error)) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Compare directory-fsync helpers and check for Windows targets in CI/manifests.
set -euo pipefail
rg -n -B5 -A15 'fn sync_directory' --type rust crates/graphforge-storage/src
rg -n -C3 'windows|BACKUP_SEMANTICS|cfg\(windows\)' --type rust crates/graphforge-storage/src
fd -t f 'Cargo.toml' crates/graphforge-storage --exec cat {}Repository: CurateLabs/graphforge
Length of output: 48086
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- graph_files imports, callers, and publication path ---'
sed -n '1,80p' crates/graphforge-storage/src/graph_files.rs
rg -n -C8 'sync_directory_tree|stage_graph_tree|publish_graph|graph_files' crates/graphforge-storage/src/graph_files.rs crates/graphforge-storage/src --type rust
printf '%s\n' '--- platform-specific module and workspace targets ---'
rg -n -C4 'mod graph_files|pub mod graph_files|stage_graph_tree|windows|ci_rust_tests' crates/graphforge-storage/src BUILD.bazel WORKSPACE* MODULE.bazel .github 2>/dev/null || trueRepository: CurateLabs/graphforge
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- exact call chain and helper definitions ---'
sed -n '245,260p' crates/graphforge-storage/src/graph_files.rs
sed -n '760,783p' crates/graphforge-storage/src/project_publication.rs
sed -n '1858,1888p' crates/graphforge-storage/src/project_publication.rs
printf '%s\n' '--- Windows CI and graph publication coverage ---'
rg -n -C5 'windows-2025|windows|ci_rust_tests|graph_files|stage_graph_tree' .github BUILD.bazel crates/graphforge-storage --glob '*.yml' --glob '*.yaml' --glob '*.bzl' --glob '*.rs' --glob 'BUILD*'Repository: CurateLabs/graphforge
Length of output: 50377
Use the platform-aware directory sync helper. On Windows, stage_graph_tree calls this unconditional File::open(path) helper before publication completes. Directory opens require FILE_FLAG_BACKUP_SEMANTICS, so every graph/files publication fails. Reuse crate::project_publication::sync_directory or move its platform-specific implementation to a shared helper. Add a Windows graph-files publication test.
🤖 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/graphforge-storage/src/graph_files.rs` around lines 638 - 642, Update
sync_directory in graph_files.rs to reuse the platform-aware
crate::project_publication::sync_directory implementation, or move that
implementation into a shared helper, instead of unconditionally opening the
directory with File::open. Ensure stage_graph_tree publication succeeds on
Windows, and add a Windows-specific graph-files publication test covering the
directory sync path.
Comment on lines
+644
to
+662
| fn validate_relative_path(path: &Path) -> Result<(), GfError> { | ||
| if path.as_os_str().is_empty() | ||
| || path.is_absolute() | ||
| || path.components().any(|component| { | ||
| !matches!(component, Component::Normal(_)) | ||
| || matches!(component, Component::ParentDir | Component::RootDir) | ||
| }) | ||
| { | ||
| return Err(validation("invalid graph file relative path")); | ||
| } | ||
| let _ = path_text(path)?; | ||
| Ok(()) | ||
| } | ||
|
|
||
| fn path_text(path: &Path) -> Result<String, GfError> { | ||
| path.to_str() | ||
| .map(str::to_owned) | ||
| .ok_or_else(|| validation("graph file path is not UTF-8")) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Check whether Windows is a supported target and how other persisted paths normalize separators.
set -euo pipefail
rg -n -C3 'MAIN_SEPARATOR' --type rust crates
fd -t f -e yml -e yaml . .github --exec rg -n -C2 'windows|runs-on' {} \;Repository: CurateLabs/graphforge
Length of output: 12606
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- graph_files.rs structure and relevant symbols ---'
ast-grep outline crates/graphforge-storage/src/graph_files.rs
rg -n -C6 'validate_relative_path|path_text|validate_inventory_contract|verify_graph_tree|GraphFilesInventory|relative_path|join\\(' \
crates/graphforge-storage/src/graph_files.rs \
crates/graphforge-storage/src/project_generation.rs
printf '%s\n' '--- focused source slices ---'
sed -n '1,180p' crates/graphforge-storage/src/graph_files.rs
sed -n '560,760p' crates/graphforge-storage/src/graph_files.rs
sed -n '360,450p' crates/graphforge-storage/src/project_generation.rs
printf '%s\n' '--- all path-related tests and inventory fixtures ---'
rg -n -C4 'graph/files|GraphFiles|relative path|relative_path|topology|nodes\\.parquet|separator|MAIN_SEPARATOR' \
crates tests .github 2>/dev/null || trueRepository: CurateLabs/graphforge
Length of output: 3768
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- graph_files.rs public flow and inventory construction ---'
sed -n '110,460p' crates/graphforge-storage/src/graph_files.rs
printf '%s\n' '--- graph_files.rs path validation and tests ---'
sed -n '630,820p' crates/graphforge-storage/src/graph_files.rs
printf '%s\n' '--- project_generation normalization context ---'
sed -n '390,440p' crates/graphforge-storage/src/project_generation.rs
printf '%s\n' '--- path consumers and related tests ---'
rg -n -F -C4 'validate_inventory_contract' crates
rg -n -F -C4 'verify_graph_tree' crates
rg -n -F -C4 'relative_path' crates/graphforge-storage/src
rg -n -C3 'graph/files|nodes\.parquet|MAIN_SEPARATOR|path separator|separator' crates .github 2>/dev/null || trueRepository: CurateLabs/graphforge
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import PurePosixPath, PureWindowsPath
native_windows = r"topology\nodes.parquet"
normalized = "topology/nodes.parquet"
print("Windows-origin inventory text:", repr(native_windows))
print("Unix interpretation:", PurePosixPath(native_windows).parts)
print("Windows interpretation:", PureWindowsPath(native_windows).parts)
print("Normalized Unix interpretation:", PurePosixPath(normalized).parts)
print("Normalized Windows interpretation:", PureWindowsPath(normalized).parts)
source = open("crates/graphforge-storage/src/graph_files.rs", encoding="utf-8").read()
required = {
"native path text is stored": "let relative_text = path_text(relative)?;" in source,
"inventory paths are rebuilt directly": 'Path::new(&entry.relative_path)' in source,
"tree observations also use native path text": "let key = path_text(relative)?" in source,
}
for label, present in required.items():
print(f"{label}: {present}")
PY
printf '%s\n' '--- exact inventory contract and path observation slices ---'
sed -n '436,550p' crates/graphforge-storage/src/graph_files.rsRepository: CurateLabs/graphforge
Length of output: 5308
Normalize all inventory paths to /.
path_text currently stores and compares platform-native separators. A Windows entry such as topology\nodes.parquet is one filename on Unix, so cross-platform verification and materialization fail. Return slash-normalized text from path_text, use it for observed keys, and rebuild filesystem paths from /-separated components. Remove the redundant ParentDir | RootDir check.
🤖 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/graphforge-storage/src/graph_files.rs` around lines 644 - 662, Update
path_text to return slash-normalized path text, and ensure inventory observed
keys use this normalized representation. Rebuild filesystem paths by splitting
the normalized `/`-separated components rather than relying on platform-native
separators. In validate_relative_path, remove the redundant ParentDir and
RootDir matches while preserving rejection of non-normal components and absolute
or empty paths.
Comment on lines
+708
to
+710
| fn resource_limit(message: impl Into<String>) -> GfError { | ||
| GfError::Execution(format!("GF_RESOURCE_LIMIT: {}", message.into())) | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Find callers that depend on the GF_RESOURCE_LIMIT message prefix or the GF_EXECUTION code for limit errors.
set -euo pipefail
rg -n -C3 'GF_RESOURCE_LIMIT' --type rust
rg -n -C3 'ProjectErrorCode::ResourceLimit' --type rustRepository: CurateLabs/graphforge
Length of output: 159
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- error definitions ---'
rg -n -C5 'enum ProjectErrorCode|ResourceLimit|fn code|impl.*GfError' crates/graphforge-core crates/graphforge-storage --type rust
printf '%s\n' '--- resource_limit definition and callers ---'
rg -n -C5 'resource_limit\(' crates/graphforge-storage --type rust
printf '%s\n' '--- all limit-related references ---'
rg -n -C3 'GF_RESOURCE_LIMIT|GF_EXECUTION|ResourceLimit' --type rust . || trueRepository: CurateLabs/graphforge
Length of output: 50378
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '%s\n' '--- graph_files imports and helper context ---'
sed -n '1,45p;675,715p' crates/graphforge-storage/src/graph_files.rs
printf '%s\n' '--- GfError code and display implementation ---'
sed -n '150,205p;250,325p' crates/graphforge-core/src/lib.rs
printf '%s\n' '--- exact resource-limit consumers ---'
rg -n 'code\(\)|GF_RESOURCE_LIMIT:|GF_RESOURCE_LIMIT|ProjectErrorCode::ResourceLimit' \
crates --type rust \
| rg -v 'src/lib.rs:[0-9]+:.*(ResourceLimit|GF_RESOURCE_LIMIT)' \
|| trueRepository: CurateLabs/graphforge
Length of output: 50377
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
import re
graph_files = Path("crates/graphforge-storage/src/graph_files.rs").read_text()
core = Path("crates/graphforge-core/src/lib.rs").read_text()
helper = re.search(
r"fn resource_limit\(message: impl Into<String>\) -> GfError \{(?P<body>.*?)\n\}",
graph_files,
re.S,
)
assert helper, "resource_limit helper not found"
assert "GfError::Execution" in helper.group("body")
assert "GF_RESOURCE_LIMIT:" in helper.group("body")
code_match = re.search(
r"Self::Project \{ code, \.\. \} => code\.as_str\(\)",
core,
)
assert code_match, "GfError::Project does not delegate code()"
resource_mapping = re.search(
r"Self::ResourceLimit => \"GF_RESOURCE_LIMIT\"",
core,
)
assert resource_mapping, "ProjectErrorCode::ResourceLimit mapping not found"
prefix_users = []
for path in Path("crates").rglob("*.rs"):
text = path.read_text()
for line_no, line in enumerate(text.splitlines(), 1):
if "GF_RESOURCE_LIMIT:" in line and path.as_posix().endswith("graph_files.rs") is False:
prefix_users.append(f"{path}:{line_no}")
assert not prefix_users, prefix_users
print("resource_limit currently returns GF_EXECUTION")
print("ProjectErrorCode::ResourceLimit maps to GF_RESOURCE_LIMIT")
print("no other Rust source uses the GF_RESOURCE_LIMIT: message prefix")
PYRepository: CurateLabs/graphforge
Length of output: 348
🏁 Script executed:
#!/bin/bash
set -euo pipefail
sed -n '255,285p' crates/graphforge-api/src/graph_snapshot.rs
sed -n '220,245p' crates/graphforge-api/src/provenance.rsRepository: CurateLabs/graphforge
Length of output: 1770
Return a structured resource-limit code
resource_limit returns GfError::Execution, so code() returns GF_EXECUTION instead of GF_RESOURCE_LIMIT. Its callers do not depend on the message prefix.
♻️ Proposed fix
fn resource_limit(message: impl Into<String>) -> GfError {
- GfError::Execution(format!("GF_RESOURCE_LIMIT: {}", message.into()))
+ GfError::Project {
+ code: ProjectErrorCode::ResourceLimit,
+ message: message.into(),
+ }
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| fn resource_limit(message: impl Into<String>) -> GfError { | |
| GfError::Execution(format!("GF_RESOURCE_LIMIT: {}", message.into())) | |
| } | |
| fn resource_limit(message: impl Into<String>) -> GfError { | |
| GfError::Project { | |
| code: ProjectErrorCode::ResourceLimit, | |
| message: message.into(), | |
| } | |
| } |
🤖 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/graphforge-storage/src/graph_files.rs` around lines 708 - 710, Update
resource_limit to construct the structured GfError variant that maps code() to
GF_RESOURCE_LIMIT instead of GfError::Execution. Remove the unnecessary
GF_RESOURCE_LIMIT message prefix while preserving the supplied message content.
Comment on lines
+763
to
+781
| let inventory = crate::decode_inventory(&files_participant.bytes)?; | ||
| let parent_tree = parent.graph_tree_root(); | ||
| let source = match graph_tree { | ||
| Some(path) => path, | ||
| None if parent_tree.exists() => { | ||
| crate::verify_graph_tree(&parent_tree, &inventory)?; | ||
| parent_tree.as_path() | ||
| } | ||
| None => { | ||
| return Err(project_error( | ||
| ProjectErrorCode::PublicationFailed, | ||
| "graph/files participant requires a graph_tree source directory", | ||
| )); | ||
| } | ||
| }; | ||
| crate::stage_graph_tree(source, generation_root, &inventory)?; | ||
| sync_directory(generation_root)?; | ||
| Ok(()) | ||
| } |
There was a problem hiding this comment.
🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy lift
Every layer calls verify_graph_tree independently, so one publication digests the whole graph tree up to five times. No caller records that a peer already verified the same bytes, and each verification is a full SHA-256 pass over every file in a tree the PR documents as exceeding 2 GiB.
crates/graphforge-storage/src/project_publication.rs#L763-L781: verify the parent tree once in the carry-forward branch and pass that result down instead of lettingstage_graph_treere-verify the copy it just hashed.crates/graphforge-storage/src/graph_files.rs#L256-L258:stage_graph_treealready compares each destination digest to the inventory inside the copy loop; drop the trailing fullverify_graph_tree(&destination_root, inventory)or gate it behind a debug assertion.crates/graphforge-storage/src/project_publication.rs#L880-L880: keep one verification at a single boundary rather than verifying in bothvalidateandmake_generation_durable.crates/graphforge-storage/src/project_publication.rs#L1095-L1095: this is the second of that pair; retain whichever single site the team chooses as the durability gate.crates/graphforge-storage/src/project_generation.rs#L146-L168: split a non-verifying inventory decode from the verifying variant, sorematerialize_graph_workspacedoes not verify here and again insidematerialize_graph_tree.
📍 Affects 3 files
crates/graphforge-storage/src/project_publication.rs#L763-L781(this comment)crates/graphforge-storage/src/graph_files.rs#L256-L258crates/graphforge-storage/src/project_publication.rs#L880-L880crates/graphforge-storage/src/project_publication.rs#L1095-L1095crates/graphforge-storage/src/project_generation.rs#L146-L168
🤖 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/graphforge-storage/src/project_publication.rs` around lines 763 - 781,
Eliminate redundant graph-tree verification across the publication and
generation flow. In crates/graphforge-storage/src/project_publication.rs lines
763-781, verify the carried-forward parent tree once and pass that result into
stage_graph_tree; in crates/graphforge-storage/src/graph_files.rs lines 256-258,
remove or debug-gate stage_graph_tree’s trailing full verify_graph_tree call. At
crates/graphforge-storage/src/project_publication.rs lines 880 and 1095, retain
verification at only one durability boundary. In
crates/graphforge-storage/src/project_generation.rs lines 146-168, split
inventory decoding into verifying and non-verifying variants so
rematerialize_graph_workspace avoids re-verifying before materialize_graph_tree.
Comment on lines
+217
to
+219
| "path": "crates/graphforge-api/tests/file_backed_graph_generation.rs::oversize_file_backed_generation_exceeds_legacy_snapshot_envelope", | ||
| "evidence_artifact": "docs/development/file-backed-oversize-evidence.json", | ||
| "notes": "Deterministic sparse multi-file generation whose validated bytes exceed 2 GiB; published and reopened through the public GraphForge path without Arrow snapshot hydrate. CI keeps the small fixture only; regenerate via the ignored oversize test into build/." |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Make the evidence artifact path consistent.
Line 218 names docs/development/file-backed-oversize-evidence.json. The documented command in crates/graphforge-api/tests/file_backed_graph_generation.rs Lines 9-11 writes build/file-backed-oversize-evidence.json, and Line 219 directs users to regenerate into build/. A reviewer who follows the documented command does not create the artifact that this contract identifies. Use one path consistently, or document the required environment variable for the contract artifact path.
🤖 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 `@tests/contracts/m4-entry-matrix.json` around lines 217 - 219, The evidence
artifact path in the m4 entry must match the path produced by the documented
regeneration command in
file_backed_graph_generation.rs::oversize_file_backed_generation_exceeds_legacy_snapshot_envelope.
Align the contract’s evidence_artifact value and notes with
build/file-backed-oversize-evidence.json, or explicitly document the environment
variable required to generate the docs/development path.
This was referenced Aug 10, 2026
This was referenced Aug 15, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Publish and reopen graph data as an immutable file-backed project generation so public embedded GraphForge can represent graphs larger than the legacy 1/2 GiB Arrow snapshot envelope without whole-graph in-memory assembly.
Rebased onto
mainafter #337 landed.Changes
graph/filesinventory + staging/verify ingraphforge-storageCURRENTauthorityPinnedInPlace(read-only) or private materialize (writable)CURRENT) so inventory validation stays consistentdocs/development/file-backed-oversize-evidence.jsonHonest limit
Full measured 8M/128M (~15 GiB) public-facade evidence was not executed on this agent VM. Oversize proof uses a deterministic >2 GiB file-backed generation with a real queryable graph plus sparse padding. Optional measured 8M remains for #345.
Testing
Closes #338
Need help on this PR? Tag
@codesmith-botwith what you need. Autofix is disabled.Note
Add file-backed graph generation storage with JSON inventory and graph tree staging
graph/filesparticipant family as an alternative to the legacysnapshot(Arrow IPC) participant, storing the graph workspace as a directory tree with a canonical JSON inventory tracking file paths, sizes, and SHA-256 digests.capture_graph_files,stage_graph_tree,verify_graph_tree, andmaterialize_graph_treein a newgraph_filesmodule to build, stage, verify, and copy graph trees during publication and open.stage_project_generation_with_graph_treeandstage_project_generation_optimistic_with_graph_treeas new public staging APIs that accept an optional explicit graph tree source path.hydrate_graph_workspaceto return open evidence (GraphFilesOpenEvidence) and support two open strategies: pinned in-place (read-only checkpoints) or private materialization (writable sessions); legacy snapshot path is preserved.execute,normalize_bulk_nodes/edges, and composite publish now rematerializes from the resolved prior generation instead of restoring an Arrow snapshot.encode_portable_projectreturns a structuredGF_UNSUPPORTED_PROJECT_FORMATerror when the generation includes agraph/filesparticipant, since portable interchange does not yet encode file-backed graph trees.snapshotandfilesparticipants are explicitly rejected at staging and open time.Macroscope summarized 2c61635.
Summary by CodeRabbit
New Features
Bug Fixes
Limitations