From 7b91e3e94e9c5e6dc3ad5c491aab7b22faa9cddd Mon Sep 17 00:00:00 2001 From: DJ Date: Mon, 30 Mar 2026 19:19:19 -0700 Subject: [PATCH 1/2] feat: add schema migration system for store files Add a sequential migration runner that checks version fields on load and applies migrations in order for state.json, config.json, and operation.json. This ensures future version bumps have a clean upgrade path instead of failing to deserialize old files. - New migrate.rs module with migrate_state, migrate_config, and migrate_operation functions - Each loader now deserializes to serde_json::Value first, runs migration, then deserializes to the typed struct - Future versions produce a clear "upgrade dgr" error - Missing version fields produce a descriptive error - Comprehensive test coverage for all migration paths Co-Authored-By: Claude Opus 4.6 (1M context) --- src/core/store/config.rs | 6 +- src/core/store/migrate.rs | 277 ++++++++++++++++++++++++++++++++++++ src/core/store/mod.rs | 1 + src/core/store/operation.rs | 6 +- src/core/store/state.rs | 6 +- 5 files changed, 293 insertions(+), 3 deletions(-) create mode 100644 src/core/store/migrate.rs diff --git a/src/core/store/config.rs b/src/core/store/config.rs index 2c91057..354a2cb 100644 --- a/src/core/store/config.rs +++ b/src/core/store/config.rs @@ -10,7 +10,11 @@ pub fn load_config(paths: &DaggerPaths) -> io::Result> { } let bytes = fs::read(&paths.config_file)?; - let config = serde_json::from_slice(&bytes) + let value: serde_json::Value = serde_json::from_slice(&bytes) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; + + let migrated = super::migrate::migrate_config(value)?; + let config: DaggerConfig = serde_json::from_value(migrated) .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; Ok(Some(config)) diff --git a/src/core/store/migrate.rs b/src/core/store/migrate.rs new file mode 100644 index 0000000..4f1f5a2 --- /dev/null +++ b/src/core/store/migrate.rs @@ -0,0 +1,277 @@ +use std::io; + +use serde_json::Value; + +use super::types::{DAGGER_CONFIG_VERSION, DAGGER_OPERATION_VERSION, DAGGER_STATE_VERSION}; + +/// Migrate a state JSON value from its current version to DAGGER_STATE_VERSION. +/// Returns the migrated value, or the original if already current. +pub fn migrate_state(mut value: Value) -> io::Result { + let version = value + .get("version") + .and_then(|v| v.as_u64()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "state file missing 'version' field", + ) + })? as u32; + + if version > DAGGER_STATE_VERSION { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "state version {} is newer than supported version {}; upgrade dgr", + version, DAGGER_STATE_VERSION + ), + )); + } + + if version == DAGGER_STATE_VERSION { + return Ok(value); + } + + // Apply migrations sequentially: 0→1, 1→2, etc. + // Currently no migrations needed since we're at version 1. + // Future migrations would be added here: + // if version < 2 { value = migrate_state_v1_to_v2(value)?; } + + // Update the version field after all migrations + value["version"] = serde_json::json!(DAGGER_STATE_VERSION); + + Ok(value) +} + +/// Migrate a config JSON value from its current version to DAGGER_CONFIG_VERSION. +/// Returns the migrated value, or the original if already current. +pub fn migrate_config(mut value: Value) -> io::Result { + let version = value + .get("version") + .and_then(|v| v.as_u64()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "config file missing 'version' field", + ) + })? as u32; + + if version > DAGGER_CONFIG_VERSION { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "config version {} is newer than supported version {}; upgrade dgr", + version, DAGGER_CONFIG_VERSION + ), + )); + } + + if version == DAGGER_CONFIG_VERSION { + return Ok(value); + } + + value["version"] = serde_json::json!(DAGGER_CONFIG_VERSION); + + Ok(value) +} + +/// Migrate an operation JSON value from its current version to DAGGER_OPERATION_VERSION. +/// Returns the migrated value, or the original if already current. +pub fn migrate_operation(mut value: Value) -> io::Result { + let version = value + .get("version") + .and_then(|v| v.as_u64()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + "operation file missing 'version' field", + ) + })? as u32; + + if version > DAGGER_OPERATION_VERSION { + return Err(io::Error::new( + io::ErrorKind::InvalidData, + format!( + "operation version {} is newer than supported version {}; upgrade dgr", + version, DAGGER_OPERATION_VERSION + ), + )); + } + + if version == DAGGER_OPERATION_VERSION { + return Ok(value); + } + + value["version"] = serde_json::json!(DAGGER_OPERATION_VERSION); + + Ok(value) +} + +#[cfg(test)] +mod tests { + use serde_json::json; + + use super::*; + + #[test] + fn state_current_version_passes_through_unchanged() { + let input = json!({ + "version": DAGGER_STATE_VERSION, + "nodes": [ + { + "id": "00000000-0000-0000-0000-000000000000", + "branch_name": "feat/api", + "parent": {"kind": "trunk"}, + "base_ref": "main", + "fork_point_oid": "abc123", + "head_oid_at_creation": "abc123", + "created_at_unix_secs": 1, + "archived": false + } + ] + }); + + let result = migrate_state(input.clone()).unwrap(); + + assert_eq!(result, input); + } + + #[test] + fn state_future_version_returns_upgrade_error() { + let input = json!({ + "version": DAGGER_STATE_VERSION + 1, + "nodes": [] + }); + + let err = migrate_state(input).unwrap_err(); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("upgrade dgr")); + } + + #[test] + fn state_missing_version_returns_error() { + let input = json!({ + "nodes": [] + }); + + let err = migrate_state(input).unwrap_err(); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("missing 'version' field")); + } + + #[test] + fn state_migration_preserves_all_fields() { + // Simulate a value at version 0 (older than current) that needs migration. + // Since DAGGER_STATE_VERSION is 1 and we have no real v0→v1 migration, + // this tests the framework: version gets bumped, other fields are preserved. + let input = json!({ + "version": 0, + "nodes": [ + { + "id": "00000000-0000-0000-0000-000000000001", + "branch_name": "feat/login", + "parent": {"kind": "trunk"}, + "base_ref": "main", + "fork_point_oid": "def456", + "head_oid_at_creation": "def456", + "created_at_unix_secs": 100, + "archived": false + } + ] + }); + + let result = migrate_state(input).unwrap(); + + assert_eq!(result["version"], json!(DAGGER_STATE_VERSION)); + assert_eq!(result["nodes"][0]["branch_name"], json!("feat/login")); + assert_eq!(result["nodes"][0]["fork_point_oid"], json!("def456")); + } + + #[test] + fn config_current_version_passes_through_unchanged() { + let input = json!({ + "version": DAGGER_CONFIG_VERSION, + "trunk_branch": "main" + }); + + let result = migrate_config(input.clone()).unwrap(); + + assert_eq!(result, input); + } + + #[test] + fn config_future_version_returns_upgrade_error() { + let input = json!({ + "version": DAGGER_CONFIG_VERSION + 1, + "trunk_branch": "main" + }); + + let err = migrate_config(input).unwrap_err(); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("upgrade dgr")); + } + + #[test] + fn config_missing_version_returns_error() { + let input = json!({ + "trunk_branch": "main" + }); + + let err = migrate_config(input).unwrap_err(); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("missing 'version' field")); + } + + #[test] + fn operation_current_version_passes_through_unchanged() { + let input = json!({ + "version": DAGGER_OPERATION_VERSION, + "origin": {"type": "commit", "current_branch": "feat/api", "summary_line": null, "recent_commits": []}, + "restack": { + "active_action": { + "node_id": "00000000-0000-0000-0000-000000000000", + "branch_name": "feat/api", + "old_upstream_branch_name": "main", + "old_upstream_oid": "abc", + "new_base": {"branch_name": "main", "source": "local"}, + "new_parent": null + }, + "remaining_actions": [], + "completed_branches": [] + } + }); + + let result = migrate_operation(input.clone()).unwrap(); + + assert_eq!(result, input); + } + + #[test] + fn operation_future_version_returns_upgrade_error() { + let input = json!({ + "version": DAGGER_OPERATION_VERSION + 1, + "origin": {"type": "commit", "current_branch": "feat/api", "summary_line": null, "recent_commits": []}, + "restack": {"active_action": {}, "remaining_actions": [], "completed_branches": []} + }); + + let err = migrate_operation(input).unwrap_err(); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("upgrade dgr")); + } + + #[test] + fn operation_missing_version_returns_error() { + let input = json!({ + "origin": {"type": "commit"} + }); + + let err = migrate_operation(input).unwrap_err(); + + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("missing 'version' field")); + } +} diff --git a/src/core/store/mod.rs b/src/core/store/mod.rs index 3a40e50..7a60667 100644 --- a/src/core/store/mod.rs +++ b/src/core/store/mod.rs @@ -2,6 +2,7 @@ pub(crate) mod bootstrap; pub(crate) mod config; pub(crate) mod events; pub(crate) mod fs; +pub(crate) mod migrate; pub(crate) mod mutations; pub(crate) mod operation; pub(crate) mod session; diff --git a/src/core/store/operation.rs b/src/core/store/operation.rs index 2a9034a..619702b 100644 --- a/src/core/store/operation.rs +++ b/src/core/store/operation.rs @@ -10,7 +10,11 @@ pub fn load_operation(paths: &DaggerPaths) -> io::Result io::Result { } let bytes = fs::read(&paths.state_file)?; - let state = serde_json::from_slice(&bytes) + let value: serde_json::Value = serde_json::from_slice(&bytes) + .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; + + let migrated = super::migrate::migrate_state(value)?; + let state: DaggerState = serde_json::from_value(migrated) .map_err(|err| io::Error::new(io::ErrorKind::InvalidData, err))?; Ok(state) From de1d41837f5244ad9dfbe712f727229a829bc506 Mon Sep 17 00:00:00 2001 From: DJ Date: Mon, 30 Mar 2026 19:29:56 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20use=20checked=20version=20parsing=20?= =?UTF-8?q?to=20prevent=20silent=20u64=E2=86=92u32=20truncation?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Split version field parsing into two steps: first check for field presence, then validate the value with try_into() instead of bare `as u32` cast. This catches wrong-type and overflow cases with specific error messages. Addresses Copilot review comments on PR #19. Co-Authored-By: Claude Opus 4.6 (1M context) --- src/core/store/migrate.rs | 69 ++++++++++++++++++++++++++++++++++----- 1 file changed, 60 insertions(+), 9 deletions(-) diff --git a/src/core/store/migrate.rs b/src/core/store/migrate.rs index 4f1f5a2..f486cb4 100644 --- a/src/core/store/migrate.rs +++ b/src/core/store/migrate.rs @@ -7,15 +7,24 @@ use super::types::{DAGGER_CONFIG_VERSION, DAGGER_OPERATION_VERSION, DAGGER_STATE /// Migrate a state JSON value from its current version to DAGGER_STATE_VERSION. /// Returns the migrated value, or the original if already current. pub fn migrate_state(mut value: Value) -> io::Result { - let version = value + let raw_version = value .get("version") - .and_then(|v| v.as_u64()) .ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, "state file missing 'version' field", ) - })? as u32; + })?; + + let version: u32 = raw_version + .as_u64() + .and_then(|v| v.try_into().ok()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("state file has invalid 'version' field: {}", raw_version), + ) + })?; if version > DAGGER_STATE_VERSION { return Err(io::Error::new( @@ -45,15 +54,24 @@ pub fn migrate_state(mut value: Value) -> io::Result { /// Migrate a config JSON value from its current version to DAGGER_CONFIG_VERSION. /// Returns the migrated value, or the original if already current. pub fn migrate_config(mut value: Value) -> io::Result { - let version = value + let raw_version = value .get("version") - .and_then(|v| v.as_u64()) .ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, "config file missing 'version' field", ) - })? as u32; + })?; + + let version: u32 = raw_version + .as_u64() + .and_then(|v| v.try_into().ok()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("config file has invalid 'version' field: {}", raw_version), + ) + })?; if version > DAGGER_CONFIG_VERSION { return Err(io::Error::new( @@ -77,15 +95,24 @@ pub fn migrate_config(mut value: Value) -> io::Result { /// Migrate an operation JSON value from its current version to DAGGER_OPERATION_VERSION. /// Returns the migrated value, or the original if already current. pub fn migrate_operation(mut value: Value) -> io::Result { - let version = value + let raw_version = value .get("version") - .and_then(|v| v.as_u64()) .ok_or_else(|| { io::Error::new( io::ErrorKind::InvalidData, "operation file missing 'version' field", ) - })? as u32; + })?; + + let version: u32 = raw_version + .as_u64() + .and_then(|v| v.try_into().ok()) + .ok_or_else(|| { + io::Error::new( + io::ErrorKind::InvalidData, + format!("operation file has invalid 'version' field: {}", raw_version), + ) + })?; if version > DAGGER_OPERATION_VERSION { return Err(io::Error::new( @@ -274,4 +301,28 @@ mod tests { assert_eq!(err.kind(), io::ErrorKind::InvalidData); assert!(err.to_string().contains("missing 'version' field")); } + + #[test] + fn state_invalid_version_type_returns_error() { + let input = json!({ + "version": "not_a_number", + "nodes": [] + }); + + let err = migrate_state(input).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("invalid 'version' field")); + } + + #[test] + fn state_version_overflow_returns_error() { + let input = json!({ + "version": u64::MAX, + "nodes": [] + }); + + let err = migrate_state(input).unwrap_err(); + assert_eq!(err.kind(), io::ErrorKind::InvalidData); + assert!(err.to_string().contains("invalid 'version' field")); + } }