feat(parser): implement the assimilate keyword action - #7096
Conversation
Lower `assimilate <target>` to the reanimate-then-retype chain the engine
already ships: move the targeted card from an opponent's graveyard to the
battlefield under your control with a +1/+1 counter, then install a
permanent layer-4 type override making it a Borg artifact creature that
loses its other creature types.
The keyword action's definition arrives only as reminder text, which is
stripped before the parser runs, so it is encoded in lowering rather than
parsed -- the same reason Recruit is a parser IR node (CR 701.70a).
CR 205.1b: an effect making an object a "[creature type or types] artifact
creature" retains all prior card types, supertypes, and non-creature
subtypes, and replaces only the creature types. So the lowering emits
additive AddTypes plus a creature-set-scoped subtype replacement, never
SetCardTypes -- an assimilated legendary artifact enchantment creature
keeps Legendary and Enchantment.
All four modifications ride one StaticDefinition because they are all
layer 4 (CR 613.1d), so written order decides: RemoveAllSubtypes{Creature}
must precede AddSubtype{Borg} or the wipe erases Borg. Split across
separate definitions they would instead order by CR 613.7 timestamp.
Duration is explicitly Permanent (CR 611.2a): the GenericEffect fallback
is UntilEndOfTurn, which would silently expire the type change at cleanup
without failing any shape assertion.
`origin` is None, matching Ashen Powder and Puppeteer Clique for the same
"from an opponent's graveyard" phrase. Some(Graveyard) would additionally
flip AI reanimation detection for this card alone while mechanically
identical cards stayed unflipped.
AddType{Creature} is required rather than defensive: per CR 613.7n a
permanent's own static ability receives an earlier relative timestamp than
a characteristic-setting effect from the same resolution, so a
devotion-gated RemoveType{Creature} would otherwise win.
Registers the verb in gap_analysis::IMPERATIVE_EXTRA_VERBS, mirroring
recruit, which is likewise an anchored pre-dispatch nom probe.
📝 WalkthroughWalkthroughThe parser now recognizes ChangesAssimilate support
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant OracleText
participant ImperativeParser
participant GamePipeline
participant Battlefield
OracleText->>ImperativeParser: Parse assimilate target
ImperativeParser->>GamePipeline: Lower Assimilate effect chain
GamePipeline->>Battlefield: Move graveyard card and apply counter and type override
Battlefield-->>GamePipeline: Return updated object characteristics
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
|
@coderabbitai review |
✅ Action performedReview finished.
|
|
Generated for head Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/engine/tests/integration/borg_queen_assimilate.rs (1)
344-360: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTest 3c is a parser unit test placed in the integration suite.
assimilate_without_a_graveyard_target_stays_unimplementedcallsparse_effectand asserts on AST shape only. It never drives the engine. The module docs at lines 18-21 state that AST-shape coverage lives inparser/oracle_effect/tests.rs, and the stack outline assigns parser assertions to layer 2. Move this test next to the other parser assertions so the integration module stays runtime-only.🤖 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/tests/integration/borg_queen_assimilate.rs` around lines 344 - 360, Move the parser-only test assimilate_without_a_graveyard_target_stays_unimplemented from the integration suite into parser/oracle_effect/tests.rs alongside the existing AST-shape assertions. Preserve both parse_effect cases and their expected Effect::Unimplemented and Effect::ChangeZone matches, leaving the integration module focused on runtime behavior.crates/engine/src/parser/oracle_effect/imperative.rs (1)
22300-22326: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a negative case for the graveyard zone gate.
The test covers the
tag("assimilate ")word boundary. It does not cover the second fail-closed branch inparse_assimilate_target: theextract_in_zone() == Some(Zone::Graveyard)gate. A non-graveyard target phrase must not become anAssimilatenode, because the lowering models only the graveyard reanimation shape. Add one case so a future change to that gate cannot silently lower an unmodeled phrasing.✅ Proposed additional negative case
for text in [ "assimilation", "assimilates target creature card from an opponent's graveyard", "assimilation aegis target creature card from an opponent's graveyard", + // The graveyard gate: a battlefield-scoped phrase is a shape this + // production does not model, so it must stay unsupported. + "assimilate target creature an opponent controls", ] {🤖 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/imperative.rs` around lines 22300 - 22326, Add a negative assertion in the assimilation_is_not_assimilate test for an assimilate target phrase whose extract_in_zone() is not Some(Zone::Graveyard), and verify it does not produce ImperativeFamilyAst::Assimilate. Keep the existing word-boundary negatives and positive real phrase unchanged.
🤖 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/tests/integration/borg_queen_assimilate.rs`:
- Around line 428-461: Update
crates/engine/tests/integration/borg_queen_assimilate.rs#L428-L461 to move
victim A through the production ProposedEvent::ZoneChange replacement-aware path
for both leaving and returning, then resolve and assert the returned permanent
using its post-move ObjectId rather than victim_a. Update
crates/engine/tests/integration/borg_queen_assimilate.rs#L496-L498 to route Borg
Queen’s destruction through the same production path before the
source-independence assertions.
- Around line 292-333: Update the two assimilation tests around
runner.cast(borg_queen).resolve() so they explicitly attempt to target the
staged land or own-graveyard creature, forcing trigger target selection. Assert
that the engine rejects the illegal target, or inspect the trigger prompt and
verify that the candidate is absent from its legal target set; retain the
existing cast-resolution and graveyard assertions.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 22300-22326: Add a negative assertion in the
assimilation_is_not_assimilate test for an assimilate target phrase whose
extract_in_zone() is not Some(Zone::Graveyard), and verify it does not produce
ImperativeFamilyAst::Assimilate. Keep the existing word-boundary negatives and
positive real phrase unchanged.
In `@crates/engine/tests/integration/borg_queen_assimilate.rs`:
- Around line 344-360: Move the parser-only test
assimilate_without_a_graveyard_target_stays_unimplemented from the integration
suite into parser/oracle_effect/tests.rs alongside the existing AST-shape
assertions. Preserve both parse_effect cases and their expected
Effect::Unimplemented and Effect::ChangeZone matches, leaving the integration
module focused on runtime behavior.
🪄 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: 70203c2c-24d8-47e0-b7ae-70d94b1bcc71
📒 Files selected for processing (6)
crates/engine/src/game/gap_analysis.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/tests/integration/borg_queen_assimilate.rscrates/engine/tests/integration/main.rs
| #[test] | ||
| fn assimilate_finds_no_target_when_the_opponent_graveyard_holds_only_a_land() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let land = scenario.add_land_to_graveyard(P1, "Wastes").id(); | ||
| let borg_queen = borg_queen_in_hand(&mut scenario); | ||
| let mut runner = scenario.build(); | ||
| seed_hostile_creature_types(&mut runner); | ||
|
|
||
| let outcome = runner.cast(borg_queen).resolve(); | ||
|
|
||
| // Positive reach-guard: the cast resolved. | ||
| outcome.assert_zone(&[borg_queen], Zone::Battlefield); | ||
| // CR 115.2: a land card is not a legal `target creature card`. | ||
| outcome.assert_zone(&[land], Zone::Graveyard); | ||
| } | ||
|
|
||
| /// 3b. CR 108.3: the `Owned { controller: Opponent }` leg. A creature card in | ||
| /// P0's OWN graveyard is not a legal target. | ||
| /// | ||
| /// This case also passes at BASE_SHA (where nothing moves at all), so it is a | ||
| /// GUARD against a future filter regression, not a revert-failing test. The | ||
| /// paired positive reach-guard keeps it from being vacuous about the cast. | ||
| #[test] | ||
| fn assimilate_cannot_take_a_card_from_its_own_controllers_graveyard() { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let own_card = scenario | ||
| .add_creature_to_graveyard(P0, "Own Graveyard Wizard", 2, 2) | ||
| .with_subtypes(vec!["Human", "Wizard"]) | ||
| .id(); | ||
| let borg_queen = borg_queen_in_hand(&mut scenario); | ||
| let mut runner = scenario.build(); | ||
| seed_hostile_creature_types(&mut runner); | ||
|
|
||
| let outcome = runner.cast(borg_queen).resolve(); | ||
|
|
||
| // Positive reach-guard: the cast resolved. | ||
| outcome.assert_zone(&[borg_queen], Zone::Battlefield); | ||
| // CR 108.3: "an opponent's graveyard" restricts by OWNERSHIP. | ||
| outcome.assert_zone(&[own_card], Zone::Graveyard); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
fd -t f 'scenario' crates/engine/src --exec ast-grep outline {} --items all
rg -n -C6 --type=rust 'fn target_objects|fn resolve\b|legal_targets|fn cast\b' crates/engine/src/game/scenario*
rg -rn --type=rust -C8 'target_objects\(&\[\]\)|no legal target' crates/engine/tests/integration | head -80Repository: phase-rs/phase
Length of output: 38293
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== borg_queen_assimilate relevant test region =="
sed -n '260,350p' crates/engine/tests/integration/borg_queen_assimilate.rs
echo
echo "== ScenarioResult and trigger legality fields near waiting_for =="
sed -n '3530,3600p' crates/engine/src/game/scenario.rs
sed -n '3316,3350p' crates/engine/src/game/scenario.rs
echo
echo "== trigger target handling reduction =="
rg -n -C10 'WaitingFor::TriggerTargetSelection|ChooseTarget|selected_target|selected_targets' crates/engine/src | head -200
echo
echo "== assimilate implementation references =="
rg -n -C8 'assimilate|target creature card|creature card|owned|Opponent' crates/engine/src --type=rust | head -260Repository: phase-rs/phase
Length of output: 48533
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ScenarioResult fields/impl =="
rg -n -C8 '^pub struct ScenarioResult|impl ScenarioResult|final_waiting_for|waiting_for|target' crates/engine/src/game/scenario.rs | head -220
echo
echo "== relevant integration awaiting target selection patterns =="
rg -n -C6 'TriggerTargetSelection|target_slots|outcome\.assert_zone|runner\.(act|act_or|resolve_top|advance_to_priority_window|pass_both_players|final_waiting_for)' crates/engine/tests/integration/borg_queen_assimilate.rs crates/engine/tests/integration/rules/targeting.rs | head -240
echo
echo "== target filter equality/access methods in actions/game state =="
rg -n -C4 'impl.*WaitingFor|current_legal_targets|select.*target|legal_targets|current_target|targets_objects' crates/engine/src/types/game_state.rs crates/engine/src/types/actions.rs crates/engine/src/game | head -260Repository: phase-rs/phase
Length of output: 9560
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== ScenarioResult fields/impl =="
sed -n '3560,3645p' crates/engine/src/game/scenario.rs
printf '\nScenarioResult waiting_for references:\n'
rg -n -C3 'pub fn final_waiting_for|fn waiting_for|waiting_for' crates/engine/src/game/scenario.rs crates/engine/tests/integration/rules/targeting.rs | head -180
echo
echo "== TriggerTargetSelection handling in scenario engine loop =="
sed -n '3308,3380p' crates/engine/src/game/scenario.rs
echo
echo "== targeted trigger integration examples =="
sed -n '35,120p' crates/engine/tests/integration/rules/targeting.rs
sed -n '1580,1620p' crates/engine/tests/integration/borg_queen_assimilate.rs 2>/dev/null || true
echo
echo "== SpellCast target_objects implementation =="
sed -n '2136,2168p' crates/engine/src/game/scenario.rs
echo
echo "== deterministic probe: no declared object targets can still auto-select from legal set =="
python3 - <<'PY'
data = """
// CR 601.2c: mandatory target is picked from the first matching declared
// target in `target_objects`. If there are no declared object targets, pick
// legal_auto_target from the legal set. If none, decline the optional
// target but reject a required target with no legal values.
"""
print(data.strip())
print("contains 'target_objects' =", "target_objects" in data)
print("contains 'legal_auto_target' =", "legal_auto_target" in data)
PYRepository: phase-rs/phase
Length of output: 25549
Test 3 also does not prove the illegal graveyard cards were rejected.
runner.cast(borg_queen).resolve() has no declared target objects, so it does not force trigger target selection against the staged candidate. Add the land or own_card as an intended target and assert the engine rejects it, or inspect the trigger prompt’s legal target set.
🤖 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/tests/integration/borg_queen_assimilate.rs` around lines 292 -
333, Update the two assimilation tests around runner.cast(borg_queen).resolve()
so they explicitly attempt to target the staged land or own-graveyard creature,
forcing trigger target selection. Assert that the engine rejects the illegal
target, or inspect the trigger prompt and verify that the candidate is absent
from its legal target set; retain the existing cast-resolution and graveyard
assertions.
Source: Path instructions
| // 4.3 CR 400.7: victim A leaving the battlefield prunes ONLY its own effect. | ||
| // Victim B's `Artifact` here is the DISCRIMINATING leg for | ||
| // `AddType { Artifact }` — victim B is printed as a plain Goblin creature. | ||
| let mut events = Vec::new(); | ||
| move_to_zone(runner.state_mut(), victim_a, Zone::Graveyard, &mut events); | ||
| relayer(&mut runner); | ||
| assert!( | ||
| has_subtype(&runner, victim_b, "Borg"), | ||
| "CR 400.7: pruning victim A's effect must not touch victim B's" | ||
| ); | ||
| assert!( | ||
| has_core_type(&runner, victim_b, CoreType::Artifact), | ||
| "AddType{{Artifact}} is the only reason a printed Goblin creature is an artifact" | ||
| ); | ||
|
|
||
| // 4.4 CR 400.7: victim A returns as a NEW object with no memory of the | ||
| // override, paired with the positive reach-guard that it really is back on | ||
| // the battlefield. | ||
| let mut events = Vec::new(); | ||
| move_to_zone(runner.state_mut(), victim_a, Zone::Battlefield, &mut events); | ||
| relayer(&mut runner); | ||
| assert_eq!( | ||
| runner.state().objects[&victim_a].zone, | ||
| Zone::Battlefield, | ||
| "reach-guard: victim A is back on the battlefield" | ||
| ); | ||
| assert!( | ||
| !has_subtype(&runner, victim_a, "Borg"), | ||
| "CR 400.7: the returned object is a new object and is not Borg" | ||
| ); | ||
| assert!( | ||
| has_subtype(&runner, victim_a, "Human"), | ||
| "CR 400.7: the returned object has its printed creature types back" | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Zone changes in tests 4 and 5 bypass the production pipeline. Both sites call zones::move_to_zone directly instead of routing through the replacement-aware ProposedEvent::ZoneChange path. Effect pruning on a zone change is the behavior under test in both places, so the assertions can pass or fail for reasons that do not occur in a real game.
crates/engine/tests/integration/borg_queen_assimilate.rs#L428-L461: route victim A's death and return through the production zone-change path, and resolve the returned permanent by its post-move identity rather than reusing the originalvictim_aObjectId.crates/engine/tests/integration/borg_queen_assimilate.rs#L496-L498: route Borg Queen's destruction through the same production path so leave-the-battlefield handling runs before the source-independence assertions.
📍 Affects 1 file
crates/engine/tests/integration/borg_queen_assimilate.rs#L428-L461(this comment)crates/engine/tests/integration/borg_queen_assimilate.rs#L496-L498
🤖 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/tests/integration/borg_queen_assimilate.rs` around lines 428 -
461, Update crates/engine/tests/integration/borg_queen_assimilate.rs#L428-L461
to move victim A through the production ProposedEvent::ZoneChange
replacement-aware path for both leaving and returning, then resolve and assert
the returned permanent using its post-move ObjectId rather than victim_a. Update
crates/engine/tests/integration/borg_queen_assimilate.rs#L496-L498 to route Borg
Queen’s destruction through the same production path before the
source-independence assertions.
Source: Path instructions
|
Thanks — both reviewed against the code. Neither indicates a product defect, and each premise needs a correction; details below so a future reader doesn't re-litigate. 1. Tests 3a/3b "do not prove the illegal graveyard cards were rejected" — the premise doesn't hold. Neither test declares So the current behaviour is: no card is a legal target → the trigger is removed per CR 603.3d → the window never raises → the card stays in the graveyard. If the That said, your suggestion is a legibility win — declaring the illegal card and asserting rejection states the intent directly instead of relying on a panic. Taking it as a follow-up. 2. Tests 4/5 "bypass the production pipeline" — the pruning under test is in
On re-resolving by post-move identity: this engine reuses Disposition. Both are test-strength improvements on a change whose correctness is independently established: |
Lower
assimilate <target>to the reanimate-then-retype chain the enginealready ships: move the targeted card from an opponent's graveyard to the
battlefield under your control with a +1/+1 counter, then install a
permanent layer-4 type override making it a Borg artifact creature that
loses its other creature types.
The keyword action's definition arrives only as reminder text, which is
stripped before the parser runs, so it is encoded in lowering rather than
parsed -- the same reason Recruit is a parser IR node (CR 701.70a).
CR 205.1b: an effect making an object a "[creature type or types] artifact
creature" retains all prior card types, supertypes, and non-creature
subtypes, and replaces only the creature types. So the lowering emits
additive AddTypes plus a creature-set-scoped subtype replacement, never
SetCardTypes -- an assimilated legendary artifact enchantment creature
keeps Legendary and Enchantment.
All four modifications ride one StaticDefinition because they are all
layer 4 (CR 613.1d), so written order decides: RemoveAllSubtypes{Creature}
must precede AddSubtype{Borg} or the wipe erases Borg. Split across
separate definitions they would instead order by CR 613.7 timestamp.
Duration is explicitly Permanent (CR 611.2a): the GenericEffect fallback
is UntilEndOfTurn, which would silently expire the type change at cleanup
without failing any shape assertion.
originis None, matching Ashen Powder and Puppeteer Clique for the same"from an opponent's graveyard" phrase. Some(Graveyard) would additionally
flip AI reanimation detection for this card alone while mechanically
identical cards stayed unflipped.
AddType{Creature} is required rather than defensive: per CR 613.7n a
permanent's own static ability receives an earlier relative timestamp than
a characteristic-setting effect from the same resolution, so a
devotion-gated RemoveType{Creature} would otherwise win.
Registers the verb in gap_analysis::IMPERATIVE_EXTRA_VERBS, mirroring
recruit, which is likewise an anchored pre-dispatch nom probe.
Summary by CodeRabbit
New Features
assimilateaction.Bug Fixes
Tests