Fix #329: Validate reserved_resources, milestone, and build-id fields in save schema

[itsmiso-ai]

What

Added validation for reserved_resources, next_build_id, current_milestone_id, completed_milestone_ids, and migration_log fields in the save schema to prevent corrupt saves from loading. …

Fixes #329

Opened by foreman on review GO (workload wl-misospace-windowstead-329).

[its-saffron]

AI Automated Review

Full PR review.

Analysis engine: dsv4f@https://litellm.jory.dev/v1 (openai) — primary route

Recommendation

Approve. The PR directly implements issue PR 329's ask and acceptance criteria, adds regression coverage for every requested field, and all CI checks are green. No must_check items were specified by the PR classifier.

Change-by-change findings

• scripts/game_state.gd: Adds presence-guarded validation in validate_save_schema() for reserved_resources, next_build_id, current_milestone_id, completed_milestone_ids, and migration_log. Each check rejects the malformed shapes called out in issue PR 329 (non-Dictionary reservations, non-numeric/negative reservation values, non-String milestone id, non-Array milestone list/migration log, non-numeric/negative next_build_id). Optional-field semantics are preserved, and descriptive failure reasons are returned. I found no correctness, compatibility, or security issues.
• tests/test_runner.gd: Adds five focused regression tests and wires them into run_tests(). The reserved_resources test also covers the end-to-end corrupt-save path (save_game → load_game() returns empty), satisfying the issue's regression requirement. All added assert_eq/assert_true/assert_empty calls include the required name argument, so the repository's PR 321 parse-error trap does not apply.

Sources

• PR PR 336: upstream misospace/windowstead PR 336
• Linked issue PR 329: upstream misospace/windowstead issue 329
• CI status for 2dc3c47: all checks success (export validation macOS/Linux/Web/Windows, script test suite, headless smoke test, macOS validation)

Standards Compliance

• AGENTS.md testing trap: all added assertions pass the required-name check for assert_eq; the suite ran under CI headless with no reported parse errors.
• Repository conventions: no manifest changes, no release-process impact; the change is scoped to save-schema validation and test coverage exactly as the linked issue expects.
• No deviation from documented conventions found.

Linked Issue Fit

Issue PR 329's acceptance criteria are fully met:

• Rejects non-Dictionary reserved_resources, non-numeric reservation values, and negative reservation values.
• Rejects non-numeric and negative next_build_id.
• Rejects non-String current_milestone_id.
• Rejects non-Array completed_milestone_ids and migration_log, plus non-String/empty entries in the milestone id list.
• Corrupt-save regression test asserts load_game() returns {} for the malformed reserved_resources fixture.
• Existing test suites stay green per CI.
• Expected files named in the issue (scripts/game_state.gd, tests/test_runner.gd) are exactly the files changed.

Tool Harness Findings

Tool harness planning was still pending at review time, so no harness-produced findings were available for this review.

Unknowns or Needs Verification

No blocking unknowns. The review did not locally re-run the headless suite, but the CI Script test suite and all validation jobs are green on the PR head commit. If tool-harness planning completes later, its output can be checked against this review; nothing in the current corpus indicates a gap.