Harden save migration and schema compatibility checks#128
Merged
Conversation
- Add schema validation for required keys, tile counts, and shape - Explicitly reject unknown/future save versions instead of returning raw data - Reject version 0 or missing save_version as unsupported - Add regression tests for v1->v2 migration, invalid old, future, and malformed saves
Contributor
|
✅ Automated recommendation: APPROVE Analysis engine: MiniMax-M2.7@https://litellm.jory.dev/v1 (anthropic) PR #128 Review: Harden save migration and schema compatibility checksRecommendationApprove. This PR fully satisfies the linked issue #124 acceptance criteria. The implementation is internally consistent, follows repository conventions, and includes comprehensive regression tests. Change-by-Change Findings
|
| Requirement | Status |
|---|---|
| Persistence changes covered by tests | ✅ 182 lines of regression tests added |
GDScript idioms (const, var, func) |
✅ |
Test conventions (_assert_eq, _assert_empty, _assert_not_empty) |
✅ |
| Descriptive test messages | ✅ |
| Schema validation is permissive (only validates present fields) | ✅ |
Linked Issue Fit
Issue #124 Acceptance Criteria
| Criterion | Fulfilled? | Evidence |
|---|---|---|
| Unknown future or invalid save versions fail explicitly | ✅ | migrate_save() now rejects save_version > SAVE_VERSION, save_version == 0, and save_version < 1 with explicit error messages |
| Compatibility checks cover tile counts and required schema shape | ✅ | validate_save_schema() validates tile array size against known grid sizes and verifies each tile has kind, amount, resource, build_kind |
| Regression tests cover v1→v2, invalid old, future, and malformed saves | ✅ | test_save_migration_hardening() covers all five categories with 8 test cases |
Evidence Provider Findings
No evidence providers configured for this PR. No output to analyze.
Tool Harness Findings
Tool harness ran in plan_execute_once mode. No tool requests were executed (0 planned, 0 executed). No findings to report.
Unknowns / Needs Verification
- None. All acceptance criteria are clearly addressed. The diff is complete, the implementation matches the PR description, and test coverage is thorough.
…124) - Treat missing save_version as current version (backward compatible) - Make schema validation field-presence based instead of required-keys - Allow empty tiles arrays (valid for fresh games) - Fix malformed_resources test to use non-dict resources type error - Update migration_no_version test to expect v2 default behavior
- Add 150, 160, 240 to expected tile grid sizes - Update e2e test to expect v0 rejection instead of preservation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #124