From 07044f439cb1f00ffab50eb87c7125d8a0d0292c Mon Sep 17 00:00:00 2001 From: funman300 Date: Fri, 10 Jul 2026 10:21:01 -0700 Subject: [PATCH] fix(core): save files store the deal via upstream serializers (schema v6) GameState's save serde had the same latent flaw PR #170 removed from replays: v5 persisted seed + saved_moves and re-dealt the board from the seed on load, so any RNG or upstream upgrade that shifts the seed->deal mapping would invalidate every in-progress save (graceful error, but the player loses their game). v6 persists the existing SessionRecording payload (upstream card_game session serde: config + dealt board + instructions) instead; seed stays as presentation metadata only. - v4/v5 files still load through the legacy seed path and are rewritten as v6 on the next save; v6 files load from the recording and replay with per-instruction validation, so tampered or corrupt files surface an error rather than a silently wrong board - saved_moves() removed (dead once serialisation reads recording()) - tests: v6 round-trip via storage, seed-corruption immunity, v5 legacy load, missing-recording rejection, invalid-history rejection Co-Authored-By: Claude Fable 5 --- solitaire_core/src/game_state.rs | 186 ++++++++++++++++++++++++------- solitaire_data/src/storage.rs | 15 ++- 2 files changed, 155 insertions(+), 46 deletions(-) diff --git a/solitaire_core/src/game_state.rs b/solitaire_core/src/game_state.rs index 705f0eb..650b453 100644 --- a/solitaire_core/src/game_state.rs +++ b/solitaire_core/src/game_state.rs @@ -22,11 +22,17 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; /// indices for enum variants. No longer loadable — v3 files are discarded. /// - v4: `saved_moves` uses upstream `KlondikeInstruction` serde with named enum /// variants (e.g. `"Foundation1"` instead of `0`). -/// - v5 (current): `score`, `undo_count`, and `recycle_count` are no longer +/// - v5: `score`, `undo_count`, and `recycle_count` are no longer /// persisted. They are derived from the upstream `card_game`/`klondike` session /// stats, which are rebuilt by replaying `saved_moves` on load. Older files that /// still carry those keys load fine — the extra fields are ignored. -pub const GAME_STATE_SCHEMA_VERSION: u32 = 5; +/// - v6 (current): `saved_moves` replaced by `recording`, the upstream +/// `card_game` session serialisation carrying the dealt board explicitly. +/// Loading no longer re-deals from `seed`, so in-progress saves survive +/// RNG/upstream upgrades that change the seed→deal mapping (the failure +/// class that PR #170 fixed for replays). v4/v5 files still load through +/// the legacy seed path and are rewritten as v6 on next save. +pub const GAME_STATE_SCHEMA_VERSION: u32 = 6; /// Default move budget for a solvability check. Matches the winnable-deal retry /// loop in the engine. @@ -95,8 +101,8 @@ pub enum GameMode { Difficulty(DifficultyLevel), } -/// Output struct for schema v4 serialisation. `saved_moves` uses upstream -/// `KlondikeInstruction` serde, which produces named enum variants. +/// Output struct for schema v6 serialisation. `recording` is the upstream +/// session serialisation (config + dealt board + instruction list). #[derive(Debug, Clone, Serialize)] struct PersistedGameState { pub draw_mode: DrawStockConfig, @@ -105,15 +111,16 @@ struct PersistedGameState { pub seed: u64, pub take_from_foundation: bool, pub schema_version: u32, - pub saved_moves: Vec, + pub recording: SessionRecording, } -/// Input struct that accepts schema v4 and v5 `saved_moves` formats. +/// Input struct that accepts schema v4, v5, and v6 save formats. /// -/// `saved_moves` is deserialised directly as upstream `KlondikeInstruction` -/// (named-variant serde). `score`, `undo_count`, and `recycle_count` are -/// intentionally absent: all three are rebuilt by replaying the instruction -/// history through the upstream session stats. Older v4 save files still carry +/// v6 files carry `recording` (the upstream session serialisation, deal +/// included); v4/v5 files carry `saved_moves` and rebuild the deal from +/// `seed`. `score`, `undo_count`, and `recycle_count` are intentionally +/// absent: all three are rebuilt by replaying the instruction history +/// through the upstream session stats. Older v4 save files still carry /// those keys; serde ignores them. #[derive(Debug, Clone, Deserialize)] struct PersistedGameStateIn { @@ -126,7 +133,12 @@ struct PersistedGameStateIn { pub take_from_foundation: bool, #[serde(default = "schema_v1")] pub schema_version: u32, + /// v4/v5 only. Replayed against a seed-dealt board. + #[serde(default)] pub saved_moves: Vec, + /// v6 only. Carries the dealt board explicitly. + #[serde(default)] + pub recording: Option, } #[cfg(feature = "test-support")] @@ -222,7 +234,7 @@ impl Serialize for GameState { seed: self.seed, take_from_foundation: self.take_from_foundation, schema_version: GAME_STATE_SCHEMA_VERSION, - saved_moves: self.saved_moves(), + recording: self.recording(), } .serialize(serializer) } @@ -232,34 +244,51 @@ impl<'de> Deserialize<'de> for GameState { fn deserialize>(deserializer: D) -> Result { let persisted = PersistedGameStateIn::deserialize(deserializer)?; - // Accept v4 (upstream named-variant serde) and v5 (current, derived - // stats). v3 (legacy u8-index format) and all others are rejected. - match persisted.schema_version { - 4 | 5 => {} + // Accept v6 (current, recording-based), plus v4/v5 (legacy seed-dealt + // saves — loadable while the seed→deal mapping they were written + // under still holds; rewritten as v6 on the next save). v3 (legacy + // u8-index format) and all others are rejected. + let (mut game, instructions) = match persisted.schema_version { + 6 => { + let Some(recording) = persisted.recording else { + return Err(serde::de::Error::custom( + "v6 save file is missing the session recording", + )); + }; + // The dealt board and session config come from the recording + // itself; `seed` is presentation metadata only. + Self::from_recording(&recording, persisted.seed, persisted.mode) + } + 4 | 5 => ( + Self { + mode: persisted.mode, + elapsed_seconds: 0, + seed: persisted.seed, + take_from_foundation: true, + session: Self::new_session(persisted.seed, persisted.draw_mode), + #[cfg(feature = "test-support")] + test_pile_state: None, + }, + persisted.saved_moves, + ), v => { return Err(serde::de::Error::custom(format!( "unsupported GameState schema version {v}" ))); } - } - - let mut game = Self { - mode: persisted.mode, - elapsed_seconds: persisted.elapsed_seconds, - seed: persisted.seed, - take_from_foundation: persisted.take_from_foundation, - session: Self::new_session(persisted.seed, persisted.draw_mode), - #[cfg(feature = "test-support")] - test_pile_state: None, }; + game.mode = persisted.mode; + game.elapsed_seconds = persisted.elapsed_seconds; + game.take_from_foundation = persisted.take_from_foundation; - // Replay the saved instruction history. The upstream session tracks - // score components and recycle_count as it processes each move, so the - // derived stats are correct once replay completes. `undo_count()` resets - // to 0 across save/load because undone moves are not part of the saved - // forward history. + // Replay the saved instruction history with validation — a tampered + // or corrupt file must surface an error, not a silently wrong board. + // The upstream session tracks score components and recycle_count as + // it processes each move, so the derived stats are correct once + // replay completes. `undo_count()` resets to 0 across save/load + // because undone moves are not part of the saved forward history. let replay_config = Self::replay_config(persisted.draw_mode); - for instruction in persisted.saved_moves { + for instruction in instructions { if !game .session .state() @@ -524,16 +553,6 @@ impl GameState { KlondikeAdapter::config_for(self.draw_mode(), self.take_from_foundation) } - /// Collects the session instruction history as upstream types for schema v4 - /// serialisation. - fn saved_moves(&self) -> Vec { - self.session - .history() - .iter() - .map(|snapshot| *snapshot.instruction()) - .collect() - } - /// Returns the deterministic instruction history for the current deal as /// upstream [`KlondikeInstruction`] values. /// @@ -1697,6 +1716,91 @@ mod tests { assert_eq!(replayed.move_count(), game.move_count()); } + /// v6 save files carry the deal in the recording; the `seed` field is + /// presentation metadata. Corrupting it must not change the loaded board. + #[test] + fn v6_save_load_ignores_seed_for_dealing() { + let game = game_with_some_moves(51); + let mut value = serde_json::to_value(&game).expect("serialise"); + assert_eq!(value["schema_version"], 6); + value["seed"] = serde_json::Value::from(0xDEAD_BEEF_u64); + let loaded: GameState = + serde_json::from_value(value).expect("v6 save with wrong seed must still load"); + assert_eq!(loaded.stock_cards(), game.stock_cards()); + assert_eq!(loaded.waste_cards(), game.waste_cards()); + for index in 0..7 { + let t = GameState::tableau_from_index(index).expect("tableau index"); + assert_eq!( + loaded.pile(KlondikePile::Tableau(t)), + game.pile(KlondikePile::Tableau(t)), + "tableau {index} differs" + ); + } + assert_eq!(loaded.move_count(), game.move_count()); + assert_eq!(loaded.score(), game.score()); + } + + /// Legacy v5 files (seed + saved_moves, no recording) must still load + /// through the seed-dealt path until players resave as v6. + #[test] + fn v5_legacy_save_still_loads() { + let game = game_with_some_moves(145); + let v5 = serde_json::json!({ + "draw_mode": game.draw_mode(), + "mode": game.mode, + "elapsed_seconds": 12, + "seed": game.seed, + "take_from_foundation": true, + "schema_version": 5, + "saved_moves": game.instruction_history(), + }); + let loaded: GameState = + serde_json::from_value(v5).expect("v5 save must load via the legacy seed path"); + assert_eq!(loaded.move_count(), game.move_count()); + assert_eq!(loaded.stock_cards(), game.stock_cards()); + assert_eq!(loaded.elapsed_seconds, 12); + } + + /// A v6 file without the recording payload is malformed and must be + /// rejected with an error rather than silently re-dealt from the seed. + #[test] + fn v6_save_without_recording_is_rejected() { + let game = GameState::new(7, DrawStockConfig::DrawOne); + let mut value = serde_json::to_value(&game).expect("serialise"); + value.as_object_mut().expect("object").remove("recording"); + let err = serde_json::from_value::(value) + .expect_err("v6 save without recording must fail"); + assert!(err.to_string().contains("recording"), "got: {err}"); + } + + /// A recording whose instruction list is invalid for its own deal must + /// be rejected on load — tampered or corrupt files surface an error, + /// never a silently wrong board. + #[test] + fn v6_save_with_invalid_history_is_rejected() { + use klondike::{DstFoundation, Foundation, KlondikePile}; + // A foundation move from an empty tableau is never legal on a fresh + // deal's first instruction slot for this source shape. + let bogus = KlondikeInstruction::DstFoundation(DstFoundation { + src: KlondikePile::Foundation(Foundation::Foundation1), + foundation: Foundation::Foundation2, + }); + let recording = + SessionRecording::from_instructions_unchecked(7, DrawStockConfig::DrawOne, [bogus]); + let v6 = serde_json::json!({ + "draw_mode": DrawStockConfig::DrawOne, + "mode": GameMode::Classic, + "elapsed_seconds": 0, + "seed": 7, + "take_from_foundation": true, + "schema_version": 6, + "recording": recording, + }); + let err = + serde_json::from_value::(v6).expect_err("invalid history must be rejected"); + assert!(err.to_string().contains("invalid"), "got: {err}"); + } + #[test] fn from_recording_of_fresh_deal_returns_empty_instructions() { let game = GameState::new(7, DrawStockConfig::DrawThree); diff --git a/solitaire_data/src/storage.rs b/solitaire_data/src/storage.rs index 294ab3a..bcc3737 100644 --- a/solitaire_data/src/storage.rs +++ b/solitaire_data/src/storage.rs @@ -477,7 +477,7 @@ mod tests { /// again must reproduce byte-identical JSON. `undo_count` deliberately resets /// to 0 on load because only the forward instruction history is persisted. #[test] - fn game_state_v5_mid_game_round_trip() { + fn game_state_v6_mid_game_round_trip() { use solitaire_core::KlondikeInstruction; use solitaire_core::game_state::GameState; @@ -518,11 +518,16 @@ mod tests { save_game_state_to(&path, &gs).expect("save"); - // Verify the file carries the v5 schema marker. + // Verify the file carries the v6 schema marker and the recording. let json = fs::read_to_string(&path).expect("read json"); + let parsed: serde_json::Value = serde_json::from_str(&json).expect("parse saved json"); + assert_eq!( + parsed["schema_version"], 6, + "saved file must use schema version 6", + ); assert!( - json.contains("\"schema_version\"") && json.contains('5'), - "saved file must use schema version 5", + parsed["recording"].is_object(), + "saved file must embed the session recording", ); let loaded = @@ -530,7 +535,7 @@ mod tests { // The forward instruction history round-trips, so the reconstructed board // re-serialises to byte-identical JSON. - let path_reload = gs_path("v5_mid_game_reload"); + let path_reload = gs_path("v6_mid_game_reload"); let _ = fs::remove_file(&path_reload); save_game_state_to(&path_reload, &loaded).expect("re-save loaded"); assert_eq!( -- 2.47.3