fix: validate state integrity after loading state.json by don-petry · Pull Request #18 · oneirosoft/dagger

don-petry · 2026-03-31T02:18:19Z

Why?

A corrupted or manually edited state.json can cause dgr to panic, silently produce wrong results, or corrupt data further. Early validation with clear error messages makes debugging straightforward and prevents cascading failures.

Summary

Adds a validate() method to DaggerState that checks referential integrity after deserializing state.json: version compatibility, duplicate node IDs, duplicate active branch names, self-referential parents, and dangling parent references
Calls validate() in load_state immediately after deserialization so corrupted state is caught early with clear error messages
Intentionally allows active branches to reference archived parents — this is a valid intermediate state that dgr sync repairs
Adds unit tests covering valid state, version mismatch, duplicate IDs, duplicate active names, dangling parent refs, self-parenting, and archived duplicate names being allowed

Refs: #10 (item 4 — state validation)

Test plan

cargo test --locked passes with all new validation tests
Existing state files load without validation errors
Manually corrupt state.json (e.g. duplicate a node ID) and verify dgr reports a clear error

🤖 Generated with Claude Code

Add a validate() method to DaggerState that checks referential integrity after deserialization: version compatibility, duplicate node IDs, duplicate active branch names, and dangling parent references. This catches corrupted state early with clear error messages instead of causing confusing failures or infinite loops downstream. Refs: oneirosoft#10 Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot

Pull request overview

This PR adds a DaggerState::validate() pass to catch corrupted state.json immediately after deserialization, improving correctness and error clarity for state integrity issues (referential integrity, duplicates, version mismatch).

Changes:

Added DaggerState::validate() to enforce basic state invariants (version, unique node IDs, unique active branch names, parent references exist).
Invoked validate() from load_state() so invalid state fails fast during store initialization/load.
Added unit tests for the new validation behavior.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File	Description
`src/core/store/types.rs`	Implements `DaggerState::validate()` and adds unit tests for version/duplicate/parent-reference validation cases.
`src/core/store/state.rs`	Calls `state.validate()` immediately after JSON deserialization in `load_state()`.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Copilot · 2026-03-31T02:23:42Z

src/core/store/types.rs

+            if let ParentRef::Branch { node_id } = &node.parent {
+                if !all_ids.contains(node_id) {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        format!(
+                            "branch '{}' references non-existent parent node {}",
+                            node.branch_name, node_id
+                        ),
+                    ));
+                }
+            }


validate() only checks that a ParentRef::Branch { node_id } exists in all_ids, but all_ids includes archived nodes. Elsewhere (e.g., DaggerState::find_branch_by_id used by BranchGraph::lineage / branch_depth) parent lookups ignore archived nodes, so an active node can reference an archived parent and still pass validation while graph traversal treats the parent as missing. Consider tightening validation to reject parent refs that point to archived nodes (at least when the child is active), or consistently use find_any_branch_by_id for parent traversal if archived parents are meant to be allowed.

Addressed in the latest push.

This is intentionally allowed. An active node referencing an archived parent is a valid intermediate state that dgr sync is designed to repair. We previously had this check and removed it (see commit 00ad7b0) because it caused false validation failures during normal workflows. The comment at lines 92-93 documents this design decision, and the test validates_active_node_with_archived_parent_is_allowed (line 847) explicitly covers this case.

Copilot · 2026-03-31T02:23:43Z

src/core/store/types.rs

+        let state = DaggerState {
+            version: 1,
+            nodes: vec![parent, child],
+        };
+


The new validation tests hardcode version: 1. Using DAGGER_STATE_VERSION instead will keep these tests correct if/when the state version is bumped and avoids having to update multiple literals.

Addressed in the latest push.

Fixed — all tests now use the DAGGER_STATE_VERSION constant instead of hardcoded 1. The only literal version number remaining is 999 in the validates_version_mismatch test, which intentionally uses an invalid version to test error handling.

Copilot · 2026-03-31T02:23:43Z

src/core/store/types.rs

+        for node in &self.nodes {
+            if !seen_ids.insert(node.id) {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidData,
+                    format!("duplicate node id {}", node.id),
+                ));
+            }
+
+            if !node.archived && !seen_names.insert(&node.branch_name) {
+                return Err(io::Error::new(
+                    io::ErrorKind::InvalidData,
+                    format!("duplicate active branch name '{}'", node.branch_name),
+                ));
+            }
+
+            if let ParentRef::Branch { node_id } = &node.parent {
+                if !all_ids.contains(node_id) {
+                    return Err(io::Error::new(
+                        io::ErrorKind::InvalidData,
+                        format!(
+                            "branch '{}' references non-existent parent node {}",
+                            node.branch_name, node_id
+                        ),
+                    ));
+                }
+            }
+        }


validate() doesn’t currently guard against self-parenting or cyclic parent chains. A corrupted state.json containing a cycle (including a node whose parent is itself) can still pass this validation but will cause infinite loops in BranchGraph::lineage / branch_depth (they follow ParentRef::Branch without a visited set). Consider adding cycle detection (or at minimum a self-parent check) as part of validation so load-time validation better protects against hangs when the state file is corrupted.

Addressed in the latest push.

Added a self-parenting check in validate() (lines 76-81) — if a node's ParentRef::Branch { node_id } points to its own id, validation returns an error. The test validates_self_parent_reference (line 832) covers this case.

Full cycle detection (A→B→C→A) is already handled by PR #17 (merged), which detects cycles during graph traversal at restack time, so we only need the simple self-referential guard here in the state integrity check.

…DAGGER_STATE_VERSION constant Address Copilot review comments on PR oneirosoft#18: - Add self-referencing parent cycle detection in validate() - Add check that active nodes don't reference archived parents - Replace hardcoded version: 1 with DAGGER_STATE_VERSION in tests - Add tests for self-parent reference and active-node-with-archived-parent Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Active branches referencing archived parents is a valid intermediate state that the sync command is designed to repair. Rejecting this in validation prevents sync from loading the state to perform the repair, causing two integration tests to panic. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Copilot AI review requested due to automatic review settings March 31, 2026 02:18

Copilot started reviewing on behalf of don-petry March 31, 2026 02:18 View session

Copilot AI reviewed Mar 31, 2026

View reviewed changes

don-petry mentioned this pull request Mar 31, 2026

Correctness & data safety improvements #10

Open

DJ and others added 3 commits March 30, 2026 19:30

style: apply cargo fmt formatting

85e5f76

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: validate state integrity after loading state.json#18

fix: validate state integrity after loading state.json#18
don-petry wants to merge 4 commits intooneirosoft:mainfrom
don-petry:fix/state-validation

don-petry commented Mar 31, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

don-petry Mar 31, 2026

Uh oh!

don-petry Apr 5, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

don-petry Mar 31, 2026

Uh oh!

don-petry Apr 5, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

don-petry Mar 31, 2026

Uh oh!

don-petry Apr 5, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Conversation

don-petry commented Mar 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why?

Summary

Test plan

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull request overview

Reviewed changes

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

don-petry Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

don-petry Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

don-petry Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

don-petry Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Copilot AI Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

don-petry Mar 31, 2026

Choose a reason for hiding this comment

Uh oh!

don-petry Apr 5, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

don-petry commented Mar 31, 2026 •

edited

Loading