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:

// before
pub fn check_win(&self) -> bool { ... 40 lines ... }

// after  
pub fn check_win(&self) -> bool {
    self.session.state().state().is_win()
}

// 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:

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