feat(engine): apply the CR 605.1a library-movement criterion by matthewevans · Pull Request #7113 · phase-rs/phase

matthewevans · 2026-08-08T23:54:56Z

What

CR 605.1a was amended to add a fourth criterion to the mana-ability test:

An activated ability is a mana ability if it meets all of the following criteria: it doesn't require a target (see rule 115.6), it could add mana to a player's mana pool when it resolves, it's not a loyalty ability (see rule 606, "Loyalty Abilities"), and its cost and effect don't move any card to or from a library. Do not take into account replacement effects that may apply, other than self-replacement effects, when evaluating these criteria.

Two deltas versus the prior printing: the library-movement criterion, and the trailing replacement-effect clause. (The amended text is already present in docs/MagicCompRules.txt.)

How

Effect::moves_card_to_or_from_library() / AbilityCost::moves_card_to_or_from_library() — exhaustive, wildcard-free per-node predicates over all 229 Effect and 33 AbilityCost variants. Every reasoned verdict carries its CR annotation inline. No wildcard arm, so a future variant cannot silently inherit false.
ResolutionScope { OwnResolutionOnly, IncludeRegisteredLater } in types/ability_visit.rs. OwnResolutionOnly stops at effects that merely register a separate ability to apply later — delayed triggers (CR 603.7a), reflexive triggers (CR 603.12), registered replacements (CR 614.1), and Effect::Mana's grants (CR 603.3). All 8 existing public entry points keep IncludeRegisteredLater, so no current caller changes behavior.
is_mana_ability is now the single authority for all four criteria. The three-criteria core is extracted as produces_mana_on_activation so that is_renewable_mana_ability — which asks a different question, "is this permanent part of a standing manabase?" — does not narrow. Composing the development predicate on the rules predicate would drop Millikin, Deranged Assistant and Codie out of phase-ai's mulligan keep_tier for a reason unrelated to manabase development.

The replacement-effect clause is satisfied by construction: is_mana_ability is a pure function of the printed AST, takes no &GameState, and therefore cannot consult replacement effects at all. Self-replacement riders printed on the ability (CR 614.15) — e.g. Memory Lapse's "put it on top of its owner's library instead", which is CR 608.2c's own worked example — are counted, which is what the closing carve-out requires.

Near-misses that deliberately stay classified, each for a different CR reason: Chromatic Star (separate ChangesZone trigger), Barbed Sextant (CR 603.7a), Shaun & Rebecca (CR 603.12), Gilanra (CR 603.3), The Secret Lair (CR 701.22a — scry reorders within a library).

Verification

Tilt clippy clean; test-engine 23,355 / 23,356 passing. The single failure (battlefield_entry_authority_census) is another agent's concurrent in-flight work — that test is a source-scanning census over TokenCreated constructions and no path in this PR contains one.
Pre-commit gates pass: parser-combinator Gate A + Gate G, PreLowered Gate P.
Four new integration tests, deliberately asserting no AST-internal flag — they pin only what is observable by driving the real pipeline. Every negative is paired with a positive reach-guard in the same test so a fixture that never reached the seam cannot pass vacuously.
The snapshot corpus acts as an independent cross-check: 16 committed snapshots pin is_mana_ability, the suite exercised all 16, and exactly one moved — Manamorphose, the one the census predicted.
All CR citations were grep-verified against docs/MagicCompRules.txt before being written.

Reviews

Plan reviewed to convergence (5 rounds, review-r8: APPROVED). Implementation independently reviewed: 12 findings, zero behavioral defects — every arm verdict against the CR was correct. All 12 fixed in the second commit, notably:

Effect::Ripple's comment quoted only the reveal and bottom-of-library legs of CR 702.60a, both of which this file classifies as false — so it argued against its own true. The warrant is the casting leg (CR 702.60a + CR 601.2a, library → stack), now cited explicitly.
CR 603.3 was cited as primary authority for the replacement-registration boundary at five sites. CR 603.3 is exclusively about a triggered ability going on the stack; replacement effects never trigger. Swapped to CR 614.1 / CR 614.15.

Known divergences (documented, none pinned as correct by any test)

Codie, Vociferous Codex is reclassified when CR 605.1a says it should not be. Accepted, documented divergence traced to a parser mis-attachment (CastFromZone / PutAtLibraryPosition parsed as chain siblings of the CreateDelayedTrigger rather than as its payload). Codie is already rules-incorrect today for the same root cause — its free-cast continuation resolves immediately instead of when the delayed trigger fires. A follow-up PR fixes the attachment, after which OwnResolutionOnly prunes them and Codie classifies correctly with no further change here.
Pre-existing CR 605.5b violation (is_mana_ability never checks AbilityKind, so spell-kind abilities serialize is_mana_ability: true). This change incidentally flips exactly one of them, Manamorphose — right outcome, wrong criterion. 31 remain misflagged over top-level .abilities[] nodes. Needs its own census.
Chartered follow-up — cost walk does not descend inline branch carriers. visit_effect_scoped descends nested AbilityDefinitions under Vote, SeparateIntoPiles, RevealFromHand.on_decline, FlipCoin/FlipCoins, FlipCoinUntilLose, RollDie.results and ChooseOneOf.branches; the cost walk does not. A library cost on such a branch would go unclassified. Zero reachable instances today (card-data.json carries no costs on effect-payload-nested defs), and the doc claim has been narrowed with an explicit KNOWN GAP naming every carrier. Worth tracking rather than closing, because the trigger is a parser change that compiles silently and fails no existing test.

Summary by CodeRabbit

Bug Fixes
- Correctly excludes activated abilities that move cards to or from a library from mana-ability classification.
- Distinguishes library movement from revealing, reordering, and other non-moving effects.
- Preserves renewable mana-source classification where appropriate.
- Improves handling of nested costs, effects, triggers, replacements, and delayed resolutions.
Tests
- Added comprehensive coverage for library-moving abilities, mana-source filtering, stack behavior, and activation triggers.

CR 605.1a was amended to add a fourth criterion to the mana-ability test: "An activated ability is a mana ability if it meets all of the following criteria: it doesn't require a target (see rule 115.6), it could add mana to a player's mana pool when it resolves, it's not a loyalty ability (see rule 606, "Loyalty Abilities"), and its cost and effect don't move any card to or from a library. Do not take into account replacement effects that may apply, other than self-replacement effects, when evaluating these criteria." Two deltas versus the prior printing: the library-movement criterion, and the trailing replacement-effect clause. What this adds: - `Effect::moves_card_to_or_from_library()` and `AbilityCost::moves_card_to_or_from_library()` — exhaustive, wildcard-free per-node predicates over all 229 `Effect` and 33 `AbilityCost` variants. Every reasoned verdict carries its CR annotation inline. - `ResolutionScope { OwnResolutionOnly, IncludeRegisteredLater }` in `types/ability_visit.rs`, threaded through the walkers. `OwnResolutionOnly` stops at any effect that merely REGISTERS a separate ability, replacement, or continuous effect to apply later — delayed triggers (CR 603.7a), reflexive triggers (CR 603.12), registered replacements (CR 614.1), and `Effect::Mana`'s `grants` (CR 603.3). Existing entry points keep `IncludeRegisteredLater`, so no current caller changes behavior. - `is_mana_ability` is now the single authority for all four criteria. The three-criteria core is extracted as `produces_mana_on_activation` so that `is_renewable_mana_ability` — which asks a different question, "is this permanent part of a standing manabase?" — does not narrow. Composing the development predicate on the rules predicate would drop Millikin, Deranged Assistant, and Codie out of `phase-ai`'s `is_intrinsic_mana_source` -> `card_value::mana_role` -> mulligan `keep_tier`, for a reason unrelated to manabase development. The replacement-effect clause is satisfied by construction: `is_mana_ability` is a pure function of the printed `AbilityDefinition` AST, takes no `&GameState`, and therefore cannot consult replacement effects at all. Self-replacement riders that ARE printed on the ability (CR 614.15) — e.g. `Counter { countered_spell_zone: Some(Library) }`, Memory Lapse's "put it on top of its owner's library instead" — are counted, which is what the closing sentence's carve-out requires. One committed insta snapshot is re-accepted: `manamorphose_lowered.snap` loses its `is_mana_ability: true`. Sixteen committed snapshots pin that field; the suite exercised all sixteen and exactly this one moved, which is an independent confirmation of the census's predicted blast radius (see F-6 for why the new value is the rules-correct one). The plan predicted the classification flip but did not predict that a snapshot pinned it, so this path is a scope addition. Known divergences, none fixed here and none pinned as correct by any test: - F-1 Codie, Vociferous Codex is reclassified and under CR 605.1a should not be. This is an accepted, documented divergence traceable to a parser mis-attachment: `CastFromZone` / `PutAtLibraryPosition` are parsed as chain siblings of the `CreateDelayedTrigger` rather than as its payload. A follow-up PR fixes that attachment, after which `OwnResolutionOnly` prunes them and Codie classifies correctly with no further change here. Codie is already rules-incorrect today for the same root cause: its free-cast continuation resolves immediately instead of when the delayed trigger fires. - F-2 `feasible_mana_capacity` / `available_mana` / `strategy_helpers` under-report reachable mana for the affected permanents, because they model mana reachable inside the payment window and these sources now require floating at priority first. Payment-planner change. - F-3 `WhenYouDo` reflexive sub-chains run inline on the off-stack mana path; under CR 603.12 + CR 603.3 they belong on the stack. Pre-existing and unchanged in severity. - F-4 `triggers.rs`'s `AbilityActivated` comment describes a construction-based invariant that is now classification-based. Behavior pinned by test; wording left alone (out of scope). - F-5 The three `ActivationExemption::ManaAbilities` gates are copy-shaped and not deduplicated. - F-6 Pre-existing CR 605.5b violation: `is_mana_ability` never checks `AbilityKind`, so `kind: "Spell"` abilities serialize `is_mana_ability: true`. CR 605.5b: "A spell can never be a mana ability." This change incidentally flips exactly one of them (Manamorphose) for the right outcome by the wrong criterion — its "Draw a card" moves a card from a library, which is the CR 605.1a criterion, not the CR 605.5b one that should have caught it. 31 remain misflagged, counted over top-level `.abilities[]` nodes; a recursive walk including nested sub-abilities counts more (210 nodes / 197 cards), so any follow-up must state its population before quoting a number. Adding an `AbilityKind` guard needs its own census and is deliberately not done here. - F-7 Informational: one of the reclassified cards, Manakin and Millikin, is `set_type: funny` / `Unknown Event` and `not_legal` in every format. It is in `card-data.json` so it counts in the census, but no tournament- relevant behavior rests on it. - F-8 Two shipped `CR 701.58e` citations overstate the rule (it is the multi-cloak ordering rule, not authority for cloak's source zone), at `types/ability.rs` and `game/effects/cloak.rs`. A third at `game/effects/cloak.rs` cites it correctly for the ordering it does state and must NOT be swept up by a cleanup. Verified on Tilt: clippy clean, and test-engine build 7331 ran 23,356 tests with 23,355 passing — the four new integration tests, the re-accepted Manamorphose snapshot, and the other fifteen snapshots that pin `is_mana_ability` all pass. Both verification builds started after the last edit to these paths. One failure remains and is NOT from this change: `battlefield_entry_authority_census::every_token_created_construction_lives_in_the_single_emitter`. That test is a source-scanning census over `TokenCreated` constructions, its file carries another agent's uncommitted in-flight edits, and none of the six paths in this commit contains a `TokenCreated` construction, so this change cannot move it. Left to its owner rather than fixed here.

Addresses all twelve findings from the implementation review of the previous commit. No arm verdict changes and no card is reclassified: eleven are comment corrections on annotations that carry the design's rules argument, and the twelfth reads a field that has no reachable non-default value today. The two that could have caused a future regression: - `Effect::Ripple`'s comment quoted only the reveal and the bottom-of-library legs of CR 702.60a. This file classifies revealing as `false` (CR 701.20b) and bottoming-within-a-library as `false` (the `Scry` arm), so the comment argued for the opposite of its own `true`. The warrant is the CASTING leg — CR 702.60a lets you "cast any of those cards ... without paying their mana costs", which CR 601.2a makes a library -> stack move. Now cited, with an explicit note not to "correct" it by reading only the other two clauses. - CR 603.3 was cited as the PRIMARY authority for the replacement-registration boundary at five sites. CR 603.3 is exclusively about a *triggered* ability going on the stack; `CreateDrawReplacement`, `CreatePlaneswalkReplacement`, `AddTargetReplacement` and `CreateDamageReplacement` create replacement effects, which never trigger and never use the stack. Swapped to CR 614.1 primary / CR 614.15 secondary. CR 603.3 is retained on the `CreateDelayedTrigger` arm, where a delayed triggered ability genuinely does go on the stack later. Also corrected: - `Effect::SearchOutsideGame` now reads its `destination` rather than returning an unconditional `false`. CR 400.11 licenses only the ORIGIN half ("outside the game is not a zone"); a `Zone::Library` destination would be a move *to* a library. All 11 shipping nodes are `Hand`, so no behavior changes. - `Effect::Manifest`'s empirical warrant said "all 8 shipping cards"; there are 24 `"type":"Manifest"` nodes in `data/card-data.json`. The arm's `true` rests entirely on that census, so the number is the whole argument. Verdict unaffected — every one is "manifest the top card of your library". - `Effect::ExileFromTopUntil` no longer cites CR 701.13a for a library origin; 701.13a says only "move it to the exile zone from wherever it is" and names no source zone. Matches the adjacent `ExileTop` arm, which cites nothing. - `Effect::Meld` cites CR 701.42a (puts the pair onto the battlefield) instead of CR 701.42, which never names exile as the source. - `AbilityCost::PaySpeed` cites CR 702.179b (speed is a numeric player value) instead of CR 702.179f, and drops "designation" — a CR term of art reserved for permanent-level markers. - `AbilityCost::KeywordCostOfCastSpell`'s comment asserted a keyword cost "is still a mana cost"; CR 118.9 says an alternative cost is paid "rather than paying the spell's mana cost". Reworded to match the rule and the variant's own doc. - `Effect::Mana`'s `TriggerOnSpend` rider is no longer called "reflexive". Under CR 603.12 a reflexive trigger is checked immediately, within the resolution that created it; this one fires in a later resolution. The sibling comments already said "a separate triggered ability"; this was the outlier. - `mana_abilities.rs` cites CR 605.3b for "resolve immediately without using the stack" instead of the CR 605.3 header, which is only a preamble. - The V14b test no longer describes CR 605.3a as a prohibition. It is a permission ("a player may activate an activated mana ability..."); once an ability stops being a mana ability that exception simply stops covering it and CR 117.1b governs again. And one documented gap, not a code change: - `visit_ability_def_costs_scoped`'s doc claimed it yields every `AbilityCost` "in the same own-resolution tree the effect walk covers". It does not: the effect walk descends nested `AbilityDefinition`s under `Vote`, `SeparateIntoPiles`, `RevealFromHand.on_decline`, `FlipCoin`/`FlipCoins`, `FlipCoinUntilLose`, `RollDie.results` and `ChooseOneOf.branches`; the cost walk does not, and `Effect::PayCost` is reached only by a one-node delegation. The claim is narrowed to the chain-link axis and the gap is named explicitly. `data/card-data.json` carries zero costs on an effect-payload-nested `AbilityDefinition`, so nothing is reachable today. Closing it means driving both walks from one shared carrier list, which widens cost coverage for every existing `IncludeRegisteredLater` caller and needs its own census. Every CR number cited here was verified against docs/MagicCompRules.txt before being written, including the three newly introduced (CR 117.1b, CR 601.2a, CR 701.42a).

coderabbitai · 2026-08-08T23:55:24Z

📝 Walkthrough

Walkthrough

The engine now detects library movement across nested ability costs and effects, excludes those abilities from mana-ability classification, preserves renewable-mana classification, and adds scoped traversal plus comprehensive unit and integration tests.

Changes

Library movement classification

Layer / File(s)	Summary
Classify library movement `crates/engine/src/types/ability.rs`	`AbilityCost` and `Effect` now classify direct and recursive card movement to or from libraries. Reveals, reordering, and unrelated zone changes remain excluded.

Resolution traversal

Layer / File(s)	Summary
Scope resolution traversal `crates/engine/src/types/ability_visit.rs`	`ResolutionScope` limits classification to an ability’s own resolution and excludes separately registered delayed, reflexive, replacement, token, emblem, and granted-ability payloads.

Mana-ability integration

Layer / File(s)	Summary
Apply CR 605.1a classification `crates/engine/src/game/mana_abilities.rs`	`is_mana_ability` now rejects library-moving costs and effects. `is_renewable_mana_ability` continues to use production, targeting, and loyalty criteria without the library check.

Validation and routing

Layer / File(s)	Summary
Validate classification and gameplay routing `crates/engine/src/game/mana_abilities.rs`, `crates/engine/tests/integration/cr605_1a_library_criterion.rs`, `crates/engine/tests/integration/main.rs`	Tests cover recursive classifiers, resolution boundaries, Millikin, Chromatic Sphere, mana-source enumeration, activation triggers, and stack behavior.

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

Sequence Diagram(s)

sequenceDiagram
  participant Ability as Ability definition
  participant Traversal as Scoped ability traversal
  participant Classifier as Mana ability classifier
  participant Engine as Game engine
  Ability->>Traversal: Traverse own resolution
  Traversal->>Classifier: Inspect nested costs and effects
  Classifier->>Classifier: Detect library movement
  Classifier->>Engine: Classify activation and renewable status
  Engine->>Engine: Route actions and activation triggers

Possibly related PRs

phase-rs/phase#6056: Directly relates to mana-ability classification and mana-related type handling.
phase-rs/phase#7099: Relates to scoped traversal and resolution handling in ability_visit.rs and ability.rs.

Suggested labels: enhancement

Suggested reviewers: andriypolanski, lgray, hurryup52

🚥 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 and concisely describes the main change: applying the CR 605.1a library-movement criterion to engine mana-ability classification.
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 cr605-1a-library-criterion

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

Actionable comments posted: 1

🤖 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/types/ability_visit.rs`:
- Around line 211-232: Align visit_ability_def_costs_scoped with
visit_effect_scoped so both traverse the same
carrier-to-nested-AbilityDefinition coverage, including inline effect carriers
such as ChooseOneOf, Vote, and other documented cases. Prefer extracting and
reusing one shared nested-definition traversal; otherwise add a failing
corpus/assertion test that enforces identical coverage. Preserve yielding only
top-level costs and leave composition recursion to the consuming predicate.

🪄 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: 9e06c6a9-3019-4271-adaa-3f4a6c883229

📥 Commits

Reviewing files that changed from the base of the PR and between bbd2445 and f5d8bc3.

⛔ Files ignored due to path filters (1)

crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__manamorphose_lowered.snap is excluded by !**/*.snap, !**/snapshots/**

📒 Files selected for processing (5)

crates/engine/src/game/mana_abilities.rs
crates/engine/src/types/ability.rs
crates/engine/src/types/ability_visit.rs
crates/engine/tests/integration/cr605_1a_library_criterion.rs
crates/engine/tests/integration/main.rs

github-actions · 2026-08-09T00:17:19Z

Generated for head f5d8bc34efb5df4c9bb37eaeee19621a95a50df4.

Parse changes introduced by this PR

✓ No card-parse changes detected.

matthewevans added 2 commits August 8, 2026 16:54

matthewevans enabled auto-merge August 8, 2026 23:55

coderabbitai Bot reviewed Aug 8, 2026

View reviewed changes

Comment thread crates/engine/src/types/ability_visit.rs

matthewevans added this pull request to the merge queue Aug 9, 2026

matthewevans mentioned this pull request Aug 9, 2026

ci(engine): gate against costs on carrier-nested ability definitions #7114

Merged

Merged via the queue into main with commit aecb991 Aug 9, 2026
13 checks passed

matthewevans deleted the cr605-1a-library-criterion branch August 9, 2026 00:40

coderabbitai Bot mentioned this pull request Aug 12, 2026

fix(parser): derive activation zone from a self-move's origin, not its destination #7316

Merged

4 tasks

Navigation Menu

Uh oh!

Uh oh!

feat(engine): apply the CR 605.1a library-movement criterion - #7113

feat(engine): apply the CR 605.1a library-movement criterion#7113
matthewevans merged 2 commits into
mainfrom
cr605-1a-library-criterion

matthewevans commented Aug 8, 2026 •

edited by coderabbitai Bot

Loading

Uh oh!

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

matthewevans commented Aug 8, 2026 • edited by coderabbitai Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

What

How

Verification

Reviews

Known divergences (documented, none pinned as correct by any test)

Summary by CodeRabbit

Uh oh!

coderabbitai Bot commented Aug 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Walkthrough

Changes

Sequence Diagram(s)

Uh oh!

coderabbitai Bot left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions Bot commented Aug 9, 2026

Parse changes introduced by this PR

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

matthewevans commented Aug 8, 2026 •

edited by coderabbitai Bot

Loading

coderabbitai Bot commented Aug 8, 2026 •

edited

Loading