From a0e0c1040645e126a569949edfdf93bf3f785e6d Mon Sep 17 00:00:00 2001 From: DJ Date: Mon, 30 Mar 2026 19:18:06 -0700 Subject: [PATCH 1/4] fix: validate state integrity after loading state.json 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: #10 Co-Authored-By: Claude Opus 4.6 (1M context) --- src/core/store/state.rs | 4 +- src/core/store/types.rs | 146 ++++++++++++++++++++++++++++++++++++++++ 2 files changed, 149 insertions(+), 1 deletion(-) diff --git a/src/core/store/state.rs b/src/core/store/state.rs index cec5259..8d7d853 100644 --- a/src/core/store/state.rs +++ b/src/core/store/state.rs @@ -10,9 +10,11 @@ pub fn load_state(paths: &DaggerPaths) -> io::Result { } let bytes = fs::read(&paths.state_file)?; - let state = serde_json::from_slice(&bytes) + let state: DaggerState = serde_json::from_slice(&bytes) .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; + state.validate()?; + Ok(state) } diff --git a/src/core/store/types.rs b/src/core/store/types.rs index 928964e..219cc97 100644 --- a/src/core/store/types.rs +++ b/src/core/store/types.rs @@ -1,3 +1,4 @@ +use std::collections::HashSet; use std::io; use std::time::{SystemTime, UNIX_EPOCH}; @@ -41,6 +42,52 @@ impl Default for DaggerState { } impl DaggerState { + pub fn validate(&self) -> io::Result<()> { + if self.version != DAGGER_STATE_VERSION { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "unsupported state version {} (expected {})", + self.version, DAGGER_STATE_VERSION + ), + )); + } + + let mut seen_ids = HashSet::new(); + let mut seen_names = HashSet::new(); + let all_ids: HashSet = self.nodes.iter().map(|n| n.id).collect(); + + 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 + ), + )); + } + } + } + + Ok(()) + } + pub fn find_branch_by_name(&self, branch_name: &str) -> Option<&BranchNode> { self.nodes .iter() @@ -666,4 +713,103 @@ mod tests { assert!(serialized.contains("\"type\":\"branch_archived\"")); assert!(serialized.contains("\"kind\":\"deleted_locally\"")); } + + fn make_node(name: &str, parent: ParentRef) -> BranchNode { + BranchNode { + id: Uuid::new_v4(), + branch_name: name.into(), + parent, + base_ref: "main".into(), + fork_point_oid: "abc123".into(), + head_oid_at_creation: "abc123".into(), + created_at_unix_secs: 1, + divergence_state: BranchDivergenceState::Unknown, + pull_request: None, + archived: false, + } + } + + #[test] + fn validates_valid_state() { + let parent = make_node("feature/parent", ParentRef::Trunk); + let child = make_node("feature/child", ParentRef::Branch { node_id: parent.id }); + + let state = DaggerState { + version: 1, + nodes: vec![parent, child], + }; + + assert!(state.validate().is_ok()); + } + + #[test] + fn validates_version_mismatch() { + let state = DaggerState { + version: 999, + nodes: Vec::new(), + }; + + let err = state.validate().unwrap_err(); + assert_eq!(err.kind(), std::io::ErrorKind::InvalidData); + assert!(err.to_string().contains("unsupported state version 999")); + } + + #[test] + fn validates_duplicate_node_ids() { + let mut a = make_node("feature/a", ParentRef::Trunk); + let mut b = make_node("feature/b", ParentRef::Trunk); + let shared_id = Uuid::new_v4(); + a.id = shared_id; + b.id = shared_id; + + let state = DaggerState { + version: 1, + nodes: vec![a, b], + }; + + let err = state.validate().unwrap_err(); + assert!(err.to_string().contains("duplicate node id")); + } + + #[test] + fn validates_duplicate_active_branch_names() { + let a = make_node("feature/dup", ParentRef::Trunk); + let b = make_node("feature/dup", ParentRef::Trunk); + + let state = DaggerState { + version: 1, + nodes: vec![a, b], + }; + + let err = state.validate().unwrap_err(); + assert!(err.to_string().contains("duplicate active branch name")); + } + + #[test] + fn validates_dangling_parent_reference() { + let dangling_id = Uuid::new_v4(); + let node = make_node("feature/orphan", ParentRef::Branch { node_id: dangling_id }); + + let state = DaggerState { + version: 1, + nodes: vec![node], + }; + + let err = state.validate().unwrap_err(); + assert!(err.to_string().contains("non-existent parent node")); + } + + #[test] + fn validates_archived_duplicate_names_allowed() { + let mut archived = make_node("feature/dup", ParentRef::Trunk); + archived.archived = true; + let active = make_node("feature/dup", ParentRef::Trunk); + + let state = DaggerState { + version: 1, + nodes: vec![archived, active], + }; + + assert!(state.validate().is_ok()); + } } From c198a36268e7a58c11539dc4c47c9f31a3be2ccd Mon Sep 17 00:00:00 2001 From: DJ Date: Mon, 30 Mar 2026 19:30:14 -0700 Subject: [PATCH 2/4] fix: add self-parent cycle detection, archived parent check, and use DAGGER_STATE_VERSION constant Address Copilot review comments on PR #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) --- src/core/store/types.rs | 71 +++++++++++++++++++++++++++++++++++------ 1 file changed, 62 insertions(+), 9 deletions(-) diff --git a/src/core/store/types.rs b/src/core/store/types.rs index 219cc97..b37ea01 100644 --- a/src/core/store/types.rs +++ b/src/core/store/types.rs @@ -72,16 +72,39 @@ impl DaggerState { )); } - if let ParentRef::Branch { node_id } = &node.parent { - if !all_ids.contains(node_id) { + if let ParentRef::Branch { node_id: parent_id } = &node.parent { + if *parent_id == node.id { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "branch '{}' references itself as parent", + node.branch_name + ), + )); + } + if !all_ids.contains(parent_id) { return Err(io::Error::new( io::ErrorKind::InvalidData, format!( "branch '{}' references non-existent parent node {}", - node.branch_name, node_id + node.branch_name, parent_id ), )); } + // Warn if an active node references an archived parent + if !node.archived { + if let Some(parent_node) = self.nodes.iter().find(|n| n.id == *parent_id) { + if parent_node.archived { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "active branch '{}' references archived parent '{}'", + node.branch_name, parent_node.branch_name + ), + )); + } + } + } } } @@ -472,7 +495,7 @@ mod tests { BranchNode, BranchPullRequestTrackedEvent, BranchPullRequestTrackedSource, DaggerConfig, DaggerEvent, DaggerState, ParentRef, PendingCommitOperation, PendingOperationKind, PendingOperationState, PendingOrphanOperation, PendingReparentOperation, - PendingSyncOperation, PendingSyncPhase, TrackedPullRequest, + PendingSyncOperation, PendingSyncPhase, TrackedPullRequest, DAGGER_STATE_VERSION, }; use crate::core::restack::{RestackAction, RestackBaseTarget}; use uuid::Uuid; @@ -735,7 +758,7 @@ mod tests { let child = make_node("feature/child", ParentRef::Branch { node_id: parent.id }); let state = DaggerState { - version: 1, + version: DAGGER_STATE_VERSION, nodes: vec![parent, child], }; @@ -763,7 +786,7 @@ mod tests { b.id = shared_id; let state = DaggerState { - version: 1, + version: DAGGER_STATE_VERSION, nodes: vec![a, b], }; @@ -777,7 +800,7 @@ mod tests { let b = make_node("feature/dup", ParentRef::Trunk); let state = DaggerState { - version: 1, + version: DAGGER_STATE_VERSION, nodes: vec![a, b], }; @@ -791,7 +814,7 @@ mod tests { let node = make_node("feature/orphan", ParentRef::Branch { node_id: dangling_id }); let state = DaggerState { - version: 1, + version: DAGGER_STATE_VERSION, nodes: vec![node], }; @@ -806,10 +829,40 @@ mod tests { let active = make_node("feature/dup", ParentRef::Trunk); let state = DaggerState { - version: 1, + version: DAGGER_STATE_VERSION, nodes: vec![archived, active], }; assert!(state.validate().is_ok()); } + + #[test] + fn validates_self_parent_reference() { + let mut node = make_node("feature/self-ref", ParentRef::Trunk); + let self_id = node.id; + node.parent = ParentRef::Branch { node_id: self_id }; + + let state = DaggerState { + version: DAGGER_STATE_VERSION, + nodes: vec![node], + }; + + let err = state.validate().unwrap_err(); + assert!(err.to_string().contains("references itself as parent")); + } + + #[test] + fn validates_active_node_with_archived_parent() { + let mut parent = make_node("feature/parent", ParentRef::Trunk); + parent.archived = true; + let child = make_node("feature/child", ParentRef::Branch { node_id: parent.id }); + + let state = DaggerState { + version: DAGGER_STATE_VERSION, + nodes: vec![parent, child], + }; + + let err = state.validate().unwrap_err(); + assert!(err.to_string().contains("archived parent")); + } } From 85e5f769df3d81d873fae6729466509122d0a2b5 Mon Sep 17 00:00:00 2001 From: DJ Date: Thu, 2 Apr 2026 04:55:49 -0700 Subject: [PATCH 3/4] style: apply cargo fmt formatting Co-Authored-By: Claude Opus 4.6 (1M context) --- src/core/store/types.rs | 21 ++++++++++++--------- 1 file changed, 12 insertions(+), 9 deletions(-) diff --git a/src/core/store/types.rs b/src/core/store/types.rs index b37ea01..9264401 100644 --- a/src/core/store/types.rs +++ b/src/core/store/types.rs @@ -76,10 +76,7 @@ impl DaggerState { if *parent_id == node.id { return Err(io::Error::new( io::ErrorKind::InvalidData, - format!( - "branch '{}' references itself as parent", - node.branch_name - ), + format!("branch '{}' references itself as parent", node.branch_name), )); } if !all_ids.contains(parent_id) { @@ -492,10 +489,11 @@ pub fn now_unix_timestamp_secs() -> u64 { mod tests { use super::{ BranchAdoptedEvent, BranchArchiveReason, BranchArchivedEvent, BranchDivergenceState, - BranchNode, BranchPullRequestTrackedEvent, BranchPullRequestTrackedSource, DaggerConfig, - DaggerEvent, DaggerState, ParentRef, PendingCommitOperation, PendingOperationKind, - PendingOperationState, PendingOrphanOperation, PendingReparentOperation, - PendingSyncOperation, PendingSyncPhase, TrackedPullRequest, DAGGER_STATE_VERSION, + BranchNode, BranchPullRequestTrackedEvent, BranchPullRequestTrackedSource, + DAGGER_STATE_VERSION, DaggerConfig, DaggerEvent, DaggerState, ParentRef, + PendingCommitOperation, PendingOperationKind, PendingOperationState, + PendingOrphanOperation, PendingReparentOperation, PendingSyncOperation, PendingSyncPhase, + TrackedPullRequest, }; use crate::core::restack::{RestackAction, RestackBaseTarget}; use uuid::Uuid; @@ -811,7 +809,12 @@ mod tests { #[test] fn validates_dangling_parent_reference() { let dangling_id = Uuid::new_v4(); - let node = make_node("feature/orphan", ParentRef::Branch { node_id: dangling_id }); + let node = make_node( + "feature/orphan", + ParentRef::Branch { + node_id: dangling_id, + }, + ); let state = DaggerState { version: DAGGER_STATE_VERSION, From 00ad7b0e7e11b990c1623609bae1e3a1ae817e96 Mon Sep 17 00:00:00 2001 From: DJ Date: Thu, 2 Apr 2026 11:03:14 -0700 Subject: [PATCH 4/4] fix: allow archived parent references in state validation 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) --- src/core/store/types.rs | 24 +++++++----------------- 1 file changed, 7 insertions(+), 17 deletions(-) diff --git a/src/core/store/types.rs b/src/core/store/types.rs index 9264401..b61812e 100644 --- a/src/core/store/types.rs +++ b/src/core/store/types.rs @@ -88,20 +88,9 @@ impl DaggerState { ), )); } - // Warn if an active node references an archived parent - if !node.archived { - if let Some(parent_node) = self.nodes.iter().find(|n| n.id == *parent_id) { - if parent_node.archived { - return Err(io::Error::new( - io::ErrorKind::InvalidData, - format!( - "active branch '{}' references archived parent '{}'", - node.branch_name, parent_node.branch_name - ), - )); - } - } - } + // Note: an active node referencing an archived parent is a valid + // intermediate state that the sync command is designed to repair. + // We intentionally do not reject it here. } } @@ -855,7 +844,7 @@ mod tests { } #[test] - fn validates_active_node_with_archived_parent() { + fn validates_active_node_with_archived_parent_is_allowed() { let mut parent = make_node("feature/parent", ParentRef::Trunk); parent.archived = true; let child = make_node("feature/child", ParentRef::Branch { node_id: parent.id }); @@ -865,7 +854,8 @@ mod tests { nodes: vec![parent, child], }; - let err = state.validate().unwrap_err(); - assert!(err.to_string().contains("archived parent")); + // An active node referencing an archived parent is a valid intermediate + // state that sync is designed to repair, so validation must accept it. + assert!(state.validate().is_ok()); } }