feat: add schema migration system for store files by don-petry · Pull Request #19 · oneirosoft/dagger

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

Why?

As dgr evolves, the on-disk store format (state.json, config.json, operation.json) will need to change. Without a migration system, users upgrading dgr would face cryptic deserialization errors or need to manually recreate their state. A sequential migration runner ensures smooth upgrades and clear error messages when the tool version is too old.

Summary

Adds a sequential migration runner (src/core/store/migrate.rs) that checks version fields on load and applies migrations in order for state.json, config.json, and operation.json
Each loader (load_state, load_config, load_operation) now deserializes to serde_json::Value first, runs the migration function, then deserializes to the typed struct
Future/unknown versions produce a clear "upgrade dgr" error; missing version fields produce a descriptive error
Version parsing uses checked try_into() conversion to prevent silent truncation of large values

Addresses item 6 from #10 (schema migration).

How it works

When a store file is loaded:

Raw JSON is parsed into an untyped serde_json::Value
The migrate_* function checks the version field
If the version is current, the value passes through unchanged
If older, sequential migrations are applied (none yet needed at v1)
If newer than supported, an error tells the user to upgrade dgr
The migrated value is then deserialized into the typed struct

Adding a future migration (e.g., v1 to v2) is a one-line addition:

if version < 2 { value = migrate_state_v1_to_v2(value)?; }

Test plan

Unit tests: current version passes through unchanged (state, config, operation)
Unit tests: future version returns "upgrade dgr" error
Unit tests: missing version field returns descriptive error
Unit tests: invalid version type and u64 overflow handled
CI: cargo test --locked passes

🤖 Generated with Claude Code

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) <noreply@anthropic.com>

Copilot

Pull request overview

Adds a JSON schema migration layer to the store-file loading path so existing on-disk state.json, config.json, and operation.json can be forward-migrated (or rejected when too new) before deserializing into typed Rust structs.

Changes:

Introduces src/core/store/migrate.rs with migrate_state, migrate_config, and migrate_operation functions plus unit tests.
Updates load_state, load_config, and load_operation to deserialize into serde_json::Value, run migrations, then deserialize into typed structs.
Registers the new migrate module in src/core/store/mod.rs.

Reviewed changes

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

Show a summary per file

File	Description
src/core/store/migrate.rs	Adds version checking and migration entry points for state/config/operation JSON values with unit tests.
src/core/store/state.rs	Routes state loading through the migration layer before typed deserialization.
src/core/store/config.rs	Routes config loading through the migration layer before typed deserialization.
src/core/store/operation.rs	Routes operation loading through the migration layer before typed deserialization.
src/core/store/mod.rs	Exposes the new `migrate` module within `store`.

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

Copilot · 2026-03-31T02:22:37Z

+    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;


version is parsed as u64 and then cast with as u32. If the JSON contains a very large integer (e.g. > u32::MAX), this will truncate and may incorrectly bypass the version > DAGGER_STATE_VERSION check. Also, the error message says "missing 'version' field" even when the field exists but is the wrong type (e.g. string/float/negative). Consider using a checked conversion (try_into) and returning an error like "missing or invalid 'version' field" (and/or a specific "version out of range" error).

Addressed in the latest push — version parsing now uses checked conversion via try_into() with specific error messages for missing, wrong-type, and overflow cases.

Copilot · 2026-03-31T02:22:38Z

+    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;


version is parsed as u64 and then cast with as u32, which can truncate large values and lead to incorrect version comparisons (e.g. a huge version might appear "older" after truncation). Also, the current error message treats an invalid type as "missing". Use a checked conversion (try_into) and a clearer "missing or invalid" message for the version field.

Addressed in the latest push — version parsing now uses checked conversion via try_into() with specific error messages for missing, wrong-type, and overflow cases.

Copilot · 2026-03-31T02:22:38Z

+    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;


version is converted via as u32 after reading as u64; this will silently truncate values > u32::MAX, which can cause the future-version guard to be bypassed. Also, a present-but-non-integer version currently yields a "missing" message. Prefer checked conversion and a message indicating the field is missing or invalid/out-of-range.

Addressed in the latest push — version parsing now uses checked conversion via try_into() with specific error messages for missing, wrong-type, and overflow cases.

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 oneirosoft#19. Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

don-petry · 2026-03-31T02:30:26Z

All three Copilot review comments addressed in the latest push (de1d418) — version parsing now uses checked conversion via try_into() with specific error messages for missing, wrong-type, and overflow cases. Also added tests for invalid version type and u64 overflow.

don-petry

Automated review — NEEDS HUMAN REVIEW

Risk: MEDIUM
Reviewed commit: de1d41837f5244ad9dfbe712f727229a829bc506
Council vote: security=LOW · correctness=MEDIUM · maintainability=MEDIUM

Summary

The PR correctly implements a sequential schema migration runner for state.json, config.json, and operation.json with proper version range checking and checked u64→u32 conversion to prevent silent truncation. The logic is sound and addresses issue #10 item 6 as claimed, and all Copilot review comments were addressed in the head commit. However, CI is in UNSTABLE state with an empty statusCheckRollup and the PR's own test plan leaves the cargo test --locked checkbox unchecked — correctness cannot be confirmed. The three migrate functions also share ~25 lines of near-identical boilerplate that should be refactored into a shared helper to ease future maintenance.

Linked issue analysis

PR correctly scopes to item 6 of issue #10 (schema migration). All acceptance criteria for that item appear addressed: version extraction is guarded against missing, wrong-type, and overflow cases with specific error messages. Items 1–5 (file locking, TOCTOU bug, cycle detection, state validation, --abort command) remain open and are explicitly called out as out-of-scope in the PR body.

Findings

Major

[major] (correctness · maintainability) mergeStateStatus=UNSTABLE — CI not confirmed green: statusCheckRollup is empty and the PR's own test plan has an unchecked checkbox for cargo test --locked. Cannot confirm the implementation compiles and tests pass against the head commit.
[major] src/core/store/migrate.rs (maintainability) — Code duplication: migrate_state, migrate_config, and migrate_operation share identical version-extraction and routing logic (~25 lines duplicated three times). A private helper (e.g. fn extract_version(value: &Value, context: &str) -> io::Result<u32>) plus a generic migration-runner would eliminate the duplication and make future migration additions a single-site change instead of three.

Minor

[minor] src/core/store/migrate.rs (correctness · maintainability) — Test coverage gap: invalid-version-type and u64-overflow tests exist only for migrate_state. The identical parsing logic in migrate_config and migrate_operation has no equivalent tests, leaving those error paths uncovered.
[minor] src/core/store/migrate.rs (maintainability) — Inconsistent documentation: migrate_state includes a placeholder comment showing how to add a future migration step, but the same guidance is absent from migrate_config and migrate_operation, leaving future authors without an example when extending those paths.

Info

[info] (security) — No CI security scanners configured: statusCheckRollup is empty, meaning no CodeQL, Semgrep, or equivalent automated security gates are in place. Consider adding security scanning to the CI pipeline.
[info] src/core/store/migrate.rs (security) — Local file trust boundary is appropriate: migration functions deserialize tool-internal JSON files via serde_json::Value before re-deserializing to typed structs. These files are not externally supplied, so no elevated trust concern.
[info] (correctness) — Issue scope: PR correctly limits itself to item 6 of issue #10. Items 1–5 remain open per the PR body.
[info] src/core/store/migrate.rs (correctness) — Future migration note: when version < DAGGER_STATE_VERSION the code bumps the version field but applies no structural transformation. This is safe today since v1 is the initial shipped version, but future contributors must add actual field transforms before bumping the version constants.

CI status

mergeStateStatus is UNSTABLE; statusCheckRollup is empty — no CI checks have reported results for the head commit.

Reviewed automatically by the don-petry PR-review council (security: opus 4.6 · correctness: sonnet 4.6 · maintainability: sonnet 4.6 · synthesis: sonnet 4.6). The marker on line 1 lets the agent detect new commits and re-review. Reply with @don-petry if you need a human.

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

Copilot started reviewing on behalf of don-petry March 31, 2026 02:20 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

don-petry commented Apr 10, 2026

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: add schema migration system for store files#19

feat: add schema migration system for store files#19
don-petry wants to merge 2 commits intooneirosoft:mainfrom
don-petry:feat/schema-migration

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!

Copilot AI Mar 31, 2026

Uh oh!

don-petry Mar 31, 2026

Uh oh!

Copilot AI Mar 31, 2026

Uh oh!

don-petry Mar 31, 2026

Uh oh!

don-petry commented Mar 31, 2026

Uh oh!

don-petry left a comment

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

How it works

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!

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!

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 commented Mar 31, 2026

Uh oh!

don-petry left a comment

Choose a reason for hiding this comment

Automated review — NEEDS HUMAN REVIEW

Summary

Linked issue analysis

Findings

Major

Minor

Info

CI status

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

don-petry commented Mar 31, 2026 •

edited

Loading