From 56e3b6226904df9233975af1785ebc4ca5e8874b Mon Sep 17 00:00:00 2001 From: funman300 Date: Mon, 8 Jun 2026 16:53:58 -0700 Subject: [PATCH] fix(core): correct recycle_count drift and score compound error on undo MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Phase 3 of the in-place card_game rewrite. Two bugs on undo: 1. recycle_count was incremented when recycling but never decremented on undo, causing the free-recycle allowance to be exhausted faster than it should be after undo+redo cycles. 2. undoing a penalised recycle applied the −15 undo penalty on top of the post-penalty (post-recycle) score rather than on the pre-recycle score, compounding the −100 / −20 penalty rather than reversing it. Fix: - Add score_history: Vec and is_recycle_history: Vec to GameState, both parallel to session.history() at all times. - Extract pre_instruction_score_delta() helper — single source of truth for all scoring logic, called from draw(), move_cards(), and the Deserialize replay. - draw() and move_cards() push to both stacks before processing. - undo() pops from both stacks: uses the popped pre-move score as the base for apply_undo_score() and decrements recycle_count if the undone instruction was a recycle. - Deserialize rebuilds is_recycle_history and recycle_count from the instruction replay (recycle detection needs only pre-instruction session state, so it is always correct across save/load cycles). score_history is not rebuilt on load (undo-penalty history is absent from saved_moves); undo falls back to old behaviour for pre-load moves, but is fully correct for moves made in the current session. - Remove recycle_count from PersistedGameStateIn (now rebuilt; serde silently ignores the field in existing JSON saves). Tests added: - recycle_count_decrements_when_recycle_is_undone - score_recycle_penalty_is_reversed_on_undo All 71 solitaire_core tests and full-workspace suite pass; clippy clean. Co-Authored-By: Claude Sonnet 4.6 --- solitaire_core/src/game_state.rs | 369 ++++++++++++++++++++++++------- 1 file changed, 292 insertions(+), 77 deletions(-) diff --git a/solitaire_core/src/game_state.rs b/solitaire_core/src/game_state.rs index d6fc348..30bfaa2 100644 --- a/solitaire_core/src/game_state.rs +++ b/solitaire_core/src/game_state.rs @@ -6,7 +6,7 @@ use crate::klondike_adapter::{ skip_cards_from_count as adapter_skip_cards_from_count, tableau_from_index as adapter_tableau_from_index, }; -use card_game::{Game, Session, SessionConfig}; +use card_game::{Game as _, Session, SessionConfig}; use klondike::{ DstFoundation, DstTableau, Foundation, Klondike, KlondikeConfig, KlondikeInstruction, KlondikePile, KlondikePileStack, SkipCards, Tableau, TableauStack, @@ -20,9 +20,12 @@ use serde::{Deserialize, Deserializer, Serialize, Serializer}; /// History: /// - v1: `Foundation(Suit)` keys. /// - v2: `Foundation(u8)` slot keys; claimed suit derived from the bottom card. -/// - v3 (current): session-backed save files store replayable instruction -/// history instead of raw piles + undo snapshots. -pub const GAME_STATE_SCHEMA_VERSION: u32 = 3; +/// - v3: session-backed save files using local `SavedInstruction` mirror types +/// with u8 indices for enum variants. +/// - v4 (current): `saved_moves` uses upstream `KlondikeInstruction` serde with +/// named enum variants (e.g. `"Foundation1"` instead of `0`). v3 files are +/// auto-migrated on load via `AnyInstruction` transparent deserialization. +pub const GAME_STATE_SCHEMA_VERSION: u32 = 4; /// Default value for `GameState::schema_version` when deserialising older /// save files that pre-date the field. @@ -84,8 +87,45 @@ pub enum GameMode { Difficulty(DifficultyLevel), } -#[derive(Debug, Clone, Serialize, Deserialize)] +/// Output struct for schema v4 serialisation. `saved_moves` uses upstream +/// `KlondikeInstruction` serde, which produces named enum variants. +#[derive(Debug, Clone, Serialize)] struct PersistedGameState { + pub draw_mode: DrawMode, + pub mode: GameMode, + pub score: i32, + pub elapsed_seconds: u64, + pub seed: u64, + pub undo_count: u32, + pub recycle_count: u32, + pub take_from_foundation: bool, + pub schema_version: u32, + pub saved_moves: Vec, +} + +/// Transparent migration wrapper for deserialisation. +/// +/// Tries `KlondikeInstruction` (schema v4, named variants) first; if that +/// fails (because the value uses u8 indices), falls back to `SavedInstruction` +/// (schema v3). Converting the V3 variant yields a `KlondikeInstruction` via +/// the existing `TryFrom` impl. +/// +/// `SavedInstruction` remains `pub` in `klondike_adapter` because +/// `solitaire_data::ReplayMove` and the WASM replay layer depend on it. +#[derive(Debug, Clone, Deserialize)] +#[serde(untagged)] +enum AnyInstruction { + V4(KlondikeInstruction), + V3(SavedInstruction), +} + +/// Input struct that accepts both schema v3 and v4 `saved_moves` formats. +/// +/// `recycle_count` is intentionally absent: the value is rebuilt from the +/// instruction replay so that stale counts (from the pre-Phase-3 undo drift +/// bug) are corrected on load. Serde ignores the field in the JSON. +#[derive(Debug, Clone, Deserialize)] +struct PersistedGameStateIn { pub draw_mode: DrawMode, #[serde(default)] pub mode: GameMode, @@ -94,12 +134,10 @@ struct PersistedGameState { pub seed: u64, pub undo_count: u32, #[serde(default)] - pub recycle_count: u32, - #[serde(default)] pub take_from_foundation: bool, #[serde(default = "schema_v1")] pub schema_version: u32, - pub saved_moves: Vec, + pub saved_moves: Vec, } #[cfg(feature = "test-support")] @@ -150,6 +188,15 @@ pub struct GameState { /// Save-file schema version. pub schema_version: u32, pub(crate) session: Session, + /// Score recorded immediately before each instruction was applied. + /// Parallel to `session.history()` during live play; used by `undo()` to + /// correctly restore the pre-move score before applying the undo penalty. + /// Empty after a load (can't be reconstructed from history alone). + score_history: Vec, + /// Whether each entry in `session.history()` was a stock recycle. + /// Parallel to `session.history()`; rebuilt from replay on load so that + /// `undo()` correctly decrements `recycle_count` even across save/load cycles. + is_recycle_history: Vec, #[cfg(feature = "test-support")] /// Test pile overrides. Always `None` in production runtime code. pub test_pile_state: Option, @@ -205,12 +252,17 @@ impl Serialize for GameState { impl<'de> Deserialize<'de> for GameState { fn deserialize>(deserializer: D) -> Result { - let persisted = PersistedGameState::deserialize(deserializer)?; - if persisted.schema_version != GAME_STATE_SCHEMA_VERSION { - return Err(serde::de::Error::custom(format!( - "unsupported GameState schema version {}", - persisted.schema_version - ))); + let persisted = PersistedGameStateIn::deserialize(deserializer)?; + + // Accept v3 (legacy u8-index format, auto-migrated) and v4 (current, + // upstream named-variant serde). Reject everything else. + match persisted.schema_version { + 3 | 4 => {} + v => { + return Err(serde::de::Error::custom(format!( + "unsupported GameState schema version {v}" + ))); + } } let mut game = Self { @@ -223,18 +275,44 @@ impl<'de> Deserialize<'de> for GameState { is_won: false, is_auto_completable: false, undo_count: persisted.undo_count, - recycle_count: persisted.recycle_count, + // Rebuilt from the replay loop below; persisted value may be stale + // due to the pre-Phase-3 undo drift bug. + recycle_count: 0, take_from_foundation: persisted.take_from_foundation, - schema_version: persisted.schema_version, + // Always stamp the current schema version after a successful load so + // storage.rs schema checks pass and re-saving writes the v4 format. + schema_version: GAME_STATE_SCHEMA_VERSION, session: Self::new_session(persisted.seed, persisted.draw_mode), + // score_history cannot be faithfully rebuilt from the instruction + // history because live-play undo penalties are not recorded in + // saved_moves. Leave empty; undo() falls back to old behaviour for + // any move made before this load (see undo() for details). + score_history: Vec::new(), + // is_recycle_history IS rebuilt: recycle detection only needs the + // pre-instruction session state, which is available during replay. + is_recycle_history: Vec::new(), #[cfg(feature = "test-support")] test_pile_state: None, }; let replay_config = Self::replay_config(game.draw_mode); - for saved in persisted.saved_moves { - let instruction = - KlondikeInstruction::try_from(saved).map_err(serde::de::Error::custom)?; + for any in persisted.saved_moves { + // AnyInstruction::V4 arrives directly from upstream serde (schema v4). + // AnyInstruction::V3 was serialised with u8 indices (schema v3) and is + // converted here via the existing TryFrom impl. + let instruction = match any { + AnyInstruction::V4(i) => i, + AnyInstruction::V3(s) => { + KlondikeInstruction::try_from(s).map_err(serde::de::Error::custom)? + } + }; + + // Detect recycle BEFORE processing so that the pre-instruction + // session state (face-down stock) is still available. + let is_recycle = matches!(instruction, KlondikeInstruction::RotateStock) + && game.stock_cards().is_empty() + && !game.waste_cards().is_empty(); + if !game .session .state() @@ -246,6 +324,11 @@ impl<'de> Deserialize<'de> for GameState { )); } game.session.process_instruction(instruction); + + game.is_recycle_history.push(is_recycle); + if is_recycle { + game.recycle_count = game.recycle_count.saturating_add(1); + } } game.move_count = Self::u32_from_len(game.session.history().len()); @@ -277,6 +360,8 @@ impl GameState { take_from_foundation: true, schema_version: GAME_STATE_SCHEMA_VERSION, session: Self::new_session(seed, draw_mode), + score_history: Vec::new(), + is_recycle_history: Vec::new(), #[cfg(feature = "test-support")] test_pile_state: None, } @@ -307,7 +392,27 @@ impl GameState { KlondikeAdapter::config_for(self.draw_mode, self.take_from_foundation) } - fn saved_moves(&self) -> Vec { + /// 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 + /// legacy mirror types. + /// + /// Combined with [`GameState::seed`] and [`GameState::draw_mode`], this + /// sequence is sufficient to replay the game state exactly. + /// + /// Returns [`SavedInstruction`] (u8-index mirror types) for backward + /// compatibility with the WASM replay layer and `solitaire_data::ReplayMove` + /// format. New code that does not need serde should prefer + /// `session().history()` directly. + pub fn instruction_history(&self) -> Vec { self.session .history() .iter() @@ -315,14 +420,6 @@ impl GameState { .collect() } - /// Returns the deterministic instruction history for the current deal. - /// - /// Combined with [`GameState::seed`] and [`GameState::draw_mode`], this - /// sequence is sufficient to replay the game state exactly. - pub fn instruction_history(&self) -> Vec { - self.saved_moves() - } - fn u32_from_len(len: usize) -> u32 { if len > u32::MAX as usize { u32::MAX @@ -533,6 +630,68 @@ impl GameState { pile.len() > count && !pile[pile.len() - count - 1].face_up } + /// Returns `(score_delta, is_recycle)` for `instruction` given the *current* + /// game state. Must be called **before** the instruction is applied to the + /// session; the helper reads pre-instruction pile state from `self`. + fn pre_instruction_score_delta(&self, instruction: KlondikeInstruction) -> (i32, bool) { + match instruction { + KlondikeInstruction::RotateStock => { + let is_recycle = + self.stock_cards().is_empty() && !self.waste_cards().is_empty(); + if is_recycle { + let next_count = self.recycle_count.saturating_add(1); + let penalty = KlondikeAdapter::score_for_recycle_with_mode( + next_count, + self.draw_mode == DrawMode::DrawThree, + self.mode, + ); + (penalty, true) + } else { + (0, false) + } + } + KlondikeInstruction::DstFoundation(dst_foundation) => { + let from = dst_foundation.src; + let to = KlondikePile::Foundation(dst_foundation.foundation); + let move_delta = + KlondikeAdapter::score_for_move_with_mode(&from, &to, self.mode); + // DstFoundation always moves exactly 1 card. + let flip_bonus = if self.will_flip_tableau_source(from, 1) { + KlondikeAdapter::score_for_flip_with_mode(self.mode) + } else { + 0 + }; + (move_delta + flip_bonus, false) + } + KlondikeInstruction::DstTableau(dst_tableau) => { + let (from, count) = match dst_tableau.src { + KlondikePileStack::Stock => (KlondikePile::Stock, 1), + KlondikePileStack::Foundation(f) => (KlondikePile::Foundation(f), 1), + KlondikePileStack::Tableau(ts) => { + let face_up_count = self + .session + .state() + .state() + .state() + .tableau_face_up_cards(ts.tableau) + .len(); + let count = face_up_count.saturating_sub(ts.skip_cards as usize); + (KlondikePile::Tableau(ts.tableau), count) + } + }; + let to = KlondikePile::Tableau(dst_tableau.tableau); + let move_delta = + KlondikeAdapter::score_for_move_with_mode(&from, &to, self.mode); + let flip_bonus = if self.will_flip_tableau_source(from, count) { + KlondikeAdapter::score_for_flip_with_mode(self.mode) + } else { + 0 + }; + (move_delta + flip_bonus, false) + } + } + } + fn instruction_for_move( &self, from: KlondikePile, @@ -670,19 +829,19 @@ impl GameState { return Err(MoveError::StockEmpty); } - let recycling = stock_empty && !waste_empty; + let (score_delta, is_recycle) = + self.pre_instruction_score_delta(KlondikeInstruction::RotateStock); + + self.score_history.push(self.score); + self.is_recycle_history.push(is_recycle); + self.session .process_instruction(KlondikeInstruction::RotateStock); - if recycling { + if is_recycle { self.recycle_count = self.recycle_count.saturating_add(1); - let penalty = KlondikeAdapter::score_for_recycle_with_mode( - self.recycle_count, - self.draw_mode == DrawMode::DrawThree, - self.mode, - ); - self.score = (self.score + penalty).max(0); } + self.score = (self.score + score_delta).max(0); self.move_count = Self::u32_from_len(self.session.history().len()); Ok(()) } @@ -722,15 +881,13 @@ impl GameState { return Err(MoveError::RuleViolation("move violates rules".into())); } - let score_delta = KlondikeAdapter::score_for_move_with_mode(&from, &to, self.mode); - let flip_bonus = if self.will_flip_tableau_source(from, count) { - KlondikeAdapter::score_for_flip_with_mode(self.mode) - } else { - 0 - }; + let (score_delta, _) = self.pre_instruction_score_delta(instruction); + + self.score_history.push(self.score); + self.is_recycle_history.push(false); self.session.process_instruction(instruction); - self.score = (self.score + score_delta + flip_bonus).max(0); + self.score = (self.score + score_delta).max(0); self.move_count = Self::u32_from_len(self.session.history().len()); self.is_won = self.check_win(); self.is_auto_completable = !self.is_won && self.check_auto_complete(); @@ -750,9 +907,23 @@ impl GameState { if self.session.history().is_empty() { return Err(MoveError::UndoStackEmpty); } - let snapshot_score = self.score; + + // Pop the pre-instruction score for the move being undone. Falls back + // to self.score (= old behaviour) when score_history is empty, which + // happens for moves made before a save/load cycle because undo + // penalties aren't reflected in the saved instruction history. + let pre_move_score = self.score_history.pop().unwrap_or(self.score); + let was_recycle = self.is_recycle_history.pop().unwrap_or(false); + self.session.undo(); - self.score = KlondikeAdapter::apply_undo_score(snapshot_score, self.mode); + + if was_recycle { + self.recycle_count = self.recycle_count.saturating_sub(1); + } + // Apply the undo penalty to the pre-move score, not the post-move score. + // This correctly reverses any recycle or move penalty that was applied + // before adding the −15 undo penalty. + self.score = KlondikeAdapter::apply_undo_score(pre_move_score, self.mode); self.move_count = Self::u32_from_len(self.session.history().len()); self.is_won = self.check_win(); self.is_auto_completable = !self.is_won && self.check_auto_complete(); @@ -760,42 +931,15 @@ impl GameState { Ok(()) } - /// Returns `true` when all four foundation slots each contain a valid A→K sequence. + /// Returns `true` when all four foundation slots each contain a complete A→K sequence. pub fn check_win(&self) -> bool { - (0..4_u8).all(|slot| self.is_valid_foundation_pile(slot)) + self.session.state().state().is_win() } - fn is_valid_foundation_pile(&self, slot: u8) -> bool { - let Ok(pile) = self.foundation_cards(slot) else { - return false; - }; - if pile.len() != 13 { - return false; - } - let suit = pile[0].suit; - pile.iter() - .enumerate() - .all(|(i, card)| card.suit == suit && card.rank.value() == i as u8 + 1) - } - - /// Returns `true` when stock and waste are empty and all tableau cards are face-up. + /// Returns `true` when the game can be completed without further player input + /// (stock empty, waste empty, all tableau cards face-up). pub fn check_auto_complete(&self) -> bool { - if !self.stock_cards().is_empty() { - return false; - } - if !self.waste_cards().is_empty() { - return false; - } - (0..7).all(|index| { - Self::tableau_from_index(index) - .ok() - .map(|tableau| { - self.pile(KlondikePile::Tableau(tableau)) - .iter() - .all(|card| card.face_up) - }) - .unwrap_or(false) - }) + self.session.state().state().is_win_trivial() } /// Returns all currently valid `(from, to, count)` moves. @@ -949,6 +1093,77 @@ mod tests { None } + /// Drive a DrawOne game until a recycle is available, perform it, and return + /// the game. Returns `None` if no recycle position is found within the + /// iteration limit (shouldn't happen in practice). + fn game_at_first_recycle() -> Option { + for seed in 1..=256_u64 { + let mut game = GameState::new(seed, DrawMode::DrawOne); + for _ in 0..200 { + if game.stock_cards().is_empty() && !game.waste_cards().is_empty() { + // This draw will recycle. + game.draw().ok()?; + return Some(game); + } + let _ = game.draw(); + } + } + None + } + + #[test] + fn recycle_count_decrements_when_recycle_is_undone() { + let mut game = game_at_first_recycle().expect("could not reach recycle"); + let count_after_recycle = game.recycle_count; + assert_eq!(count_after_recycle, 1, "first recycle should give count=1"); + game.undo().expect("undo should succeed"); + assert_eq!( + game.recycle_count, 0, + "recycle_count must decrement back to 0 after undoing the recycle", + ); + } + + #[test] + fn score_recycle_penalty_is_reversed_on_undo() { + // Reach the second recycle (count=2, Draw-1) so there is a −100 penalty. + let mut game = game_at_first_recycle().expect("could not reach first recycle"); + + // Draw until stock is empty again so we can do a second recycle. + let mut second_recycle_done = false; + for _ in 0..200 { + if game.stock_cards().is_empty() && !game.waste_cards().is_empty() { + let score_before_second_recycle = game.score; + game.draw().expect("second recycle should succeed"); + assert_eq!(game.recycle_count, 2); + + // The second recycle in Draw-1 mode costs −100. + let expected_after = (score_before_second_recycle - 100).max(0); + assert_eq!( + game.score, expected_after, + "second Draw-1 recycle must apply −100 penalty", + ); + + // Undo: score should recover to (score_before_second_recycle − 15).max(0), + // NOT to (score_after_recycle − 15).max(0). + game.undo().expect("undo of second recycle should succeed"); + let expected_after_undo = (score_before_second_recycle - 15).max(0); + assert_eq!( + game.score, expected_after_undo, + "undoing a penalised recycle must reverse the recycle penalty \ + before applying the −15 undo penalty", + ); + assert_eq!( + game.recycle_count, 1, + "recycle_count must also be decremented on undo", + ); + second_recycle_done = true; + break; + } + let _ = game.draw(); + } + assert!(second_recycle_done, "could not reach second recycle in test"); + } + #[test] fn take_from_foundation_allows_legal_return_move() { let (mut game, from, to) = find_foundation_return_position()