Skip to content

ship/cr733 p2 mana appliers - #6336

Merged
matthewevans merged 3 commits into
mainfrom
ship/cr733-p2-mana-appliers
Jul 22, 2026
Merged

ship/cr733 p2 mana appliers#6336
matthewevans merged 3 commits into
mainfrom
ship/cr733-p2-mana-appliers

Conversation

@matthewevans

@matthewevans matthewevans commented Jul 22, 2026

Copy link
Copy Markdown
Member
  • cr733(p2): apply resolved mana commands
  • cr733(p2): record mana command run report
  • cr733(p2): debug-format ManaPipId in ExactManaRemovalError messages

Summary by CodeRabbit

  • New Features

    • Added deterministic journaling for mana entering and leaving players’ mana pools.
    • Preserved exact mana identities and payment details during replay.
    • Added validation to detect invalid, duplicate, missing, or mismatched mana records.
    • Maintained compatibility with existing journal entries and replay behavior.
  • Bug Fixes

    • Prevented partial mana removal when a payment cannot be applied exactly.
    • Improved handling when a mana pool changes before payment is finalized.
  • Tests

    • Added coverage for replay consistency, exact mana removal, journal serialization, and invalid command rejection.
  • Documentation

    • Added a report documenting verification results and replay guidance.

@matthewevans
matthewevans enabled auto-merge July 22, 2026 12:35
@coderabbitai

coderabbitai Bot commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

The PR adds semantic resolved mana insert/spend commands to the journal, validates their provenance, applies exact mana units atomically, updates payment paths to use the new flow, and adds replay and invariant tests using a Dimir Signet scenario.

Changes

Resolved mana command replay

Layer / File(s) Summary
Command payloads and journal validation
crates/engine/src/types/resolved_commands.rs, crates/engine/src/types/mod.rs
Adds mana insert/spend command types, optional journal payloads, recording APIs, provenance validation, re-exports, and serialization tests.
Exact mana application
crates/engine/src/game/mana_payment.rs, crates/engine/src/types/game_state.rs
Adds atomic exact removal and replay-aware mana insertion/spend methods with typed invariant errors.
Payment-path integration
crates/engine/src/game/casting.rs, crates/engine/src/game/casting_costs.rs, crates/engine/src/game/mana_abilities.rs
Payment paths select units without mutating pools, then journal and apply exact spends through GameState.
Replay integration coverage
crates/engine/tests/integration/*, .agents/cr733/RUN5-REPORT.md
Adds Dimir Signet replay tests, repeated-spend rejection coverage, and the Run 5 report.

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

Sequence Diagram(s)

sequenceDiagram
  participant CastingPath
  participant GameState
  participant ResolvedRulesJournal
  participant ManaPool
  CastingPath->>GameState: select exact mana units
  GameState->>ResolvedRulesJournal: record_mana_spend
  GameState->>ManaPool: apply_resolved_mana_spend
  ManaPool-->>GameState: remove exact pip units
Loading

Possibly related PRs

  • phase-rs/phase#6331: Introduces the related CR733 mana provenance and resolved-rules journal foundation.

Suggested labels: enhancement

Suggested reviewers: kiannidev, andriypolanski, dripsmvcp

🚥 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 is concise and matches the main change: CR733 phase 2 mana appliers.
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-p2-mana-appliers

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

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/engine/src/game/mana_abilities.rs (1)

3258-3275: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Honor life-payment alternatives for mana-ability costs. Both select_mana_payment calls hard-code LifePaymentColors::EMPTY, so mana-ability sub-costs can never use K'rrik-style 2-life substitution. Thread the active life-payment grant through here, and apply the returned Vec<LifePayment> instead of discarding it.

🤖 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/src/game/mana_abilities.rs` around lines 3258 - 3275, The two
select_mana_payment call sites in mana_abilities.rs, at lines 3258-3275 and
3363-3382, must thread the active LifePaymentColors grant instead of passing
LifePaymentColors::EMPTY, then apply the returned Vec<LifePayment> rather than
discarding it. Preserve the existing restriction-aware payment context and
ensure both mana-ability sub-cost paths honor K’rrik-style life substitution.

Sources: Coding guidelines, Path instructions

🤖 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.

Outside diff comments:
In `@crates/engine/src/game/mana_abilities.rs`:
- Around line 3258-3275: The two select_mana_payment call sites in
mana_abilities.rs, at lines 3258-3275 and 3363-3382, must thread the active
LifePaymentColors grant instead of passing LifePaymentColors::EMPTY, then apply
the returned Vec<LifePayment> rather than discarding it. Preserve the existing
restriction-aware payment context and ensure both mana-ability sub-cost paths
honor K’rrik-style life substitution.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: d6d985cb-db14-431c-adfc-ccb380578159

📥 Commits

Reviewing files that changed from the base of the PR and between d247cee and 3fc5a9a.

📒 Files selected for processing (10)
  • .agents/cr733/RUN5-REPORT.md
  • crates/engine/src/game/casting.rs
  • crates/engine/src/game/casting_costs.rs
  • crates/engine/src/game/mana_abilities.rs
  • crates/engine/src/game/mana_payment.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/types/mod.rs
  • crates/engine/src/types/resolved_commands.rs
  • crates/engine/tests/integration/cr733_resolved_commands_p2.rs
  • crates/engine/tests/integration/main.rs

@matthewevans
matthewevans added this pull request to the merge queue Jul 22, 2026
@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

Merged via the queue into main with commit 7592ea2 Jul 22, 2026
15 checks passed
@matthewevans
matthewevans deleted the ship/cr733-p2-mana-appliers branch July 22, 2026 13:07
jsdevninja pushed a commit to jsdevninja/phase that referenced this pull request Jul 24, 2026
* cr733(p2): apply resolved mana commands

* cr733(p2): record mana command run report

* cr733(p2): debug-format ManaPipId in ExactManaRemovalError messages

---------

Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant