feat(engine): implement Jetfire, Ingenious Scientist // Jetfire, Air Guardian

[JacobWoodson]

Implements the Transformers DFC Jetfire, Ingenious Scientist // Jetfire, Air Guardian (set BOT).

Summary

End-to-end tracing showed almost every mechanic already existed — More Than Meets the Eye, Living metal, Flying, convert→transform, adapt, the "can't be spent to cast nonartifact spells" restriction (SpellTypeOrAbilityActivation{Artifact, Any}) and its cross-sentence attachment, and the counter-removal cost → chosen_x → "that much" wiring. Four surgical changes complete the two activated abilities:

1. parser (oracle_effect/mana.rs): strip_mana_subject_prefix recognizes "target player adds" → TargetFilter::Player, a genuine chosen recipient distinct from the "active/that player" anaphors (CR 115.1 + CR 106.4).
2. engine (effects/mana.rs): mana_effect_recipient now deposits mana into the targeted player for a Player recipient, provenance-gated by a new mana_count_reads_targets predicate — the identical TargetFilter::Player is also emitted as a count source ("Add {U} for each card in target player's hand"), where the recipient must stay the controller. No Jeska's Will regression.
3. parser (oracle_effect/mana.rs): "that much {C}" now parses as a count-prefixed colorless production (previously fell to Unimplemented).
4. parser (sequence.rs): "adapt" added to the clause-start verb table so "Convert Jetfire, then adapt 3" chains the adapt sub-ability.

No new enum variants.

Card text

• Front {4}{U} Legendary Artifact Creature — Robot 3/4: More Than Meets the Eye {3}{U}; Flying; Remove one or more +1/+1 counters from among artifacts you control: Target player adds that much {C}. This mana can't be spent to cast nonartifact spells. Convert Jetfire.
• Back Legendary Artifact — Vehicle 3/4: Living metal; Flying; {U}{U}{U}: Convert Jetfire, then adapt 3.

Tests

6 new tests: both faces parse end-to-end (front: chosen-X counter cost + targeted restricted mana + Convert sub-ability; back: convert→adapt chain), the "target player adds that much {C}" clause, targeted-player deposit, the multi-authority recipient-vs-count-source gate (incl. Jeska's Will non-regression), and spend-restriction deposit.

Verification

• Full engine test suite: 16,725 passed, 0 failed
• cargo fmt clean; cargo clippy -p engine --lib --tests clean (-D warnings)

🤖 Generated with Claude Code

Summary by CodeRabbit

• New Features

  • Mana effects now model recipient vs count-source targeting as distinct roles, including role-aware multi-slot selection.

• Bug Fixes

  • Improved mana parsing, targeting, and resolution so amounts and recipient determination use the correct role slots.
  • Enhanced retargeting and target scanning so loop/firewall and legacy/retarget logic can’t drop or misclassify mana target axes.
  • Updated signature rendering for mana so role-specific targeting no longer collapses together.

• Tests

  • Added unit, regression, and integration tests covering multi-role parsing/resolution, illegal-recipient/count-source behavior, and mana-target role migration.

[gemini-code-assist]

Code Review

This pull request implements support for target player mana recipients and dynamic colorless mana production (e.g., for Jetfire, Ingenious Scientist), including parsing, resolution, and associated tests. The review feedback highlights style guide violations regarding non-exhaustive matches with wildcard fallbacks in quantity_expr_reads_targets and quantity_ref_reads_targets, as well as a missing CR annotation in quantity_expr_reads_targets.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[matthewevans]

Blocked — the new generic target-player mana path still conflates a mana recipient with a count source.

🔴 Blocker

crates/engine/src/parser/oracle_effect/mana.rs:193-201 recursively parses the bare add … clause before it stamps the subject recipient. For Target player adds {C} for each card in target player's hand, the bare parser already emits Effect::Mana.target = Some(TargetFilter::Player) as the count-source target, so the wrapper leaves that field unchanged. crates/engine/src/game/effects/mana.rs:35-44 then uses the production count to reinterpret that same field and returns ability.controller whenever the count reads the target. That sends mana to the controller even though the sentence's subject is the chosen target player.

The current Jetfire fixture is target-independent (that much), so it does not reach this failure mode. The new parser arm accepts the broader subject-led class, however, and the implementation has no typed provenance that distinguishes its recipient target from the count target. The wildcard fallbacks in quantity_expr_reads_targets / quantity_ref_reads_targets (effects/mana.rs:87-144, also raised by Gemini) make that inference incomplete as the quantity vocabulary grows.

Suggested fix: model the two roles explicitly at the Effect::Mana/mana-production boundary (recipient vs count-source) and thread their independently surfaced target slots through parsing and resolution. Do not use quantity-shape inference to reinterpret the one overloaded target field; add a runtime fixture for the target-recipient + target-derived-count class as well as the existing Jetfire and Jeska's Will cases.

✅ Clean

The Scryfall Oracle text confirms Jetfire's front and back clauses, and the change uses the existing convert/adapt and restricted-mana seams. The Jetfire-specific parser assertions are appropriately detailed.

Recommendation: request changes for the explicit target-role model, then re-run current-head parser/engine evidence.

[github-actions]

Parse changes introduced by this PR · 58 card(s), 21 signature(s) (baseline: main 47118ea6c902)

🟢 Added (5 signatures)

• 1 card · ➕ ability/Adapt · added: Adapt (counters=3)
  • Affected (first 3): Jetfire, Air Guardian
• 1 card · ➕ ability/Mana · added: Mana (expiry=EndOfTurn, mana={C} x8)
  • Affected (first 3): Su-Chi Cave Guard
• 1 card · ➕ ability/Mana · added: Mana (kind=activated, mana={C} xevent amount)
  • Affected (first 3): Rasputin, the Oneiromancer
• 1 card · ➕ ability/Mana · added: Mana (kind=activated, mana={C} xevent amount, mana recipient=player, restrictions=[SpellTypeOrAbilityActivation { spell_type: "Artifact", ability: Any }])
  • Affected (first 3): Jetfire, Ingenious Scientist
• 1 card · ➕ ability/Mana · added: Mana (mana={C} xevent amount)
  • Affected (first 3): Mana Seism

🔴 Removed (2 signatures)

• 2 cards · ➖ ability/add · removed: add
  • Affected (first 3): Mana Seism, Su-Chi Cave Guard
• 2 cards · ➖ ability/add · removed: add (kind=activated)
  • Affected (first 3): Jetfire, Ingenious Scientist, Rasputin, the Oneiromancer

🟡 Modified fields (14 signatures)

• 20 cards · 🔄 ability/Mana · changed field mana recipient: ∅ → parent target's controller
  • Affected (first 3): Blighted Burgeoning, Buried in the Garden, Dawn's Reflection (+17 more)
• 20 cards · 🔄 ability/Mana · changed field target: parent target's controller → ∅
  • Affected (first 3): Blighted Burgeoning, Buried in the Garden, Dawn's Reflection (+17 more)
• 12 cards · 🔄 ability/Mana · changed field mana recipient: ∅ → triggering player
  • Affected (first 3): Barbflare Gremlin, Dictate of Karametra, Eloren Wilds (+9 more)
• 12 cards · 🔄 ability/Mana · changed field target: triggering player → ∅
  • Affected (first 3): Barbflare Gremlin, Dictate of Karametra, Eloren Wilds (+9 more)
• 7 cards · 🔄 ability/Mana · changed field mana recipient: ∅ → scoped player
  • Affected (first 3): Belbe, Corrupted Observer, Blinkmoth Urn, Cheering Crowd (+4 more)
• 7 cards · 🔄 ability/Mana · changed field target: scoped player → ∅
  • Affected (first 3): Belbe, Corrupted Observer, Blinkmoth Urn, Cheering Crowd (+4 more)
• 5 cards · 🔄 ability/Mana · changed field mana recipient: ∅ → player
  • Affected (first 3): Bigger on the Inside, Color Pie, Mad Science Fair Project (+2 more)
• 5 cards · 🔄 ability/Mana · changed field target: player → ∅
  • Affected (first 3): Bigger on the Inside, Color Pie, Mad Science Fair Project (+2 more)
• 4 cards · 🔄 ability/Mana · changed field mana count source: ∅ → opponent
  • Affected (first 3): Carpet of Flowers, Jeska's Will, Orcish Squatters Avatar (+1 more)
• 4 cards · 🔄 ability/Mana · changed field target: opponent → ∅
  • Affected (first 3): Carpet of Flowers, Jeska's Will, Orcish Squatters Avatar (+1 more)
• 3 cards · 🔄 ability/Mana · changed field mana recipient: ∅ → chosen player
  • Affected (first 3): Spectral Searchlight, Stadium Vendors, Valleymaker
• 3 cards · 🔄 ability/Mana · changed field mana recipient: ∅ → controller
  • Affected (first 3): Organ Harvest, Priest of Forgotten Gods, Red Death, Shipwrecker
• 3 cards · 🔄 ability/Mana · changed field target: chosen player → ∅
  • Affected (first 3): Spectral Searchlight, Stadium Vendors, Valleymaker
• 3 cards · 🔄 ability/Mana · changed field target: controller → ∅
  • Affected (first 3): Organ Harvest, Priest of Forgotten Gods, Red Death, Shipwrecker

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[JacobWoodson]

Thanks @matthewevans — the critique was correct, and digging into it showed the problem was worse than "architecturally untidy". Pushed as 70911c11.

The bug you predicted is real

For Target player adds {C} for each card in target player's hand, the inner add … parse already claimed Effect::Mana.target as the count source, so the subject-led wrapper's if target.is_none() guard silently discarded the recipient and the mana went to the controller. It could not have been fixed by better inference: QuantityRef::TargetZoneCardCount resolves by taking the first TargetRef::Player in ability.targets, so a recipient slot and a count-source slot both land at index 0 and collide. One field genuinely cannot carry two CR 601.2c instances of "target".

What changed

• Explicit roles. New ManaTargetRole { Recipient, CountSource, Both } replaces Effect::Mana.target's bare Option<TargetFilter>. The role is stamped at parse time from grammar — the point that actually knows which grammatical position a filter came from — never from the quantity.
• Quantity-shape inference deleted outright. mana_count_reads_targets, mana_production_count, quantity_expr_reads_targets, quantity_ref_reads_targets are gone. That also removes the two wildcard _ => false arms and the missing-CR annotation Gemini flagged, by construction rather than by patching them.
• Independently surfaced slots. One slot per role, recipient first, in collect_target_slots and its collect_target_slot_specs mirror, with matching arms in both assign functions, chain_has_target_sink, minimum_targets_in_chain, and a position-stable validate_targets_in_chain arm. retarget_slot_violation enforces per-slot CR 115.7a legality (each target can be changed only to another legal target), which the flat retarget pool could not express.
• Per-role resolution. Each role resolves against its own slot via ability_scoped_to_slot, so shared quantity resolution stays slot-agnostic and untouched. mana_effect_recipient now returns Option<PlayerId>, so an illegal recipient deposits nowhere rather than falling back to the controller (CR 608.2b).

Blast radius was deliberately held to the new class

Effect::target_filter() still returns the sole declared filter (declared_filters().next()). Making it return None for Mana looked cleaner but cascaded into ~40 call sites — chain_has_target_sink, minimum_targets_in_chain, the four pre-generic assign branches, the stack/triggers auto-skip gates, change_targets, ai_support/filter — and silently changed routing for shipping cards. The explicit arms are gated on mana_multi_role(), so all 13 shipping mana-target cards keep byte-identical routing; only the two-role shape enters new code. Worth noting for reviewers: 10 of the 11 fixture entries carry context-ref recipients, so a "first surfaced filter" accessor would return None for them and reinstate the whole cascade — hence declared_filters().next().

Fixture migration is name-keyed, not shape-derived

integration_cards.json carries 11 legacy Effect::Mana targets. A legacy-tolerant Deserialize is impossible without re-introducing exactly the inference you rejected: Carpet of Flowers (count source) and Spectral Searchlight (recipient) are both Typed with opposite roles. So the 11 were migrated by card name, with Carpet of Flowers as the canary, and the file stays one minified line (1 insertion / 1 deletion, zero snapshot churn).

Tests — all three classes you asked for

1. Jetfire: recipient + target-independent count → the targeted player's pool.
2. Jeska's Will: count-source only → the controller's pool.
3. New end-to-end cast-pipeline fixture for recipient + target-derived count: two different players with different hand sizes (2 vs 5), so a slot mix-up changes both the destination and the amount. Reverting the fix fails it with left: 0, right: 5.

18 new tests total. Engine suite 16743 passed / 0 failed; integration 3164 / 0; cargo clippy --workspace --all-targets -- -D warnings clean.

Known gaps, stated plainly

• retarget_slot_violation is covered at helper level, not driven through the full GameAction::RetargetSpell → apply_retarget route. It is gated behind a shape no printed card reaches, so I left it as a declared gap rather than build more synthetic harness — happy to add it if you'd rather.
• Attach / MoveCounters / Fight share the same pre-existing flat-pool retarget gap. Deliberately untouched; retarget_slot_violation is the seam they'd extend into if that's fixed on its own merits.
• cargo coverage, cargo semantic-audit, and parser gate family (D) could not run in my environment (missing card-data.json / python3). Treating those as could-not-determine rather than passing — CI covers them.

One deliberate behavioral note: validate_targets_in_chain's new arm is gated on mana_multi_role, so single-role mana keeps the generic companion-aware split_first path. An earlier revision broadened it and silently discarded a legal unless_pay companion target; that's fixed and pinned by a test.

[matthewevans]

[MED] An illegal count-source target can make the effect count the recipient instead. Evidence: crates/engine/src/game/effects/mana.rs:110-120 falls back to ability.clone() whenever ability_scoped_to_slot(..., CountSource) returns None; crates/engine/src/game/quantity.rs:2134-2141 then reads the first player target. Why it matters: for a Both mana effect whose recipient remains legal but whose count source becomes illegal, the fallback exposes the recipient as that first target, so the effect produces mana based on the wrong player's hand rather than failing to determine the illegal target's information (CR 608.2b). Suggested fix: distinguish “no count-source role” from “declared count source no longer legal”; for the latter resolve the count through an empty/invalid scoped target view (or skip the dependent mana instruction), and add a cast-pipeline regression with a legal recipient and an illegal count source having different hand sizes.

[JacobWoodson]

Confirmed and fixed in f4789fa9. You were right, and I reproduced it before touching anything: reverting the fix makes the new regression fail with left: 2, right: 2 — the count read the recipient's hand (2) instead of failing to determine. Exactly the path you described.

The root cause was that count_scoped_ability conflated two different None returns from ability_scoped_to_slot. It now distinguishes four cases, not three:

1. No count-source role declared — unscoped, which is exactly today's behavior for every single-role mana (Jetfire, and all target: None).
2. Context-ref count source — resolved through context, mirroring what mana_effect_recipient already did. This one is on me: slot_index returns None for a context-ref filter, not just for an illegal target, so this input was landing in case 4 and silently resolving to 0 under a CR 608.2b citation that does not apply to it — nothing there is an illegal target. Latent today (the parser only builds Player / opponent-Typed count sources) but with_count_source is public.
3. Declared and still legal — narrowed to that one player.
4. Declared but illegal — no player exposed, count resolves to 0 per CR 608.2b.

Two things beyond the reported instance

The same defect existed at a second site you didn't cite. handle_choose_mana_effect → chosen_mana_types_for_prompt also derives the count — its SingleColor arm multiplies the chosen color by it — and was passing the unscoped ability. So a Both role with a colour prompt had the identical bug. Both sites now route through count_scoped_ability. Fixing only the site you named would have repeated the "fixed the instance, missed the class" mistake from the previous round.

Scoping is now confined to the player axis. My first pass cleared the whole targets vec, which was too blunt: ManaProduction::AnyCombinationOfObjectColors { scope: Target } reads an object target out of that same vec and requires nothing about the count source. Clearing it would have made the colour half of the production silently yield no colours — which CR 608.2b doesn't license, since only the part requiring information about the illegal target fails. retain_only_player_at now drops only TargetRef::Player entries and leaves non-player targets at their original positions. That also tightened case 3, which inherited the same over-broad clear from 70911c11.

Regression test

illegal_count_source_fails_to_determine_instead_of_counting_the_recipient — real cast pipeline, 3 players, recipient P1 holding 2 cards, count source P2 holding 5, P2 eliminated between SelectTargets and resolution. Two reach guards, because assert_eq!(pool, 0) on its own is satisfiable by a whole-spell fizzle: the recipient is asserted still legal, and the test collects ActionResult.events through the resolve loop and asserts EffectResolved { kind: Mana } actually fired. So it pins the per-target "fails to determine" clause rather than CR 608.2b's other clause.

Engine suite 16743 passed / 0 failed; integration 3165 / 0; cargo clippy --workspace --all-targets -- -D warnings clean.

Still open from my earlier note

retarget_slot_violation remains helper-level only (not driven through GameAction::RetargetSpell), and the Attach / MoveCounters / Fight flat-pool retarget gap is untouched. Both are gated behind shapes no printed card reaches. Say the word if you'd rather I close either now rather than leave them as declared gaps.

[matthewevans]

Blocked — the current fix addresses the earlier illegal count-source fallback, but the authorized maintainer port cannot safely complete under the Tilt-only verification policy.

The attempted merge across current main has one conflict in crates/engine/tests/fixtures/integration_cards.json. This is a generated single-line fixture, not a textual choice: this PR regenerates 12 mana-related entries, while current main has a broad independent regeneration (2,945 keys versus this branch's 2,807). Several entries overlap. Taking either side would silently discard parser-derived fixture behavior; producing the merged artifact requires the project card-data/Tilt path, which is not attached to this isolated worktree and must not be replaced by a direct build.

The targeted code review of f4789fa found the new count-scoped view and its real cast-pipeline regression appropriately address the prior recipient-count fallback. The remaining blocker is the verified generated-fixture merge/verification conflict only.

Recommendation: hold this head until a Tilt-attached maintainer port can regenerate and verify the fixture.

[matthewevans]

Maintainer status — held on a real merge conflict.

Current head 4ce98ba3bc3ad9147f0fe0ed6694685cca17e268 is DIRTY/CONFLICTING, so GitHub's update-branch operation cannot perform the required semantic integration. The earlier maintainer port identified a conflict in the generated integration-card fixture; resolving it requires the generated fixture to be regenerated and verified through the project's Tilt workflow. I have not used a direct build or a pick-a-side merge resolution.

[coderabbitai]

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: a33b563e-ca3a-42e0-8f14-d0d4e32659af

📥 Commits

Reviewing files that changed from the base of the PR and between cfc6882 and 5f7ce31.

📒 Files selected for processing (2)

• crates/engine/src/game/engine.rs
• crates/engine/tests/integration/main.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/engine/tests/integration/main.rs
• crates/engine/src/game/engine.rs

---

📝 Walkthrough

Walkthrough

Mana effects now model recipient and count-source targets separately, preserve both roles through parsing and migration, expose role-specific target slots, scope quantity resolution correctly, and validate legality per slot. Supporting scans, rendering, importer logic, and regression coverage were updated.

Changes

Mana role model and parsing

Layer / File(s)	Summary
Role model and parser propagation crates/engine/src/types/ability.rs, crates/engine/src/parser/oracle_effect/*, crates/engine/src/parser/oracle_ir/ast.rs, crates/engine/src/parser/oracle_tests.rs, crates/engine/src/parser/oracle_trigger_tests.rs	Mana targets use ManaTargetRole and preserve recipient/count-source combinations through parsing, lowering, serialization, and oracle tests.
Fixture and state migration scripts/migrate-mana-target-roles.mjs, crates/engine/tests/integration/*mana*	Migration tooling converts legacy mana target encodings, validates mapped entries, and tests migrated role classifications and runtime behavior.
Multi-role target slots crates/engine/src/game/ability_utils.rs, crates/engine/src/game/effects/change_targets.rs, crates/engine/src/game/engine.rs	Target collection, assignment, reservation, retargeting, and per-slot legality now support surfaced recipient and count-source slots.
Role-scoped mana resolution crates/engine/src/game/effects/mana.rs	Mana quantities resolve from the count-source slot, deposits use the legal recipient, and illegal targets produce no redirected mana.
Compatibility scans and derived output crates/engine/src/ai_support/filter.rs, crates/engine/src/game/ability_rw.rs, crates/engine/src/game/ability_scan.rs, crates/engine/src/game/coverage.rs, crates/engine/src/game/mana_abilities.rs, crates/mtgish-import/src/convert/action.rs	Safety, legacy-tag, loop-firewall, classification, parse-detail, and importer logic inspect declared mana role filters.

Estimated code review effort: 5 (Critical) | ~120 minutes

Sequence Diagram(s)

sequenceDiagram
  participant Parser
  participant TargetSelection
  participant ManaResolution
  participant CountSource
  participant Recipient
  Parser->>TargetSelection: expose recipient and count-source slots
  TargetSelection->>ManaResolution: submit role-specific targets
  ManaResolution->>CountSource: resolve quantity from count-source slot
  CountSource-->>ManaResolution: return resolved quantity
  ManaResolution->>Recipient: deposit mana for legal recipient

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 is clearly related to the PR’s main Jetfire card implementation and engine support work.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
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 unit tests (beta)

• Create PR with unit tests

---

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

[matthewevans]

One multi-role retargeting path remains unsafe:

The new ManaTargetRole::Both branch can produce a flat target union, but forced retargeting replaces stack_ability.targets with a one-element vector and bypasses the per-slot validation used by interactive retargeting. A forced-target effect can therefore drop the other target or place the target in the wrong role. Please preserve the target vector and select/validate the intended role slot (or keep multi-slot mana out of forced retargeting until its semantics are represented), with an end-to-end forced-retarget regression.

[coderabbitai]

Actionable comments posted: 2

🧹 Nitpick comments (2)

crates/engine/src/game/ability_rw.rs (1)

6440-6475: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Duplicate mana_fixture_roles() test helper across two files. The same fixture matrix (five shipping mana-target-role shapes) is copy-pasted verbatim into both test modules; the shared root cause is having no single source of truth for this fixture set even though both suites test CR-behavior-preservation against it.

• crates/engine/src/game/ability_rw.rs#L6440-L6475: keep (or move) the canonical definition here.
• crates/engine/src/game/mana_abilities.rs#L3770-L3806: drop the duplicate and import the shared helper instead — e.g. via crate::game::test_fixtures, the module mana_abilities.rs already imports from for brushland_colored_ability.

🤖 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/ability_rw.rs` around lines 6440 - 6475, The
five-shape mana fixture matrix is duplicated across two test modules; retain
mana_fixture_roles() in crates/engine/src/game/ability_rw.rs:6440-6475 as the
canonical definition, remove the duplicate helper from
crates/engine/src/game/mana_abilities.rs:3770-3806, and import/reuse the shared
helper there through the existing crate::game::test_fixtures path.

crates/engine/src/parser/oracle_effect/mana.rs (1)

612-630: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a fixed-count test for the new colorless count-prefix arm.

This new arm is exercised only via the "that much {C}" (EventContextAmount) path (parse_add_mana_target_player_that_much_colorless). There's no test driving a literal numeral (e.g., "Add three {C}.") through this exact branch to confirm scale_for_each_count(symbol_count, QuantityExpr::Fixed { .. }) behaves correctly here — the parameter range of the new grammar arm (Fixed vs. Ref counts) isn't independently covered.

🧪 Suggested additional test

#[test]
fn parse_add_mana_fixed_count_colorless_no_subject() {
    let effect = try_parse_add_mana_effect("Add three {C}.")
        .expect("count-prefixed colorless mana must parse");
    match effect {
        Effect::Mana {
            produced: ManaProduction::Colorless { count },
            target,
            ..
        } => {
            assert_eq!(count, QuantityExpr::Fixed { value: 3 });
            assert_eq!(target, None);
        }
        other => panic!("expected Effect::Mana, got {other:?}"),
    }
}

As per path instructions, "Build reusable card-class building blocks rather than one-card special cases... test the building block and its parameter range rather than a single card case."

🤖 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/parser/oracle_effect/mana.rs` around lines 612 - 630, Add a
fixed-count parser test near the existing add-mana tests using
try_parse_add_mana_effect with “Add three {C}.”. Assert it produces Effect::Mana
with Colorless count QuantityExpr::Fixed value 3 and no target, covering the
count-prefix branch and its Fixed parameter path.

Source: 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.

Inline comments:
In `@crates/engine/src/game/ability_rw.rs`:
- Around line 3109-3123: Update the comment above the Effect::Mana arm to cite
CR 601.2c instead of CR 603.10a, while preserving the existing explanation that
the activated mana ability scans the target role’s declared filters.

In `@scripts/migrate-mana-target-roles.mjs`:
- Around line 151-155: Update FIELD_BY_ROLE and the envelope-writing paths to
handle Both explicitly: make envelopeField(role) reject Both (and any
unsupported role) before constructing an envelope, since a single legacy target
filter cannot reconstruct both filters. Use envelopeField(role) for the computed
keys at both writers so invalid roles fail loudly instead of producing an
"undefined" property.

---

Nitpick comments:
In `@crates/engine/src/game/ability_rw.rs`:
- Around line 6440-6475: The five-shape mana fixture matrix is duplicated across
two test modules; retain mana_fixture_roles() in
crates/engine/src/game/ability_rw.rs:6440-6475 as the canonical definition,
remove the duplicate helper from
crates/engine/src/game/mana_abilities.rs:3770-3806, and import/reuse the shared
helper there through the existing crate::game::test_fixtures path.

In `@crates/engine/src/parser/oracle_effect/mana.rs`:
- Around line 612-630: Add a fixed-count parser test near the existing add-mana
tests using try_parse_add_mana_effect with “Add three {C}.”. Assert it produces
Effect::Mana with Colorless count QuantityExpr::Fixed value 3 and no target,
covering the count-prefix branch and its Fixed parameter path.

🪄 Autofix (Beta)

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: 54010d4d-b01c-4738-b792-aa30ca2a78da

📥 Commits

Reviewing files that changed from the base of the PR and between bdb1f3d and 7517ad6.

⛔ Files ignored due to path filters (4)

• crates/engine/tests/fixtures/combo_infinite_pile_4p_offer.json.gz is excluded by !**/*.gz
• crates/engine/tests/fixtures/combo_infinite_pile_4p_untapped_precast.json.gz is excluded by !**/*.gz
• crates/engine/tests/fixtures/kilo_freed_relic_pentad_4p.json.gz is excluded by !**/*.gz
• crates/engine/tests/fixtures/sprout_witherbloom_realistic_lands_4p.json.gz is excluded by !**/*.gz

📒 Files selected for processing (24)

• crates/engine/src/ai_support/filter.rs
• crates/engine/src/game/ability_rw.rs
• crates/engine/src/game/ability_scan.rs
• crates/engine/src/game/ability_utils.rs
• crates/engine/src/game/coverage.rs
• crates/engine/src/game/effects/change_targets.rs
• crates/engine/src/game/effects/mana.rs
• crates/engine/src/game/engine.rs
• crates/engine/src/game/mana_abilities.rs
• crates/engine/src/parser/oracle_effect/imperative.rs
• crates/engine/src/parser/oracle_effect/mana.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_effect/sequence.rs
• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/src/parser/oracle_ir/ast.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/src/types/ability.rs
• crates/engine/tests/fixtures/integration_cards.json
• crates/engine/tests/integration/main.rs
• crates/engine/tests/integration/mana_role_fixture_migration.rs
• crates/engine/tests/integration/mana_target_recipient_and_count_source.rs
• crates/mtgish-import/src/convert/action.rs
• scripts/migrate-mana-target-roles.mjs

[coderabbitai]

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
rg -n "^603\.10" docs/MagicCompRules.txt
rg -n "^601\.2c" docs/MagicCompRules.txt

Repository: phase-rs/phase

Length of output: 280

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'Repo root: '; pwd
printf '\nTop-level files:\n'
git ls-files | sed -n '1,80p'

printf '\nLooking for CR docs and relevant code:\n'
fd -a 'MagicCompRules.txt|CLAUDE.md|ability_rw.rs|ai_support/filter.rs|ability_scan.rs|coverage.rs' .

printf '\nSearch for cited rules and mana-target annotation:\n'
rg -n '^603\.10a|^601\.2c|CR 603\.10a|CR 601\.2c|Effect::Mana' .

Repository: phase-rs/phase

Length of output: 50371

---

🏁 Script executed:

#!/bin/bash
set -euo pipefail

printf 'CR docs locations:\n'
fd -a 'MagicCompRules.txt|MagicCompRules.*' . || true

printf '\nability_rw slice:\n'
sed -n '3095,3135p' crates/engine/src/game/ability_rw.rs

printf '\nSibling CR citations around Effect::Mana:\n'
sed -n '1,120p' crates/phase-ai/src/policies/free_outlet_activation.rs
printf '\n---\n'
sed -n '1,120p' crates/phase-ai/src/search.rs
printf '\n---\n'
sed -n '1,120p' crates/engine/tests/integration/kutzils_flanker_mode_one_counter.rs

Repository: phase-rs/phase

Length of output: 17871

---

Replace the CR 603.10a citation with CR 601.2c here; this Effect::Mana arm is scanning the target role on an activated mana ability.

🤖 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/ability_rw.rs` around lines 3109 - 3123, Update the
comment above the Effect::Mana arm to cite CR 601.2c instead of CR 603.10a,
while preserving the existing explanation that the activated mana ability scans
the target role’s declared filters.

Source: Path instructions

[coderabbitai]

🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win

Both roles silently produce a malformed envelope instead of failing loudly. FIELD_BY_ROLE has no Both entry, yet the docs (lines 80, 292) tell maintainers to add Both { recipient, count_source } mappings. If a "Both" value ever reaches the writers at lines 210/271, FIELD_BY_ROLE["Both"] is undefined, so the computed key stringifies to "undefined" and you get { role: "Both", "undefined": <filter> } written into the fixture/state — silent corruption, directly contradicting the "fail loudly rather than guess" property. Additionally, a Both card cannot be reconstructed from a single legacy target filter at all (there's only one filter to wrap), so it must be rejected explicitly.

🛡️ Proposed guard

 const FIELD_BY_ROLE = {
   Recipient: "recipient",
   CountSource: "count_source",
 };
+
+// A `Both` role needs two independent filters; the legacy encoding carries only
+// one, so it is not migratable by this script and must never be inferred here.
+function envelopeField(role) {
+  const field = FIELD_BY_ROLE[role];
+  if (field === undefined) {
+    throw new Error(
+      `Cannot migrate role "${role}" from a single legacy filter ` +
+        `(only Recipient/CountSource are single-filter). Decide it by hand.`,
+    );
+  }
+  return field;
+}

Then use [envelopeField(role)] at lines 210 and 271.

🤖 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 `@scripts/migrate-mana-target-roles.mjs` around lines 151 - 155, Update
FIELD_BY_ROLE and the envelope-writing paths to handle Both explicitly: make
envelopeField(role) reject Both (and any unsupported role) before constructing
an envelope, since a single legacy target filter cannot reconstruct both
filters. Use envelopeField(role) for the computed keys at both writers so
invalid roles fail loudly instead of producing an "undefined" property.

[coderabbitai]

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/coverage.rs (1)

10743-10808: 📐 Maintainability & Code Quality | 🟠 Major | 🏗️ Heavy lift

Move the new coverage tests to the integration test tree.

Lines [10743] and [12389] add engine tests inside the private mod tests of crates/engine/src/game/coverage.rs. Move these cases under crates/engine/tests/integration/, wired through crates/engine/tests/integration/main.rs, and exercise the corresponding public coverage entry points there.

As per path instructions, new engine tests must go under crates/engine/tests/integration/ as modules wired from crates/engine/tests/integration/main.rs (avoid new top-level test binaries).

Also applies to: 12389-12440

🤖 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/coverage.rs` around lines 10743 - 10808, Move the new
tests, including mana_role_parse_details_names_each_role_key and the
corresponding test near the later range, out of the private tests module in
coverage.rs into modules under the integration test tree. Wire those modules
through integration/main.rs, adapt them to use the public coverage entry points
and accessible APIs, and remove the in-file test copies without creating
additional top-level test binaries.

Source: 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/coverage.rs`:
- Around line 10743-10808: Move the new tests, including
mana_role_parse_details_names_each_role_key and the corresponding test near the
later range, out of the private tests module in coverage.rs into modules under
the integration test tree. Wire those modules through integration/main.rs, adapt
them to use the public coverage entry points and accessible APIs, and remove the
in-file test copies without creating additional top-level test binaries.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 3bc6eefe-b5a5-47e4-b94f-febc3b2997e6

📥 Commits

Reviewing files that changed from the base of the PR and between 7517ad6 and cfc6882.

📒 Files selected for processing (15)

• crates/engine/src/ai_support/filter.rs
• crates/engine/src/game/ability_rw.rs
• crates/engine/src/game/ability_scan.rs
• crates/engine/src/game/ability_utils.rs
• crates/engine/src/game/coverage.rs
• crates/engine/src/game/engine.rs
• crates/engine/src/game/mana_abilities.rs
• crates/engine/src/parser/oracle_effect/imperative.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/src/parser/oracle_ir/ast.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/src/types/ability.rs
• crates/engine/tests/integration/main.rs

🚧 Files skipped from review as they are similar to previous changes (14)

• crates/engine/tests/integration/main.rs
• crates/engine/src/parser/oracle_effect/imperative.rs
• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_ir/ast.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/src/game/engine.rs
• crates/engine/src/parser/oracle_effect/tests.rs
• crates/engine/src/game/mana_abilities.rs
• crates/engine/src/types/ability.rs
• crates/engine/src/game/ability_scan.rs
• crates/engine/src/ai_support/filter.rs
• crates/engine/src/game/ability_rw.rs
• crates/engine/src/game/ability_utils.rs

[matthewevans]

Blocked — the current head still drops multi-role targets during forced retargeting and can corrupt a future migration.

🔴 Blocker

crates/engine/src/game/effects/change_targets.rs:71-82 takes the forced-retarget path outside engine.rs::apply_retarget: after checking a flat union, it overwrites stack_ability.targets with vec![new_target]. A ManaTargetRole::Both spell has two independently declared target slots; this drops the other slot and never applies the positional legality check added for interactive retargeting. CR 115.7a says each target may change only to another legal target and otherwise the original target remains unchanged; CR 115.7b permits changing only one target, not deleting the others (verified in docs/MagicCompRules.txt:863-869). The existing forced-retarget test covers a single-target destroy spell, so it cannot exercise this new multi-slot production path.

Please represent which target slot a forced effect changes, preserve all other targets, and validate the replacement against that slot's filter. Add an end-to-end forced-retarget regression for a Both mana ability that proves both the untouched target and the role-specific replacement survive; it should fail with this vec![new_target] assignment.

🟡 Non-blocking

scripts/migrate-mana-target-roles.mjs:151-155,210,271 documents Both as a supported mapping but has field names only for Recipient and CountSource. If a maintainer follows that documented path, the computed key is undefined, producing a malformed { role: "Both", undefined: ... } envelope rather than failing loudly. A legacy single filter cannot reconstruct both roles, so reject Both explicitly (and require a dedicated migration) before either writer constructs an envelope.

✅ Clean

The role model correctly separates recipient and count-source provenance, and the current parse-diff artifact is present for this head: 58 cards / 21 signatures, updated after the merge commit (https://github.com/phase-rs/phase/pull/6056#issuecomment-4997648480).

Recommendation: request changes. Resolve the forced-retarget behavior with a discriminating runtime test; the migration guard can ride with that repair.

[matthewevans]

Blocked — forced retargeting still destroys one target slot for a multi-role mana ability.

🔴 Blocker

crates/engine/src/game/effects/change_targets.rs:71-82 validates the forced replacement against a flat union and then assigns stack_ability.targets = vec![new_target]. A mana ability with independent recipient and count-source targets therefore loses its other declared target; this path does not call the positional retarget_slot_violation guard added only to interactive apply_retarget in crates/engine/src/game/engine.rs:8456-8483. CR 115.7a requires each target to be changed only to another legal target, not removal or reassignment of a separate target slot.

Use the role-slot authority to identify which slot the forced filter can replace, preserve the full target vector, validate that slot against its own filter, and add an end-to-end forced-retarget regression with distinct legal recipient and count-source players.

Recommendation: request changes; retain the two target roles through the forced-retarget assignment before re-review.