cr733(p2): journal draw bookkeeping as a ledger edit (composes zone-change hub)

[matthewevans]

Summary by CodeRabbit

• Bug Fixes

  • Improved draw handling when a player’s library is empty.
  • Ensured draw counters, first-draw effects, and drawn-card records remain accurate during normal and replayed game resolution.
  • Added stronger validation to prevent inconsistent draw results or invalid card references.

• Tests

  • Added integration coverage confirming replayed draws produce the same cards, counters, effects, and zone changes as the original resolution.

[coderabbitai]

📝 Walkthrough

Walkthrough

Draw bookkeeping is moved into resolved ledger edits with explicit replay validation. Draw effects use the ledger for card and empty-library accounting, while integration tests verify serialized command replay, object identity, counters, facts, hand contents, and library order.

Changes

Resolved draw bookkeeping

Layer / File(s)	Summary
Draw command contract and validation crates/engine/src/types/resolved_commands.rs	CardsDrawn now carries explicit expected and resulting draw state, with validation and replay errors for mismatched objects, preconditions, and library membership.
Ledger draw settlement crates/engine/src/game/ledger.rs, crates/engine/src/types/resolved_commands.rs	Ledger helpers create and apply CardsDrawn edits, update draw state, record drawn cards, and maintain first-draw facts.
Draw effect integration and replay coverage crates/engine/src/game/effects/draw.rs, crates/engine/tests/integration/cr733_resolved_draw.rs, crates/engine/tests/integration/main.rs	Draw resolution delegates bookkeeping to the ledger, and integration tests verify command pairing, serialization, replay, and resulting state.

Estimated code review effort: 4 (Complex) | ~45 minutes

Possibly related PRs

• phase-rs/phase#6331: Introduces the resolved-commands provenance infrastructure extended here for draw bookkeeping.
• phase-rs/phase#6521: Adds related resolved ledger edit replay machinery used by the new CardsDrawn path.

Suggested labels: bug

Suggested reviewers: andriypolanski, lgray, rykerwilliams

Sequence Diagram(s)

sequenceDiagram
  participant DrawEffect
  participant DrawLedger
  participant ResolvedCommandApplier
  participant GameState
  DrawEffect->>DrawLedger: Resolve card or empty-library draw
  DrawLedger->>ResolvedCommandApplier: Apply CardsDrawn edit
  ResolvedCommandApplier->>GameState: Update counters, flags, cards, and first-draw fact
  GameState-->>DrawEffect: Updated draw state
  DrawEffect-->>DrawEffect: Emit CardDrawn and enqueue first-draw offer when applicable

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: draw bookkeeping is now journaled as a ledger edit.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch ship/cr733-draw-ledger

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

🧹 Nitpick comments (2)

crates/engine/tests/integration/cr733_resolved_draw.rs (2)

73-136: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Replay coverage omits the empty-library / partial-draw ledger path.

This test only replays a fully-settled 2-card draw. The PR's empty-library accounting — a CardsDrawn edit with drawn_object: None and resulting_drew_from_empty_library, plus the DrawnObjectStillInLibrary/DrawnObjectMismatch guards — is never driven through the journal/replay comparison here. A regression that broke empty-library or partial-draw replay would stay green.

Consider a sibling case: seed fewer library cards than the draw count, resolve, then replay and assert drew_from_empty_library and the drawn-card ledger match. Want me to draft it?

As per path instructions: "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline."

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/tests/integration/cr733_resolved_draw.rs` around lines 73 -
136, Extend the integration coverage beside
divination_draw_replays_zone_changes_and_ledger_bookkeeping_exactly with a
partial draw scenario whose library contains fewer cards than Divination draws.
Resolve and replay it through the production pipeline, then compare the journals
and assert the CardsDrawn edit for the empty-library attempt uses drawn_object:
None with resulting_drew_from_empty_library, while preserving the existing
drawn-object and guard behavior for the card that was drawn.

Source: Path instructions

---

27-66: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Keep the shuffle action in this replay helper
LibraryShuffle is the only branch that drops the production side effect by passing &mut Vec::new(). If shuffle replay events matter here, thread a real sink through this helper so the test exercises the same outcome as the resolver.

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/tests/integration/cr733_resolved_draw.rs` around lines 27 - 66,
Update apply_semantic_command to accept a mutable shuffle-event sink and pass
that sink to apply_resolved_library_shuffle instead of creating a temporary
Vec::new(). Thread the same sink through every caller so LibraryShuffle replay
preserves the production side effect.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@crates/engine/tests/integration/cr733_resolved_draw.rs`:
- Around line 73-136: Extend the integration coverage beside
divination_draw_replays_zone_changes_and_ledger_bookkeeping_exactly with a
partial draw scenario whose library contains fewer cards than Divination draws.
Resolve and replay it through the production pipeline, then compare the journals
and assert the CardsDrawn edit for the empty-library attempt uses drawn_object:
None with resulting_drew_from_empty_library, while preserving the existing
drawn-object and guard behavior for the card that was drawn.
- Around line 27-66: Update apply_semantic_command to accept a mutable
shuffle-event sink and pass that sink to apply_resolved_library_shuffle instead
of creating a temporary Vec::new(). Thread the same sink through every caller so
LibraryShuffle replay preserves the production side effect.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 45196b84-dc78-4e47-a08e-4a115719e480

📥 Commits

Reviewing files that changed from the base of the PR and between 8985e8c and 5595561.

📒 Files selected for processing (5)

• crates/engine/src/game/effects/draw.rs
• crates/engine/src/game/ledger.rs
• crates/engine/src/types/resolved_commands.rs
• crates/engine/tests/integration/cr733_resolved_draw.rs
• crates/engine/tests/integration/main.rs

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.