Skip to content

fix(parser): bind paid-energy resolution thresholds - #7312

Merged
matthewevans merged 3 commits into
mainfrom
ship/fix-parser-bind-paid-energy-resolution-thresholds
Aug 12, 2026
Merged

fix(parser): bind paid-energy resolution thresholds#7312
matthewevans merged 3 commits into
mainfrom
ship/fix-parser-bind-paid-energy-resolution-thresholds

Conversation

@matthewevans

@matthewevans matthewevans commented Aug 12, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • New Features

    • Added support for interpreting “the amount of {e} paid this way” and “amount of {e} paid this way” in game effects.
  • Bug Fixes

    • Corrected Wrath of the Skies behavior so its destruction threshold uses the energy actually paid during resolution, rather than the announced value.

@matthewevans
matthewevans enabled auto-merge August 12, 2026 20:59
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Warning

Review limit reached

@matthewevans, you've reached your PR review limit, so we couldn't start this review.

Next review available in: 18 minutes

You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository.

How can I continue?

After more reviews become available, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews.

How do review limits work?

CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability.

For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window.

Please refer docs for additional details.

Review details
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0f9f793f-5e02-4fbe-b284-3a7eabdfdce7

📥 Commits

Reviewing files that changed from the base of the PR and between 61c3fc0 and ebcafa3.

📒 Files selected for processing (1)
  • crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0c4bdc89-8044-4aba-ac7a-c3a52d4f64bf

📥 Commits

Reviewing files that changed from the base of the PR and between b32294f and 61c3fc0.

📒 Files selected for processing (2)
  • crates/engine/src/parser/oracle_nom/quantity.rs
  • crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/engine/src/parser/oracle_nom/quantity.rs
  • crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs

📝 Walkthrough

Walkthrough

The quantity parser now handles paid-energy amount phrases. A Wrath of the Skies integration test verifies that the effect uses energy paid during resolution instead of the announced X value.

Changes

Paid Energy Quantity Handling

Layer / File(s) Summary
Paid-energy quantity parsing
crates/engine/src/parser/oracle_nom/quantity.rs
The parser recognizes both article and article-less paid-energy phrases and emits QuantityRef::EventContextAmount. Tests verify complete parsing.
Wrath of the Skies regression coverage
crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs, crates/engine/tests/integration/main.rs
The integration test casts Wrath of the Skies with X=2, pays zero energy, and verifies the destruction threshold. The test module is registered in the integration test suite.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Possibly related PRs

  • phase-rs/phase#6698: Adds related event-context quantity parsing in oracle_nom/quantity.rs.
  • phase-rs/phase#7047: Uses QuantityRef::EventContextAmount for amount-based card effects.
  • phase-rs/phase#7306: Adds and registers an integration regression test for a different gameplay mechanic.

Suggested labels: bug

🚥 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 clearly summarizes the parser fix that binds paid-energy resolution thresholds, matching the main changes and regression test.
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/fix-parser-bind-paid-energy-resolution-thresholds

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

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.

Actionable comments posted: 2

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

Inline comments:
In `@crates/engine/src/parser/oracle_nom/quantity.rs`:
- Around line 831-833: Add a verified “CR 608.2c” annotation to the
documentation at crates/engine/src/parser/oracle_nom/quantity.rs lines 831-833,
explaining that written resolution order causes “paid this way” to read the
immediately preceding resolution-time payment; add the same verified CR
reference and description at
crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs lines 1-2 to
document the regression behavior.

In `@crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs`:
- Around line 31-37: Update the regression test around thought_knot_seer and the
corresponding battlefield assertions so the affected creature has mana value 1
or 2, announces X as 2, and survives after paying 0 energy. Preserve the
mana-value-0 artifact assertion as the destruction guard, ensuring the test
distinguishes announced X from paid energy.
🪄 Autofix

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 55db683e-950e-4f6b-86eb-f1d087d3cee7

📥 Commits

Reviewing files that changed from the base of the PR and between 479ad39 and b32294f.

📒 Files selected for processing (3)
  • crates/engine/src/parser/oracle_nom/quantity.rs
  • crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs
  • crates/engine/tests/integration/main.rs

Comment thread crates/engine/src/parser/oracle_nom/quantity.rs Outdated
Comment thread crates/engine/tests/integration/issue_6473_wrath_of_the_skies.rs Outdated
@github-actions

github-actions Bot commented Aug 12, 2026

Copy link
Copy Markdown

Generated for head ebcafa38762ea49bb84764761e3ab0c24668aa56.

Parse changes introduced by this PR · 3 card(s), 4 signature(s) (baseline: main 479ad396d463)

🟢 Added (2 signatures)

  • 1 card · ➕ ability/Token · added: Token (conditional=previous effect outcome, token=+event amount/+event amount Black Aetherborn (Creature Aetherborn))
    • Affected (first 3): Aether Refinery
  • 1 card · ➕ ability/Token · added: Token (conditional=previous effect outcome, token=Nalaar Aetherjet (Artifact Vehicle) with Flying, Crew)
    • Affected (first 3): Pia Nalaar, Chief Mechanic

🔴 Removed (1 signature)

  • 2 cards · ➖ ability/create · removed: create (conditional=previous effect outcome)
    • Affected (first 3): Aether Refinery, Pia Nalaar, Chief Mechanic

🟡 Modified fields (1 signature)

  • 1 card · 🔄 ability/DestroyAll · changed field filter: artifact or creature or enchantmentmv event amount- artifact or mv event amount- creature or mv event amount- enchantment
    • Affected (first 3): Wrath of the Skies

@matthewevans
matthewevans added this pull request to the merge queue Aug 12, 2026
@github-merge-queue
github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Aug 12, 2026
@matthewevans
matthewevans added this pull request to the merge queue Aug 12, 2026
Merged via the queue into main with commit 1edf14c Aug 12, 2026
15 checks passed
@matthewevans
matthewevans deleted the ship/fix-parser-bind-paid-energy-resolution-thresholds branch August 12, 2026 22:39
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