Ferrous-Solitaire/docs/in-place-card-game-rewrite-plan.md

# In-Place card_game / klondike Rewrite Plan

**Date:** 2026-06-08  
**Upstream rev:** `99b49e62`  
**Status:** All phases complete (0–3). recycle_count drift and score compound error on undo fixed in `56e3b62`.

---

## 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)