feat(card): implement Trench Behemoth landfall → forced attack

[Whovencroft]

Summary

Extend try_parse_attack_if_able to recognize "attacks during its controller's next combat phase if able" as a new duration arm: Duration::UntilNextStepOf { step: EndCombat, player: Controller }.

Cards unlocked: Trench Behemoth, and any creature whose triggered ability forces a target to attack during its controller's next combat phase (the same MustAttack static already works for "this turn if able" and "this combat if able" durations).

Files changed

File	Change
crates/engine/src/parser/oracle_effect/imperative.rs	Add new duration arm to try_parse_attack_if_able bare-form parser: "during its controller's next combat phase if able" → Duration::UntilNextStepOf { step: Phase::EndCombat, player: PlayerScope::Controller }
crates/engine/tests/integration/trench_behemoth_landfall_force_attack.rs	New integration test verifying the landfall trigger parses to GenericEffect { MustAttack } with the correct duration
crates/engine/tests/integration/main.rs	Register the new test module

Anchored on

• crates/engine/src/parser/oracle_effect/imperative.rs:11786 — existing "this turn if able" and "this combat if able" duration arms in the same alt(()) combinator (same position in the bare-form parser)
• crates/engine/src/parser/oracle_effect/subject.rs:4426 — analogous Duration::UntilNextStepOf { step: Phase::Untap, player: PlayerScope::Controller } for "doesn't untap during its controller's next untap step" (same duration shape, different step)

Gate A

$ ./scripts/check-parser-combinators.sh
(exit 0 — no violations)

Verification

• cargo fmt --all — no changes
• ./scripts/check-parser-combinators.sh — exit 0
• Local cargo check blocked by network timeout; CI will verify full compilation

CR annotations verified

CR 508.1d: A creature that must attack does so during the declare
attackers step if able.

CR 502.3 (analogous): "doesn't untap during its controller's next
untap step" uses the same UntilNextStepOf duration pattern.

CR 603.1: Triggered abilities have a trigger condition and an effect.

Track

Non-developer (CI runs full verification).

Test description

trench_behemoth_landfall_trigger_parses_must_attack_next_combat: Parses the Trench Behemoth landfall trigger Oracle text through parse_oracle_text, verifies exactly one trigger is produced, the execute effect is GenericEffect (not Unimplemented), contains a MustAttack static, and has Duration::UntilNextStepOf { step: EndCombat, player: Controller }. Would fail if the new duration arm were reverted (predicate would not match any existing arm and fall through to Unimplemented).

trench_behemoth_full_oracle_no_unimplemented_trigger: Reach-guard parsing the full two-ability Oracle text and asserting no trigger produces an Unimplemented execute effect.

[gemini-code-assist]

Code Review

This pull request adds support for the Trench Behemoth landfall trigger by introducing a new Duration::UntilNextStepOf variant to the parser and providing comprehensive integration tests. The reviewer identified that the CR citations in the new code and tests incorrectly referenced CR 502.3 instead of CR 502.2, which violates the project's mandatory CR annotation style guide (R6).

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.

[gemini-code-assist]

The CR citation CR 502.3 is incorrect for the analogous untap step rule. In the Magic Comprehensive Rules, CR 502.3 describes priority during the untap step, whereas CR 502.2 describes effects that keep permanents from untapping ("Normally, all of a player's permanents untap, but effects can keep one or more of a player's permanents from untapping."). Please update the citation to CR 502.2 to adhere to the mandatory and verified CR annotations rule (R6).

Suggested change

	// CR 508.1d + CR 502.3: "attacks during its controller's next combat
	// CR 508.1d + CR 502.2: "attacks during its controller's next combat

References

1. Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. ^(link)

[gemini-code-assist]

The CR citation CR 502.3 is incorrect for the analogous untap step rule. In the Magic Comprehensive Rules, CR 502.3 describes priority during the untap step, whereas CR 502.2 describes effects that keep permanents from untapping. Please update the citation to CR 502.2 to adhere to the mandatory and verified CR annotations rule (R6).

Suggested change

	//! - CR 502.3 (analogous): Duration pattern "during its controller's next
	//! - CR 502.2 (analogous): Duration pattern "during its controller's next

References

1. Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. ^(link)

[gemini-code-assist]

The CR citation CR 502.3 is incorrect for the analogous untap step rule. In the Magic Comprehensive Rules, CR 502.3 describes priority during the untap step, whereas CR 502.2 describes effects that keep permanents from untapping. Please update the citation to CR 502.2 to adhere to the mandatory and verified CR annotations rule (R6).

Suggested change

	/// CR 508.1d + CR 502.3: The landfall trigger's execute effect must contain a
	/// CR 508.1d + CR 502.2: The landfall trigger's execute effect must contain a

References

1. Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. ^(link)

[chatgpt-codex-connector]

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 1ba09472c0

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

• Open a pull request for review
• Mark a draft as ready
• Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

[chatgpt-codex-connector]

Expire the next-combat MustAttack duration

When Trench Behemoth targets a creature outside that creature controller's combat, this new duration is registered as a transient continuous effect, but there is no corresponding prune for Duration::UntilNextStepOf { step: Phase::EndCombat, player: Controller }: prune_end_of_combat_effects only removes UntilEndOfCombat, and the existing UntilNextStepOf pruners only handle End and Untap. The effect will therefore stay in state.transient_continuous_effects after the controller's next combat and keep forcing the creature to attack in later combats, rather than expiring after the single required combat phase.

Useful? React with 👍 / 👎.

[github-actions]

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

2 card(s) · ability/MustAttack · added: MustAttack (affects=parent target, conditional=2 resolution this turn, duration=until next end combat (you), grants=MustAttack, target=triggering source)

Examples: A-Sizzling Soloist, Sizzling Soloist

2 card(s) · ability/attack · removed: attack (conditional=2 resolution this turn)

Examples: A-Sizzling Soloist, Sizzling Soloist

1 card(s) · ability/MustAttack · added: MustAttack (affects=parent target, duration=until next end combat (you), grants=MustAttack, target=opponent controls creature)

Examples: Trench Behemoth

1 card(s) · ability/attack · removed: attack

Examples: Trench Behemoth

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

[matthewevans]

Thanks for this one. The hard part is right, so let me start there.

What's correct:

• The parse target matches the real card. Trench Behemoth reads "Landfall — Whenever a land you control enters, target creature an opponent controls attacks during its controller's next combat phase if able."
• CR 508.1d is exactly the right anchor: "The active player checks each creature they control to see whether it's affected by any requirements (effects that say a creature attacks if able)."
• The new arm sits inside the existing alt(()) in try_parse_attack_if_able (crates/engine/src/parser/oracle_effect/imperative.rs:11791) next to its siblings "this turn if able" and "this combat if able", so it extends the local combinator pattern rather than introducing string dispatch.
• The second test, a reach-guard asserting the full Oracle text yields no Unimplemented trigger effects, is a good habit. Please keep it.

---

Blocking (HIGH): the new Duration has no expiry path, so the requirement never ends

The arm emits Duration::UntilNextStepOf { step: Phase::EndCombat, player: PlayerScope::Controller }. Expiry in crates/engine/src/game/layers.rs is hardcoded per duration shape rather than generic over step:

• prune_end_of_combat_effects (layers.rs:125) does .retain(|e| e.duration != Duration::UntilEndOfCombat), an exact-equality check.
• prune_until_next_end_step_effects (layers.rs:142) matches UntilNextStepOf { step: Phase::End, player: Controller }.
• prune_controller_untap_step_effects (layers.rs:493) matches UntilNextStepOf { step: Phase::Untap, .. }.

Phase::EndCombat appears nowhere in layers.rs. The Phase::EndCombat arm of advance_phase (crates/engine/src/game/turns.rs:2555) calls only prune_end_of_combat_effects, whose exact-equality check does not match your shape. Nothing removes this transient continuous effect.

Concrete failure: you play a land, the landfall trigger resolves, and the targeted creature gains a MustAttack requirement that is never pruned. It is compelled to attack during its controller's next combat, and during the one after that, and every combat thereafter. The card grants a one-combat requirement per CR 508.1d, so this is a rules violation with unbounded duration.

One note so you don't reach for the wrong fix: neither existing variant is a drop-in. Duration::UntilEndOfCombat is pruned at end of combat, which for a landfall trigger resolving on your turn means it expires before the opponent's combat ever arrives, leaving the effect inert instead of permanent. A correct implementation needs a real expiry path for the "controller's next combat phase" shape, most likely a new pruner keyed on Phase::EndCombat invoked at the end-of-combat step, together with whatever ensures the requirement is not consumed by a combat that happens before the controller's next one.

Blocking (MED): the tests cannot observe either failure mode

crates/engine/tests/integration/trench_behemoth_landfall_force_attack.rs asserts parser AST shape only: that the trigger's execute effect contains MustAttack with Duration::UntilNextStepOf { step: EndCombat, player: Controller }, plus the Unimplemented reach-guard.

The PR title says "implement ... forced attack", which is a claim about behavior. Nothing in the test plays a land, declares attackers, checks the creature is actually compelled to attack, or checks the requirement is gone afterwards.

Please add a runtime test that drives the pipeline: resolve the landfall trigger targeting an opponent's creature, advance to that opponent's combat, assert the creature must attack (declaring no attackers is rejected, or the creature appears in the required-attackers set), then advance past that combat to the opponent's next combat and assert the requirement is gone. That second half is precisely what would have caught the pruning bug above.

House standard worth stating plainly: a parser-shape assertion proves the AST, and a runtime assertion proves the behavior. This repo has been bitten before by AST fields that parse correctly and are never consumed. A newly introduced Duration value in particular needs a runtime test proving something reads it.

---

Non-blocking

• CR 502.3 is cited in both the code comment (imperative.rs:11791) and the test doc comment as an analogy for the duration pattern. I grep-verified it: "Third, the active player determines which permanents they control will untap. Then they untap them all simultaneously." It governs the untap step, and this code implements no untap rule. Citing a rule as an analogy makes grep -rn "CR 502.3" misleading for the next person auditing untap behavior. Please drop it, or cite a combat-phase rule instead.
• Minor, no action needed here: oracle_nom/duration.rs owns parse_duration and already constructs Duration::UntilNextStepOf, while the new phrase is hardcoded as a tag in imperative.rs. That matches its sibling arms, so it is fine as written. If a second card ever needs "during its controller's next [step]", that belongs in the shared duration combinator.

---

The parse is right and the diagnosis is sound. The gap is that the duration you introduced has no expiry path, and a runtime test is how that becomes visible.

[matthewevans]

Thanks for turning this around so quickly — and for fixing the right thing rather than the easy thing.

What you got right

The HIGH is resolved, and the hard part of it is correct. prune_controller_end_combat_step_effects (crates/engine/src/game/layers.rs:141) expires Duration::UntilNextStepOf { step: Phase::EndCombat, player: PlayerScope::Controller }, and the keying is the subtle part: at layers.rs:156-163 you check the affected object's controller (e.affected → TargetFilter::SpecificObject { id } → obj.controller == active_player), not the effect's controller. That is exactly what makes the requirement survive Trench Behemoth's controller's own combat and expire only at the end of the target creature's controller's combat. Getting that backwards would have made the whole effect inert, and it is an easy mistake to make.

The wiring is symmetric rather than ad hoc. I verified that prune_end_of_combat_effects is called at exactly four sites, and the new pruner sits immediately after each one: engine_combat.rs:341-342, engine_combat.rs:863-864, turns.rs:209-210, turns.rs:2567-2568. All four are EndCombat transitions, so the doc comment at layers.rs:137 ("Called alongside prune_end_of_combat_effects at the EndCombat phase") is accurate rather than aspirational.

CR 511.3 is right, and I grep-verified it: "As soon as the end of combat step ends, all creatures, battles, and planeswalkers are removed from combat." Apt for this duration. Thanks also for dropping the CR 502.3 analogy I flagged last round.

trench_behemoth_runtime_not_pruned_during_wrong_players_combat is a genuinely good discriminator. Pruning at P0's combat must not remove an effect scoped to P1's creature, and pruning at P1's must. Please keep that test exactly as it is.

Blocking — one item, the same one as last round

Neither runtime test drives the pipeline. Both build the effect by hand with state.add_transient_continuous_effect(...) (trench_behemoth_landfall_force_attack.rs:152 and :200) and then call prune_controller_end_combat_step_effects(...) directly (:177, :220, :234). Nothing in that file plays a land, resolves the trigger, advances a phase, or declares attackers.

So the tests establish two statements that are both true and both insufficient:

1. Given that transient effect exists, MustAttack is honored.
2. Given the pruner is called with the right player, the effect is removed.

Neither implies the card works. Two arrows remain untested:

• Parse → resolve. Nothing proves the landfall trigger, when it resolves, actually installs a transient effect carrying this duration.
• Phase advance → prune. Nothing proves the pruner is reached by real phase machinery. The four call sites you just added are exercised by no test in this PR.

For your reassurance: I checked the resolver seam by hand and it does exist. force_attack.rs destructures duration and passes duration.clone() into add_transient_continuous_effect, and effects/mod.rs matches Effect::GenericEffect carrying StaticMode::MustAttack. I expect an end-to-end test to pass on the first try. The problem is that nothing currently proves it, and the arrow from a parsed AST field to a consumed runtime value is precisely where this codebase has been bitten before.

What I need is one test, not a suite:

1. Trench Behemoth on the battlefield under P0, a creature under P1.
2. Play a land from P0's hand so the landfall trigger resolves, targeting P1's creature.
3. Advance to P1's combat; assert the creature is required to attack.
4. Advance past that combat; assert the requirement is gone.

That single test collapses both arrows into one assertion, and it would have caught the original never-expires bug on its own without any of the static analysis either of us did. .claude/skills/card-test/SKILL.md has the GameScenario / GameRunner recipe.

This is the last item. The pruner itself looks right.

Non-blocking

• layers.rs:164 — the _ => true arm retains any effect whose e.affected is not a TargetFilter::SpecificObject. For this card the affected filter is always a specific object, so nothing is broken today. But an effect carrying this duration with a Typed affected filter would never expire, which is the same never-pruned shape as the bug you just fixed, one level down. Worth a comment naming the assumption, or a debug_assert.
• layers.rs:157-160 — same arm: if the affected object has left the battlefield, state.objects.get(id) yields None, is_some_and yields false, and the effect is retained. Harmless for MustAttack on a dead creature, but it accumulates.

[matthewevans]

The e2e test is correct, and it is telling you the truth. Leaving this blocked, but the failure is good news, not a setback.

trench_behemoth_e2e_landfall_forces_attack_then_expires ... FAILED
panicked at trench_behemoth_landfall_force_attack.rs:337:
  P1's creature must no longer be forced to attack after P1's combat ends (phase advance → pruner pipeline)

One failure out of 10,253 tests, and it is the only test that drives the real pipeline. Read what passed before it:

• the sanity assert that resolving the landfall trigger installs a transient effect: passes
• the assert that the creature is forced to attack during P1's combat: passes
• the assert that the requirement is gone after P1's combat: fails

So the parse → resolve arrow works. The phase-advance → prune arrow does not. Your pruner is correct in isolation, which is what your two earlier tests proved by calling prune_controller_end_combat_step_effects directly. Nothing in real phase advancement reaches it on this path.

That is the exact gap I was pointing at, and it is why the hand-built tests could not have caught it: they mocked away the seam that was broken.

Where I would look first: advance_to_phase(Phase::PostCombatMain) may not traverse end_combat_phase_to_postcombat, which is one of the four sites where you call the pruner. The other three are finish_declare_attackers, handle_empty_attackers, and auto_advance. Worth checking whether the test's manual state fixup (state.phase = Phase::PreCombatMain and friends) skips the transition that owns your call site, and whether combat is entered and exited through a path that hits any of them. If the answer is that the scenario harness jumps phases without running the EndCombat hook, then the fix belongs in whichever transition genuinely owns end-of-combat cleanup, next to prune_end_of_combat_effects, rather than in four places that a real turn may not all visit.

Keep the test exactly as it is. When it goes green, the card works.

[matthewevans]

Approved. Round three landed exactly where it needed to.

The most useful thing in this PR is what happened when the test ran

I asked for an end-to-end test because the two existing runtime tests didn't cover the arrow I was worried about. When you added it, it failed — and it failed on precisely the seam it was written to cover: trench_behemoth_e2e_landfall_forces_attack_then_expires, one failure out of 10,253, panicking at "P1's creature must no longer be forced to attack after P1's combat ends (phase advance → pruner pipeline)". The trigger installed the transient effect, the creature was compelled to attack, and the requirement never expired.

That is the whole argument for the test I asked for. The two earlier tests passed while the behavior under test was broken, because they hand-built the transient effect and called prune_controller_end_combat_step_effects directly. They mocked away the exact seam that was defective. A test that mocks the seam under suspicion cannot fail, no matter how green it is.

The diagnosis was right

You concluded the engine was correct and the test fixture was wrong, instead of reaching for an engine change to turn a red test green. That is the harder call and the correct one.

The fixture called runner.advance_to_phase(Phase::PostCombatMain), which jumps phases without traversing end_combat_phase_to_postcombat — one of the four sites where the pruner actually runs. Replacing it with declare_attackers for the forced creature, then combat_damage(), then a bounded priority-pass loop, drives combat to its end through the real machinery, and the pruner fires. The added declare_attackers call is a bonus: it asserts the requirement is satisfiable, not merely present.

I diffed the final assertion. It is unchanged. Nothing was weakened to make the test pass. 55bfcfe5 touches only the test file, which is the correct blast radius for a fixture bug.

The test now drives the whole pipeline: the creature is built from Oracle text via add_creature_from_oracle, GameAction::PlayLand fires landfall, the trigger resolves targeting P1's creature, a sanity assert confirms a transient effect was installed, the creature is compelled to attack in P1's combat, and after that combat the requirement is gone.

The pruner design

prune_controller_end_combat_step_effects keys on the affected object's controller rather than the effect's controller. That is what lets the requirement survive Trench Behemoth's controller's own combat and expire at the end of the target creature's controller's combat. Getting that backwards would have made the effect inert in the common case, and it is an easy mistake to make. It is wired at exactly the four EndCombat sites where prune_end_of_combat_effects already runs (engine_combat.rs:341, engine_combat.rs:862, turns.rs:209, turns.rs:2566). CR 508.1d and CR 511.3 check out against the rules text.

Two non-blocking notes

Neither of these blocks the merge. Both are worth a follow-up thought.

1. The _ => true arm is the same shape you just fixed, one level down. prune_controller_end_combat_step_effects retains on _ => true for any e.affected that is not TargetFilter::SpecificObject. An effect carrying Duration::UntilNextStepOf { step: Phase::EndCombat } with a Typed affected filter would therefore never expire — which is the never-pruned failure mode you spent this PR eliminating. Nothing is broken today, because this card always names a specific object. But the assumption is load-bearing and currently invisible. A comment naming it, or a debug_assert, would make the next contributor's life easier.

2. The e2e test's bounded loop exits silently. for _ in 0..16 over priority passes will fall through without complaint if PostCombatMain is never reached. It cannot make the test pass vacuously — an unpruned effect still fails the final assert — so this is not a correctness gap. It is a legibility one: if the loop ever stops reaching combat's end, the failure will point at the requirement instead of at the loop. An assertion that the phase actually arrived at PostCombatMain would name the real cause.

Thanks for the persistence through three rounds, and for reading the red test correctly.