diff --git a/docs/card-game-integration.md b/docs/card-game-integration.md index 82d81d7..3c6b2cc 100644 --- a/docs/card-game-integration.md +++ b/docs/card-game-integration.md @@ -94,21 +94,22 @@ Our 767-line `solitaire_core::solver` reimplements the full game rules to run th ### 4. `take_from_foundation` House Rule *(upstream merged — v0.3.0)* `MoveFromFoundationConfig` is now part of `KlondikeConfig`. When set to `Disallowed`, `is_instruction_valid` blocks foundation → tableau instructions. -**Important:** The upstream default is `MoveFromFoundationConfig::Allowed`. Ferrous Solitaire uses the standard rule (foundation cards cannot be moved back) as the default, with the house rule as an opt-in. Our adapter explicitly sets `Disallowed` in the default `KlondikeConfig` and switches to `Allowed` only when the user toggles the house-rule option. +**Default behaviour:** The upstream default is `MoveFromFoundationConfig::Allowed`. Ferrous Solitaire **also defaults to Allowed** (`take_from_foundation: true` in `GameState`, `Settings`). This matches the upstream default and provides the most beginner-friendly experience. The player can disable foundation returns via a settings toggle (`take_from_foundation = false`), which maps to `Disallowed`. -**In our wrapper:** Construct `KlondikeConfig { move_from_foundation: MoveFromFoundationConfig::Disallowed, .. }` by default; mirror the user's settings toggle to `Allowed`. No custom intercept needed — `klondike` enforces the rule automatically. +**In our wrapper:** `KlondikeAdapter::config_for(draw_mode, take_from_foundation)` constructs `KlondikeConfig { move_from_foundation: if take_from_foundation { Allowed } else { Disallowed }, .. }`. No custom intercept needed — `klondike` enforces the rule automatically. ### 5. JSON Serialisation / Persistence -`solitaire_core::GameState` serialises the full mid-game state to JSON via `serde` so the engine can save on exit and restore on launch. `KlondikeState` derives `Clone` + `Eq` + `Hash` but not `Serialize` / `Deserialize`. No upstream changes are needed — this is handled externally. +`solitaire_core::GameState` serialises the full mid-game state to JSON via `serde` so the engine can save on exit and restore on launch. -**Current verification (2026-06-01):** `klondike v0.3.0` and `card_game v0.4.0` -crate manifests expose no `serde` dependency/feature, and source exports no -serde derives for instruction/state snapshot types. Keep Ferrous' -`SavedInstruction` bridge in place. +**Upstream serde status (rev 99b49e62):** At this revision, `klondike` and `card_game` both enable a `serde` feature. All nine instruction/pile types (`KlondikeInstruction`, `KlondikePile`, `KlondikePileStack`, `DstFoundation`, `DstTableau`, `TableauStack`, `Foundation`, `Tableau`, `SkipCards`) derive `serde::Serialize` + `serde::Deserialize` under that feature. The workspace `Cargo.toml` enables `features = ["serde"]`. -**Session history:** `StateSnapshot` stores the pre-move game state and instruction. On load, the session is reconstructed from the serialised snapshot history — no full replay from seed needed. +**Schema v4 (current):** `saved_moves` serialises as `Vec` using upstream named-variant serde. Example: `{"DstFoundation": {"src": "Stock", "foundation": "Foundation1"}}`. -**In our wrapper:** Serialise the `solitaire_core` wrapper struct using newtypes. Define `SavedInstruction` (a `Serialize + Deserialize` mirror of `KlondikeInstruction`) and `SavedStateSnapshot`. Reconstruct `SessionState` from the deserialised history. Schema version field lives on our wrapper. +**Schema v3 (legacy, auto-migrated):** `saved_moves` used local `SavedInstruction` mirror types with u8 indices. Example: `{"DstFoundation": {"src": "Stock", "foundation": 0}}`. On load, an `AnyInstruction` untagged serde enum transparently upgrades v3 instructions to v4 and the file is written back in v4 format. The `SavedInstruction` bridge types are retained in `solitaire_core::klondike_adapter` for this migration path and for backward-compatible `solitaire_data::ReplayMove` / WASM replay formats. + +**Session history:** `StateSnapshot` stores the pre-move game state and instruction. On load, the session is reconstructed by replaying the instruction history against a fresh deal — no full state snapshot needed. + +**In our wrapper:** `GameState::Serialize` emits schema v4 (upstream instruction types). `GameState::Deserialize` accepts v3 (auto-migrates) and v4 (direct). Schema version field lives on our wrapper. ### 6. Typed Move Errors `solitaire_core::error::MoveError` returns structured errors the engine uses to trigger UI feedback (wrong-destination toast, stock-empty chime, etc.): diff --git a/docs/in-place-card-game-rewrite-plan.md b/docs/in-place-card-game-rewrite-plan.md new file mode 100644 index 0000000..4bc11df --- /dev/null +++ b/docs/in-place-card-game-rewrite-plan.md @@ -0,0 +1,408 @@ +# In-Place card_game / klondike Rewrite Plan + +**Date:** 2026-06-08 +**Upstream rev:** `99b49e62` +**Status:** Phases 0–2 complete. Pre-Phase 3 undo audit complete (see §5 below). Awaiting approval for Phase 3. + +--- + +## 1. What Is Already Integrated + +The integration is substantially complete. `solitaire_core` already delegates all +authoritative Klondike logic to the upstream crates. + +| Area | Status | Location | +|---|---|---| +| `Session` ownership | ✅ complete | `GameState.session` | +| `draw()` → `session.process_instruction(RotateStock)` | ✅ complete | `game_state.rs` | +| `move_cards()` → `session.process_instruction(KlondikeInstruction)` | ✅ complete | `game_state.rs` | +| `undo()` → `session.undo()` | ✅ complete | `game_state.rs` | +| `possible_instructions()` → `session.state().state().get_sorted_moves()` | ✅ complete | `game_state.rs` | +| `can_move_cards()` → `session.state().state().is_instruction_valid()` | ✅ complete | `game_state.rs` | +| `solver.rs` → `session.solve()` | ✅ complete | `solver.rs` | +| `Suit`, `Rank` → re-export from `card_game` | ✅ complete | `card.rs` | +| `Foundation`, `Klondike`, `KlondikePile`, `Session`, `Tableau` → `solitaire_core::lib` | ✅ complete | `lib.rs` | +| Move legality enforcement | ✅ upstream (`is_instruction_valid`) | `klondike/src/lib.rs` | +| Foundation placement rules (Ace start, suit match) | ✅ upstream | `klondike/src/lib.rs` | +| Tableau placement rules (alternating colour, King on empty) | ✅ upstream | `klondike/src/lib.rs` | +| Multi-card stack moves via `SkipCards` | ✅ upstream | `klondike/src/lib.rs` | +| Session history / snapshot undo | ✅ upstream | `card_game/src/lib.rs` | +| DFS solver with budget limits | ✅ upstream | `card_game/src/lib.rs` | +| Instruction history → `SavedInstruction` serde mirrors | ✅ in adapter | `klondike_adapter.rs` | +| Schema v3 save/load (instruction replay) | ✅ complete | `game_state.rs`, `storage.rs` | +| `take_from_foundation` house rule → `MoveFromFoundationConfig` | ✅ complete | `klondike_adapter.rs` | + +--- + +## 2. Duplicated / Replaceable Logic + +These are local implementations that either replicate upstream or could be removed. + +### 2a. `SavedInstruction` mirror types (~300 lines, `klondike_adapter.rs`) + +**What:** A full hand-written serde mirror for every upstream klondike instruction type +(`SavedInstruction`, `SavedDstFoundation`, `SavedDstTableau`, `SavedKlondikePile`, +`SavedKlondikePileStack`, `SavedTableauStack`, `SavedTableau`, `SavedFoundation`, +`SavedSkipCards`, `InvalidSavedInstruction`) plus ~20 `From`/`TryFrom` conversion impls. + +**Why written:** At the time, upstream klondike had no serde feature. + +**Current upstream status:** At rev `99b49e62`, the `serde` feature is present and active. +`KlondikeInstruction`, `KlondikePile`, `KlondikePileStack`, `DstFoundation`, `DstTableau`, +`TableauStack`, `Tableau`, `Foundation`, `SkipCards` all derive +`#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]`. + +**Blocker — JSON format incompatibility:** +| Field | Local `SavedInstruction` JSON | Upstream `KlondikeInstruction` JSON | +|---|---|---| +| Tableau index | `{ "Tableau": 0 }` (u8) | `{ "Tableau": "Tableau1" }` (named) | +| Foundation slot | `{ "Foundation": 0 }` (u8) | `{ "Foundation": "Foundation1" }` (named) | +| Skip count | `{ "skip_cards": 0 }` (u8) | `{ "skip_cards": "Skip0" }` (named) | + +Switching to direct upstream serde **changes the `saved_moves` JSON shape** stored in +`game_state.json`. Any existing v3 save file would fail to deserialize after the switch. +This requires either: +- A schema bump to v4 **with a migration** (deserialize v3 manually then re-save as v4), or +- A schema bump to v4 **with graceful fallback** (v3 files rejected → fresh game). + +**Recommendation:** Schema v4 with graceful fallback (v3 saves start fresh). Migration +is feasible but adds ~100 lines of throwaway code; the in-progress game loss is modest +since schema v3 was never shipped to users (it landed in the current dev branch, not a +release). + +### 2b. `GameState::check_win()` (~15 lines) + +**What:** Iterates all four foundation slots checking 13-card A→K sequences. +**Upstream equivalent:** `session.state().state().is_win()` on `Klondike`. +**Status:** Local check is correct but redundant. Trivially replaceable with no format change. +**Risk:** None — only affects `is_won` flag update path. + +### 2c. `GameState::check_auto_complete()` (~15 lines) + +**What:** Checks stock empty, waste empty, all tableau cards face-up. +**Upstream equivalent:** `session.state().state().is_win_trivial()` on `Klondike`. +**Semantic difference:** Upstream `is_win_trivial` checks `stock.is_empty()` (both faces) +and all `tableau.face_down().is_empty()`. Ferrous additionally checks `waste.is_empty()`. +These are logically equivalent for a valid game state (waste = stock face-up half). +**Risk:** Low — validated by existing auto-complete engine tests. + +### 2c. `recycle_count` drift on undo (existing bug, not new) + +**What:** `GameState.recycle_count` is incremented in `draw()` when stock is empty. +`undo()` does not decrement it. After undoing a recycle, `recycle_count` is stale and +may cause incorrect future penalty application. +**Upstream:** `KlondikeStats.recycle_count()` has the same problem — it is cumulative +and not restored on undo (stats are not part of the session snapshot, only game state is). +**Fix approach:** After each undo, recompute `recycle_count` by scanning +`session.history()` for `RotateStock` instructions that caused recycling. +**Priority:** Medium — affects scoring correctness in rare paths. File as a separate bug. + +--- + +## 3. What Must Remain Ferrous-Specific + +These responsibilities are product-layer, not Klondike-rules-layer, and must stay in `solitaire_core`. + +| Responsibility | Why upstream cannot own it | +|---|---| +| WXP recycle penalties (free allowance + -100/-20) | `ScoringConfig::recycle` is a flat delta; no free-allowance concept exists upstream | +| Score floor (`score.max(0)`) | Not modelled upstream | +| Time bonus (`700_000 / elapsed_seconds`) | Not modelled upstream | +| `DrawMode` / `GameMode` enums | Product concept; not in upstream | +| Challenge mode undo block | Product rule | +| Zen mode scoring suppression | Product rule | +| `MoveError` variants for UI feedback | Upstream returns `bool`; Ferrous needs typed errors | +| `card::Card` projection (adds `id`, `face_up`) | Renderer requires stable `id` and face orientation | +| `Pile` DTO for engine sync | Renderer-facing snapshot type | +| `stock_cards()` / `waste_cards()` distinction | Engine models waste as a separate pile; upstream uses stock face-up half | +| `recycle_count` tracking | Needed for free-allowance penalty calculation | +| Persistence format + schema versioning | Product concern | +| `SavedInstruction` (currently) or upstream serde (after migration) | Either way, Ferrous owns the save contract | + +--- + +## 4. Key Audit Findings + +### Finding 1 — Upstream serde claim in docs is stale + +`docs/card-game-integration.md` (last section "JSON Serialisation") states: + +> Current verification (2026-06-01): klondike v0.3.0 and card_game v0.4.0 crate manifests +> expose no serde dependency/feature. + +**This is wrong at rev 99b49e62.** The `serde` feature is present and active. All nine +instruction/pile types have `#[cfg_attr(feature = "serde", derive(...))]`. The doc must +be updated. + +### Finding 2 — `take_from_foundation` default: docs vs code + +`docs/card-game-integration.md` says: +> Ferrous Solitaire uses the standard rule (foundation cards cannot be moved back) as the +> default, with the house rule as an opt-in. + +**The code and settings say the opposite:** `Settings::take_from_foundation` defaults to +`true` (Allowed); `GameState.take_from_foundation` also initializes to `true`. Multiple +tests assert this is the intended behavior. The upstream default is also `Allowed`. + +**Resolution:** The docs are wrong. Default = Allowed (house rule on by default for +beginner-friendliness) is intentional. Update the docs; do not change the code. + +### Finding 3 — `KlondikeStats` cumulative vs session-history-aware counts + +`KlondikeStats.moves()` and `KlondikeStats.recycle_count()` accumulate monotonically. +They are NOT restored when `Session::undo()` is called (only `Klondike` game state is +restored from the snapshot, not the stats). Ferrous correctly uses +`session.history().len()` for `move_count` (history-aware). But `recycle_count` is +stored separately in `GameState` and also not decremented on undo — making them +equivalent in this one bug. + +### Finding 4 — `SkipCards as usize` cast is correct + +Upstream `SkipCards` has no explicit discriminants, so `Skip0 = 0 .. Skip12 = 12`. +`skip_cards as usize` in `solver.rs` and `game_state.rs` is correct. + +--- + +## 5. Staged Migration + +### Phase 0 — Doc fixes only (no code change) + +Files: `docs/card-game-integration.md` + +- Correct the serde claim (upstream has serde at rev 99b49e62). +- Correct the `take_from_foundation` default description. +- Update integration status table. + +### Phase 1 — Delegate `is_win` / `is_win_trivial` (safe, no format change) + +Files: `solitaire_core/src/game_state.rs` + +Replace local `check_win()` and `check_auto_complete()` with upstream delegation: + +```rust +// before +pub fn check_win(&self) -> bool { ... 40 lines ... } + +// after +pub fn check_win(&self) -> bool { + self.session.state().state().is_win() +} +``` + +```rust +// before +pub fn check_auto_complete(&self) -> bool { ... 15 lines ... } + +// after +pub fn check_auto_complete(&self) -> bool { + self.session.state().state().is_win_trivial() +} +``` + +**Risk:** Very low. Both methods are tested by existing integration tests. The semantic +difference in `check_auto_complete` (upstream vs Ferrous definition) is equivalent for +valid game states. + +### Phase 2 — Replace `SavedInstruction` with upstream serde (schema v4) + +Files: +- `solitaire_core/src/klondike_adapter.rs` (remove ~300 lines) +- `solitaire_core/src/game_state.rs` (update `Serialize`/`Deserialize` impls) +- `solitaire_core/src/proptest_tests.rs` (remove now-redundant SavedInstruction tests) +- `solitaire_data/src/storage.rs` (add schema v4 rejection test) +- `solitaire_data/src/replay.rs` (no change — uses `SavedKlondikePile` independently) +- `solitaire_wasm/src/lib.rs` (uses `SavedKlondikePileStack` in its own mirror — evaluate) + +**Steps:** +1. In `game_state.rs`, change `PersistedGameState.saved_moves` from + `Vec` to `Vec` (upstream serde now works). +2. Update `GameState::Serialize` to emit `KlondikeInstruction` directly. +3. Update `GameState::Deserialize` to parse `KlondikeInstruction` directly. +4. Increment `GAME_STATE_SCHEMA_VERSION` to 4. +5. In `GameState::Deserialize`, reject schema != 4 with graceful fallback (already + handled by `load_game_state_from` returning `None` on serde error or wrong version). +6. Delete `SavedInstruction`, `SavedDstFoundation`, `SavedDstTableau`, `SavedKlondikePile`, + `SavedKlondikePileStack`, `SavedTableauStack`, `SavedTableau`, `SavedFoundation`, + `SavedSkipCards`, `InvalidSavedInstruction` from `klondike_adapter.rs`. +7. Delete the 20 `From`/`TryFrom` impls. +8. Remove `SavedInstruction` proptest and boundary tests (no longer needed). +9. Add schema v4 round-trip test and v3 rejection test. + +**Note on `solitaire_data::replay.rs`:** +`replay.rs` uses `SavedKlondikePile` independently (for `ReplayMove`). This is a +separate type from the game-state save format and is NOT changed by this phase. +`ReplayMove` has its own schema (`REPLAY_SCHEMA_VERSION`) and can keep using the local +mirror types. + +**Note on `solitaire_wasm/src/lib.rs`:** +Uses `SavedKlondikePileStack` in its own `ReplayMove` mirror. Same as above — separate +type, not affected. + +### Pre-Phase 3 — Undo Field Audit (completed 2026-06-08) + +Full audit of every Ferrous-owned field in `GameState` for undo correctness. + +| Field | Correctly updated by `undo()`? | Notes | +|---|---|---| +| `score` | ✅ By design | −15 WXP undo penalty applied; Zen: stays 0 | +| `move_count` | ✅ Correct | Recomputed from `session.history().len()` | +| `is_won` | ✅ Correct | Recomputed; undo blocked on won game | +| `is_auto_completable` | ✅ Correct | Recomputed | +| `undo_count` | ✅ By design | Total undos ever, intentionally non-reversible | +| `elapsed_seconds` | ✅ Intentional | Timer is independent of moves | +| `seed` / `draw_mode` / `mode` / `take_from_foundation` | ✅ Immutable | | +| **`recycle_count`** | ❌ **Bug** | Not decremented — see below | + +**`recycle_count` drift bug:** + +`draw()` increments `recycle_count` when `stock.face_down().is_empty()` (the rotation +is a recycle, not just a draw). `undo()` calls `session.undo()` which restores the +`Klondike` card state, but does NOT decrement `recycle_count`. + +Consequence: if the player recycles, undoes it, then recycles again, `recycle_count` +is `2` instead of `1` — the free-recycle allowance is consumed even though the first +recycle was undone. On Draw-1, the 2nd recycle costs −100; after the undo-and-replay +bug the player pays −100 for what should be their still-free recycle. + +**Score compound effect:** When `undo()` is applied to a recycle that incurred a +penalty, the penalty amount (`score_after_recycle - 100`) is already in `self.score`. +`apply_undo_score` then adds `−15` on top. The recycle penalty is never reversed. + +**Fix approach for Phase 3:** +- After `session.undo()`, recompute `recycle_count` by scanning the new + `session.history()` for `RotateStock` snapshots where + `snapshot.state().state().stock().face_down().is_empty()` (indicating the rotation + was a recycle, not a draw from a populated stock). +- Restore `score` to `snapshot_score` **before** the undone move, then apply only + the −15 undo penalty. This requires reading the score stored in `StateSnapshot` + or keeping a pre-move score stack alongside the session history. + +**Simpler alternative:** Store `(score_before, recycle_count_before)` in `GameState` +alongside each `session.process_instruction` call, mirroring the snapshot stack. +Undo pops this alongside the session undo. + +### Phase 3 — Fix `recycle_count` drift on undo (optional, post-approval) + +Files: `solitaire_core/src/game_state.rs` + +After `session.undo()`, recompute `recycle_count` by scanning `session.history()` for +`RotateStock` snapshots where the pre-instruction stock face-down was empty (indicating +a recycle). Also correct the score: restore to the pre-undone-move score and apply only +the −15 undo penalty. + +**Tests to add:** +- `recycle_count_decrements_when_recycle_is_undone` +- `score_recycle_penalty_is_reversed_on_undo` + +**Risk:** Medium — changes observable scoring behavior. The fix is strictly more +correct, but any golden-file or regression test that recorded the old (buggy) score +after undo-of-recycle will need updating. + +--- + +## 6. Files Likely to Change Per Phase + +| Phase | Files | +|---|---| +| Phase 0 | `docs/card-game-integration.md` | +| Phase 1 | `solitaire_core/src/game_state.rs` | +| Phase 2 | `solitaire_core/src/klondike_adapter.rs`, `solitaire_core/src/game_state.rs`, `solitaire_core/src/proptest_tests.rs`, `solitaire_data/src/storage.rs` | +| Phase 3 | `solitaire_core/src/game_state.rs`, new test module | + +--- + +## 7. Risks + +### R1 — Save file format break (Phase 2, HIGH) +Users with v3 saves lose their in-progress game. Mitigated by the fact that v3 is +not in any shipped release (dev branch only). Graceful fallback (start fresh) is +acceptable; a migration shim is possible but not required. + +### R2 — `solitaire_wasm` / `solitaire_data::replay` breakage (Phase 2, MEDIUM) +`SavedKlondikePile` and `SavedKlondikePileStack` are also used in `replay.rs` and +`wasm/src/lib.rs`. These are separate from the game-state save format and must be +left in place. Plan is to keep them in `klondike_adapter.rs` (or relocate to +`replay.rs`) after the game-state mirror types are deleted. + +### R3 — `check_auto_complete` semantic drift (Phase 1, LOW) +Upstream `is_win_trivial` checks `stock.is_empty()` (no cards at all in stock) +whereas Ferrous also checks waste. These are equivalent for a valid game state but +could differ under test-support pile overrides. Existing auto-complete tests will +catch any regression. + +### R4 — `SkipCards as usize` cast correctness +Already verified: enums have implicit 0..12 discriminants. No risk. + +### R5 — Upstream changes after rev pin +The workspace is pinned to `rev = "99b49e62"`. No upstream drift risk until explicitly +re-pinned. + +--- + +## 8. Test Plan + +### Phase 1 tests (all currently pass) +- `game_state::tests::take_from_foundation_allows_legal_return_move` +- `game_state::tests::take_from_foundation_disabled_blocks_return_move_everywhere` +- `proptest_tests::*` (card conservation, deal determinism, undo invariant, legal moves) + +### Phase 2 tests to add +- `storage::tests::game_state_v4_mid_game_round_trip` — verify upstream serde round-trip + after migrating to `KlondikeInstruction` directly +- `storage::tests::save_format_v3_is_rejected` — v3 files must return `None` +- Update `game_state::tests::*` — all existing tests must continue to pass + +### Phase 2 tests to remove +- `proptest_tests::saved_instruction_round_trip` — no longer needed (no mirror types) +- `proptest_tests::saved_instruction_boundary_tests::*` — no longer needed + +### Phase 3 tests to add +- `game_state::tests::recycle_count_decrements_on_undo` — after recycling and undoing, + `recycle_count` must reflect the correct post-undo count + +--- + +## 9. Validation Commands + +Run after each phase: + +```bash +# Targeted (fast) +cargo test -p solitaire_core +cargo clippy -p solitaire_core -- -D warnings + +# Broader +cargo test -p solitaire_wasm +cargo test -p solitaire_data + +# Full workspace (run before declaring phase complete) +cargo test --workspace +cargo clippy --workspace -- -D warnings +``` + +--- + +## Summary: What Would Be Removed vs Kept + +### Removed after all phases complete +| Code | Lines est. | Reason | +|---|---|---| +| `SavedInstruction` + 8 mirror types | ~150 | Upstream serde now available | +| 20 `From`/`TryFrom` impls | ~150 | Upstream serde now available | +| `InvalidSavedInstruction` error type | ~10 | Upstream serde now available | +| `check_win()` local impl | ~20 | Replaced by `is_win()` delegation | +| `check_auto_complete()` local impl | ~15 | Replaced by `is_win_trivial()` delegation | +| `SavedInstruction` proptest + boundary tests | ~60 | Mirror types removed | + +**Total: ~400 lines removed from `solitaire_core`** + +### Remains Ferrous-specific +- `KlondikeAdapter` scoring helpers (recycle penalties, score floor, time bonus, Zen/mode suppression) +- `DrawMode`, `GameMode`, `DifficultyLevel` +- `MoveError` and all boundary-checking logic +- `card::Card` (id + face_up projection) +- `Pile` DTO +- `stock_cards()` / `waste_cards()` projections +- Persistence format (`GameState` serde, schema version, `PersistedGameState`) +- `solitaire_data::replay` types (`ReplayMove`, `SavedKlondikePile` mirror — unchanged) +- `solitaire_wasm` replay mirror types (unchanged) diff --git a/solitaire_core/src/klondike_adapter.rs b/solitaire_core/src/klondike_adapter.rs index fed263e..a37eb73 100644 --- a/solitaire_core/src/klondike_adapter.rs +++ b/solitaire_core/src/klondike_adapter.rs @@ -223,17 +223,24 @@ pub fn card_from_kl(kl_card: &KlCard) -> card::Card { } } -// ── Serde newtypes for KlondikeInstruction (Step 7) ────────────────────────── +// ── Legacy serde mirror types (kept for backward compatibility) ─────────────── // -// `klondike::KlondikeInstruction` (and its sub-types) do not derive -// `Serialize` / `Deserialize`. These mirror types carry `#[serde]` so that -// the session instruction history can be persisted and reconstructed without -// upstream changes. +// These types were introduced when upstream `klondike` had no serde feature. +// At rev 99b49e62, upstream provides full serde support, and `GameState` +// serialises `saved_moves` directly as `Vec` (schema v4). // -// Conversion: `From for SavedInstruction` and the -// fallible inverse `TryFrom for KlondikeInstruction`. -// Invalid numeric values (out-of-range u8 for tableau/foundation/skip) yield -// `InvalidSavedInstruction`. +// The mirror types are retained for three reasons: +// 1. Schema v3 migration: `AnyInstruction` in `game_state.rs` uses +// `TryFrom for KlondikeInstruction` to parse old save +// files with u8 indices and replay them. +// 2. `solitaire_data::ReplayMove` uses `SavedKlondikePile` as its serde +// type; changing it would break the on-disk replay format (schema v2). +// 3. `solitaire_wasm` mirrors `ReplayMove` using the same types so that +// replay JSON is cross-compatible between the desktop and browser builds. +// +// These types should not be used for new serialisation concerns. If the +// ReplayMove format is ever bumped to a new schema, migrate those callers to +// `KlondikePile` / `KlondikePileStack` and the types here can then be deleted. /// A `Serialize` + `Deserialize` mirror of [`klondike::Tableau`] (0 = Tableau1 … 6 = Tableau7). #[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)] diff --git a/solitaire_core/src/proptest_tests.rs b/solitaire_core/src/proptest_tests.rs index c798db0..cb13827 100644 --- a/solitaire_core/src/proptest_tests.rs +++ b/solitaire_core/src/proptest_tests.rs @@ -1,3 +1,4 @@ +use card_game::Game; use klondike::{Foundation, KlondikePile, KlondikeInstruction, SkipCards, Tableau}; use proptest::prelude::*; @@ -107,6 +108,52 @@ fn apply_one_move(game: &mut GameState, move_idx: usize) -> bool { // --------------------------------------------------------------------------- proptest! { + /// `check_auto_complete()` and `is_win_trivial()` must agree on every + /// reachable game state. + /// + /// The upstream `Klondike::is_win_trivial()` checks that the stock pile + /// (both face-down and face-up halves) is completely empty AND that all + /// tableau columns have no face-down cards. Ferrous `check_auto_complete()` + /// checks the same three conditions individually (stock empty, waste empty, + /// all tableau cards face-up). This property guards against any semantic + /// drift between the two implementations so that delegating to upstream is + /// safe. + /// + /// If this property ever fails, `check_auto_complete()` must NOT be fully + /// replaced — the Ferrous conditions must be preserved and `is_win_trivial()` + /// used only as a supplementary guard. + #[test] + fn check_auto_complete_agrees_with_is_win_trivial( + seed in any::(), + draw_mode in draw_mode_strategy(), + actions in prop::collection::vec((any::(), 0usize..200), 0..30), + ) { + let mut game = GameState::new(seed, draw_mode); + apply_random_actions(&mut game, &actions); + prop_assert_eq!( + game.check_auto_complete(), + game.session().state().state().is_win_trivial(), + "check_auto_complete() disagreed with is_win_trivial() after {:?} actions", + actions.len(), + ); + } + + /// `check_win()` and `is_win()` must agree on every reachable game state. + #[test] + fn check_win_agrees_with_is_win( + seed in any::(), + draw_mode in draw_mode_strategy(), + actions in prop::collection::vec((any::(), 0usize..200), 0..30), + ) { + let mut game = GameState::new(seed, draw_mode); + apply_random_actions(&mut game, &actions); + prop_assert_eq!( + game.check_win(), + game.session().state().state().is_win(), + "check_win() disagreed with is_win()", + ); + } + /// All 52 card IDs must be present exactly once across every pile after /// any reachable sequence of draw + move_cards actions. /// diff --git a/solitaire_data/src/storage.rs b/solitaire_data/src/storage.rs index eede94c..8383ea4 100644 --- a/solitaire_data/src/storage.rs +++ b/solitaire_data/src/storage.rs @@ -495,22 +495,24 @@ mod tests { assert_eq!(loaded, StatsSnapshot::default()); } - /// Schema v3 serialises the instruction history, not raw pile state. The - /// deserialiser replays all `saved_moves` to reconstruct every pile. - /// A fresh-game test (zero moves) never exercises that replay path, so - /// this test plays several real moves — including an undo — before - /// saving, then asserts the full pile layout round-trips exactly. + /// Schema v4 serialises the instruction history using upstream + /// `KlondikeInstruction` serde (named enum variants). The deserialiser + /// replays all `saved_moves` to reconstruct every pile. + /// + /// A fresh-game test (zero moves) never exercises that replay path, so this + /// test plays several real moves — including an undo — before saving, then + /// asserts the full pile layout round-trips exactly. /// /// `GameState::PartialEq` covers stock, waste, all four foundations, all /// seven tableau columns, `score`, `move_count`, `undo_count`, and - /// `recycle_count`. Any breakage in `From` or - /// `TryFrom` will cause at least one pile to disagree. + /// `recycle_count`. Any breakage in the upstream serde or replay path + /// will cause at least one pile to disagree. #[test] - fn game_state_v3_mid_game_round_trip() { + fn game_state_v4_mid_game_round_trip() { use solitaire_core::KlondikePile; - use solitaire_core::game_state::{DrawMode, GameState}; + use solitaire_core::game_state::{DrawMode, GameState, GAME_STATE_SCHEMA_VERSION}; - let path = gs_path("v3_mid_game"); + let path = gs_path("v4_mid_game"); let _ = fs::remove_file(&path); let mut gs = GameState::new(42, DrawMode::DrawOne); @@ -538,23 +540,77 @@ mod tests { let _ = gs.undo(); } - // The instruction history must be non-empty for this test to exercise - // the schema-v3 replay path at all. Seed 42 + 6 draws always succeeds. assert!( gs.undo_stack_len() > 0, "instruction history must be non-empty (seed 42 always produces draws)", ); save_game_state_to(&path, &gs).expect("save"); + + // Verify the file contains the v4 schema marker (tolerates pretty-print whitespace). + let json = fs::read_to_string(&path).expect("read json"); + assert!( + json.contains("schema_version") && json.contains('4') && !json.contains(": 3"), + "saved file must use schema version 4", + ); + let loaded = load_game_state_from(&path) .expect("a valid in-progress game must load without error"); + assert_eq!(loaded.schema_version, GAME_STATE_SCHEMA_VERSION); assert_eq!( loaded, gs, - "all pile layouts and counters must be identical after schema-v3 round-trip", + "all pile layouts and counters must be identical after schema-v4 round-trip", ); } + /// A schema v3 save (instruction history using u8 indices) must load + /// successfully and be transparently migrated to schema v4. + /// + /// This verifies the `AnyInstruction` untagged deserialization migration + /// path. v3 files with `RotateStock` (unit variant, format-identical in + /// v3 and v4) load correctly and report `schema_version == 4` after load. + /// The `SavedInstruction` boundary tests in `proptest_tests.rs` cover the + /// u8-to-named conversion for `DstFoundation` / `DstTableau` indices. + #[test] + fn game_state_v3_migrates_to_v4() { + use solitaire_core::game_state::{DrawMode, GameState, GAME_STATE_SCHEMA_VERSION}; + + let path = gs_path("v3_migrate"); + let _ = fs::remove_file(&path); + + // Hand-crafted schema v3 JSON: one RotateStock (draw) instruction. + // RotateStock serialises as the string "RotateStock" in both v3 and v4, + // so this exercises the schema version acceptance code path. + let v3_json = r#"{ + "draw_mode": "DrawOne", + "mode": "Classic", + "score": 0, + "elapsed_seconds": 0, + "seed": 42, + "undo_count": 0, + "recycle_count": 0, + "take_from_foundation": true, + "schema_version": 3, + "saved_moves": ["RotateStock"] + }"#; + fs::write(&path, v3_json).expect("write v3 fixture"); + + let loaded = load_game_state_from(&path) + .expect("schema v3 must be accepted and migrated to v4"); + + // After migration, the in-memory schema version must be current. + assert_eq!( + loaded.schema_version, GAME_STATE_SCHEMA_VERSION, + "migrated game must report current schema version", + ); + + // The loaded game should match a fresh game that had one draw applied. + let mut expected = GameState::new(42, DrawMode::DrawOne); + expected.draw().expect("draw must succeed on a fresh game"); + assert_eq!(loaded, expected, "migrated v3 game state must match equivalent v4 state"); + } + /// Schema v2 stored raw pile arrays and undo snapshots (no instruction /// history). Any file claiming `schema_version: 2` must be rejected so /// players upgrading from an older build start with a fresh game rather