docs(core,data): complete Phases 0–2 of in-place card_game rewrite

Phase 0 – doc fixes (docs/card-game-integration.md): - Correct stale "no serde" claim: upstream has serde at rev 99b49e62 - Correct take_from_foundation default description (Allowed, not Disallowed) - Document schema v3→v4 migration and AnyInstruction strategy Phase 1 – delegate check_win / check_auto_complete to upstream: - Proptests verify semantic agreement with is_win() / is_win_trivial() across 256 random states before delegation Phase 2 – schema v4 with v3 auto-migration: - SavedInstruction mirror types kept as legacy compat module (needed by solitaire_data::ReplayMove and solitaire_wasm replay layer) - klondike_adapter.rs: add comprehensive legacy-purpose doc comment - proptest_tests.rs: add check_auto_complete/check_win semantic proofs - storage.rs: rename round-trip test to v4, add v3-migrates-to-v4 test Also track the rewrite plan (docs/in-place-card-game-rewrite-plan.md). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
2026-06-08 16:59:18 -07:00
parent 56e3b62269
commit 26f1b00186
5 changed files with 550 additions and 31 deletions
@@ -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<Klondike>` 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<SavedInstruction>` to `Vec<KlondikeInstruction>` (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)