-
Notifications
You must be signed in to change notification settings - Fork 1
fix: validate state integrity after loading state.json #18
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| use std::collections::HashSet; | ||
| use std::io; | ||
| use std::time::{SystemTime, UNIX_EPOCH}; | ||
|
|
||
|
|
@@ -41,6 +42,61 @@ 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<Uuid> = 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: 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, parent_id | ||
| ), | ||
| )); | ||
| } | ||
| // 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. | ||
| } | ||
| } | ||
|
|
||
| Ok(()) | ||
| } | ||
|
|
||
| pub fn find_branch_by_name(&self, branch_name: &str) -> Option<&BranchNode> { | ||
| self.nodes | ||
| .iter() | ||
|
|
@@ -422,10 +478,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, | ||
| 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; | ||
|
|
@@ -666,4 +723,139 @@ 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: DAGGER_STATE_VERSION, | ||
| nodes: vec![parent, child], | ||
| }; | ||
|
|
||
|
Comment on lines
+747
to
+751
|
||
| 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: DAGGER_STATE_VERSION, | ||
| 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: DAGGER_STATE_VERSION, | ||
| 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: DAGGER_STATE_VERSION, | ||
| 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: 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_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 }); | ||
|
|
||
| let state = DaggerState { | ||
| version: DAGGER_STATE_VERSION, | ||
| nodes: vec![parent, child], | ||
| }; | ||
|
|
||
| // 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()); | ||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
validate()doesn’t currently guard against self-parenting or cyclic parent chains. A corruptedstate.jsoncontaining a cycle (including a node whose parent is itself) can still pass this validation but will cause infinite loops inBranchGraph::lineage/branch_depth(they followParentRef::Branchwithout 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.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in the latest push.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added a self-parenting check in
validate()(lines 76-81) — if a node'sParentRef::Branch { node_id }points to its ownid, validation returns an error. The testvalidates_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.