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 targeting now models recipient vs count-source as distinct roles, enabling correct multi-slot selection and retargeting.
• Bug Fixes
  • Fixed mana resolution to scope quantities to the declared count-source role and handle illegal recipient/count-source choices without redirecting.
  • Improved mana scanning/legacy retargeting and strengthened memoization and coverage rendering to correctly reflect role-specific targeting.
• Tests
  • Added unit, regression, and integration tests for role parsing/resolution and multi-role mana behavior.
• Chores
  • Added a migration script to convert legacy mana-target data into role-based targeting forms.

[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 c521e8654c5b)

🟢 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]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 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.
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, crates/engine/tests/integration/mana_target_recipient_and_count_source.rs	Mana quantities resolve from the count-source slot, deposits use the legal recipient, and illegal targets produce no redirected mana.
Fixture and state migration scripts/migrate-mana-target-roles.mjs, crates/engine/tests/integration/mana_role_fixture_migration.rs	Migration tooling converts legacy mana target encodings and validates mapped role classifications.
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

Suggested labels: quality, needs-maintainer

Suggested reviewers: matthewevans, andriypolanski, claytonlin1110, jsdevninja

🚥 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 names the primary feature and matches the Jetfire DFC support implemented by this change set.
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.

[matthewevans]

[HIGH] Forced multi-role retargeting can still resolve as an illegal no-op.

Current head: e14eb2c43400f57d3a0d3531e30500cd0519a88d.

change_targets::resolve accepts the first target in the forced filter that is in the flattened legal pool (lines 71–90). forced_retarget_targets then chooses the first compatible role slot without requiring that slot's existing target differ (lines 146–165).

For a Both mana ability with [recipient=P1, count_source=P2], forced-to P1 selects slot 0 and writes P1 back into it. That leaves both targets unchanged even though slot 1 can legally change from P2 to P1. CR 115.7a requires a changed target to become another legal target; CR 115.7b permits exactly one such change.

Select a compatible slot whose current target differs from the forced target, and leave the original targets unchanged only when no slot can be changed. Add the P1/P2 → forced-P1 regression beside the existing forced-retarget test; it must assert [P1, P1], not merely that the two-slot list survives.

The prior target-collapse fix is covered, but its current P3 fixture only exercises a different recipient and cannot detect this no-op branch. Current CI is also still in progress; this functional blocker is independent of that pending evidence.

[JacobWoodson]

@matthewevans — to save you the round-trips: JacobWoodson:jetfire-rebased is a single clean solution to all three blockers on this head, and I'm offering to make it the PR head if you're open to it.

It's the Jetfire work rebased into a linear history on main (no merge commits), which is what makes the parser diff auditable — the net diff is only the Jetfire changes (the reconciliation in my previous comment maps every one of them). On top of that it carries:

• The ability_utils.rs compile fix — the multi-role mana slot arm returns no_legal_target_slots() instead of EngineError::ActionNotAllowed, so it builds against current main.
• The forced-retarget current != new refinement (46c302de) with the overlapping-role [A,B]→force A→[A,A] regression, revert-verified.
• The phase-ai/zone_eval.rs ManaTargetRole::Recipient wrapper main's new test needs.

It was green when built (phase-engine lib + integration + clippy --workspace --all-targets). main has moved since, so if you want it as the head I'll rebase it onto whatever main tip is current at that moment and re-run the full gate before it lands — the fixture step is now a single node scripts/migrate-mana-target-roles.mjs (it fail-loud-guards any new mana-target card, which already caught The Warring Triad).

Because making it the head means replacing your 803e6905, I won't touch the PR branch without your go-ahead. Two ways to do it:

• You: git fetch https://github.com/JacobWoodson/phase jetfire-rebased && git push origin +jetfire-rebased:claude/jetfire-characters-3b456b (after a freshen).
• Me: say the word and I'll rebase it current, verify green, and force-push it to the PR branch.

If you'd rather keep driving 803e6905, everything's already in your hands — the compile fix, the CR 115.7a patch, and the reconciliation — and I'll stand down on the branch.

[matthewevans]

Thanks for the proposed jetfire-rebased solution. I re-reviewed the PR's actual current head, e14eb2c43400f57d3a0d3531e30500cd0519a88d; it remains unchanged, so the proposal has not resolved this PR's formal blocker.

forced_retarget_targets still chooses the first compatible role slot without requiring current_targets[slot] != new_target, then writes the target back into that slot. Thus [P1, P2] forced to P1 still selects slot 0 and no-ops although slot 1 can legally become P1. The current required Rust/card-data checks are also failing on this head.

I am retaining the existing CHANGES_REQUESTED review. Please push the proposed solution (including the [P1, P2] -> [P1, P1] runtime regression and its current-head checks) to this PR, or open the rebased branch as a replacement PR for review.

[JacobWoodson]

@matthewevans — pushed the solution to this PR head (e14eb2c4..e93b4732, fast-forward on top of your merge, nothing force-pushed). Both HIGH blockers are now resolved on the branch:

• ability_utils.rs compile break (E0308, EngineError::ActionNotAllowed vs TargetSlotBuildError) — the multi-role mana slot arm now returns no_legal_target_slots() like its sibling arms, so it builds against current main.
• Forced-retarget CR 115.7a refinement — forced_retarget_targets now picks the first surfaced slot whose filter legally accepts the candidate and whose current target actually differs from it. "Change a target" must change it to another legal target, so a slot already holding the candidate is skipped in favor of one that can genuinely change. Added the overlapping-role regression: targets [P1, P2], force P1 → [P1, P1] by changing the count-source slot (revert-verified — dropping the changes guard leaves [P1, P2] and fails).
• phase-ai/zone_eval.rs targeted-mana test adapted to the ManaTargetRole::Recipient model.

Green locally: cargo fmt; clippy --workspace --all-targets clean; phase-engine integration forced_retarget tests pass; phase-ai targeted-mana test passes.

[matthewevans]

Current-head review for e93b47320d11198cfd70bd4de7734e4f6c3e2d51:

The CR 115.7a forced-retarget correction is now present and the new overlapping-role regression covers the previously reported no-op. The PR still cannot merge because the schema migration is incomplete.

Effect::Mana.target now deserializes as ManaTargetRole, but the tracked integration fixture has not been migrated: it still contains 12 legacy target objects and zero { role: ... } envelopes. The current CI merge commit consequently fails both Rust test shards while loading the fixture with missing field 'role' (for example, temporal_anchor..., run 30728170186). The migration script itself is not sufficient unless its generated fixture output is committed.

Please run the intended fixture migration/regeneration, commit the resulting crates/engine/tests/fixtures/integration_cards.json, and rerun the required Rust checks. I will re-review the resulting head.

[JacobWoodson]

Follow-up: the required Rust test shards were red on the head, and the cause was a stale test fixture, not the retarget logic. Pushed e93b4732..2e274b1c.

crates/engine/tests/fixtures/integration_cards.json still carried the legacy Effect::Mana.target: Option<TargetFilter> shape — the main-merge regenerated it from main without re-running the mana-target-role migration — so every fixture-loading test panicked with missing field 'role' (both shards).

Re-running the migration surfaced a second, latent defect in the migration script itself: the fixture now carries a u64 sentinel (a SpecificObject id of u64::MAX, 18446744073709551615) that arrived on main via the Jailbreak fix (#6691). The script's JSON.parse/JSON.stringify round-trip silently rewrote it to a lossy float (1.8446744073709552e+19), which serde rejects as invalid type: floating point, expected u64. I made the fixture-mode round-trip bigint-lossless — a string-aware scanner quotes 16+ digit integer values behind an ASCII sentinel across parse/stringify, tracking JSON string state so it never touches [/,+digit sequences inside card text (the fragility a naive regex would have).

Verified locally after the fix:

• migration migrates 12 entries fail-loud clean; u64 sentinel survives as an integer, no float leak;
• previously-failing phase-ai subtlety scenario passes; engine analysis::corpus_tests (46), ai_support::candidates (43), analysis::ability_graph (42) all pass;
• forced_retarget integration tests (2) still pass; clippy --workspace --all-targets clean.

Note: the two hour-long AI/perf gates (Decision-cost, Paired-seed AI) were already red before this and this PR changes no AI-behavior or hot-path code — happy to dig in if you consider them in scope for this PR.

[matthewevans]

Current-head review for 2e274b1cf9180d18a6738f193fd929d3183f8dfc:

The curated fixture migration fixes the previous blocker, and the earlier CR 115.7a retarget correction remains correct. Required Rust CI is still red because the persisted-game migration is incomplete.

The current head migrates four *.json.gz game-state fixtures, but two tracked fixtures still contain legacy Effect::Mana.target values without a ManaTargetRole.role field:

• crates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gz (four Priest of Forgotten Gods entries)
• crates/engine/tests/fixtures/dina_conqueror_4p.json.gz (Priest of Forgotten Gods and The Warring Triad entries)

This is exactly what the current required test run reports: both shards fail deserializing real game states with missing field 'role' (run 30731083891; e.g. the loop-shortcut and Dina scenarios). The migration script already has --state handling and explicit mappings for these state-only cards; apply it to both remaining fixtures, commit the regenerated gzip files, and let the current-head Rust checks complete successfully.

[JacobWoodson]

Update: the fixture fix resolved most of the shard failures but exposed a second stale-fixture class — two persisted 4p GameState dumps still carried the legacy mana shape, so restoring them panicked with missing field 'role' (6 tests across both shards). Pushed 2e274b1c..bee266df.

• dina_conqueror_4p.json.gz (8 mana targets) → analysis::resource::tests::dina_untargeted_drain_4p_...
• witherbloom_sprout_lumaret_simple_4p.json.gz (4) → the 5 loop_shortcut::*collapse* tests

A GameState deserializes through typed serde before any restore-time migration hook runs, so the dump itself has to carry the role encoding. Re-migrated both via the script's --state mode (all roles → Recipient: Priest of Forgotten Gods, The Warring Triad), and applied the same bigint-lossless round-trip to --state mode since these dumps carry u64 object-id sentinels.

To make sure this was the last of it, I swept every committed .json/.json.gz in the repo (169 files) for a Mana effect whose target lacks a role — zero remain. So the card fixture plus these two dumps were the complete set.

Verified locally: the dina resource test and all 71 loop_shortcut integration tests pass (including the 6 that were red); both dumps re-scan with zero stale targets and no float/sentinel corruption. CI is re-running on bee266df.

[matthewevans]

Current-head review complete: persisted fixtures are migrated, required checks are green, and prior retarget/schema findings are resolved.