fix(engine): flush layers before combat damage to use evaluated power (CR 510.1a)

[nickmopen]

Summary

• A creature buffed by +1/+1 counters (e.g. 8 counters on a base-1/3) was dealing combat damage equal to its base power (1) instead of its layer-evaluated power (9), causing planeswalkers to lose only 1 loyalty counter instead of the full damage amount.
• Root cause: commit_attackers (combat.rs) marks layers_dirty = Full when attackers are declared, but flush_layers in run_post_action_pipeline runs after handle_priority_pass → auto_advance → resolve_combat_damage has already read obj.power via combat_damage_amount — so stale base power was used.
• Fix: call flush_layers in auto_advance Phase::CombatDamage, immediately before resolve_combat_damage, so combat_damage_amount always reads the layer-evaluated power (CR 510.1a + CR 613.4c layer 7c).

Fixes #2352

Design

One call added to turns.rs:

Phase::CombatDamage => {
    // CR 510.1a + CR 613.4c: flush layers so combat_damage_amount reads
    // evaluated power (base + counters), not stale base power.
    super::layers::flush_layers(state);
    if let Some(waiting) = combat_damage::resolve_combat_damage(state, events) { ...

The fix intentionally lives in turns.rs rather than inside resolve_combat_damage: placing it at the call site keeps resolve_combat_damage free of layer-system side-effects so unit tests that construct combat state directly (setting keywords, assigns_damage_as_though_unblocked, etc.) are not disrupted by an unconditional flush resetting their live fields.

Tests

• New regression test attacker_pt_counters_deal_full_power_damage_to_planeswalker: creates a 1/3 creature with 8 +1/+1 counters attacking Professor Onyx (10 loyalty), marks layers dirty to reproduce the production timing, flushes layers (mirroring the turns.rs fix), then asserts the planeswalker loses 9 loyalty counters (10 → 1) instead of 1.

Verification

• cargo fmt --all → clean
• cargo test -p engine "game::combat_damage" → 60/60 pass (all trample, deathtouch, first-strike, interactive-assignment, and planeswalker-damage variants)

Notes

No new types or public API changes. The fix is a single flush_layers call in the production turn-advance path; it is idempotent when layers are already clean (no-op cost on the common path).

[gemini-code-assist]

Code Review

This pull request addresses two main areas: it ensures combat damage calculations use layer-evaluated power (such as from +1/+1 counters) instead of stale base power by flushing layers in Phase::CombatDamage, and it adds parsing support and tests for Aura target legs with artifact token subtypes (e.g., 'Enchant creature or Food'). Feedback is provided on the parser changes to avoid eager heap allocations in the nom combinators by replacing value with lazy .map() evaluation.

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]

[MEDIUM] Eager heap allocation in value combinator

Using nom::combinator::value with a value that allocates (like TypeFilter::Subtype("...".to_string())) causes the allocation to happen eagerly on every call to parse_enchant_type_leg, even if the parser doesn't match that arm. Since alt evaluates parsers sequentially, this results in multiple unnecessary heap allocations on every parse attempt.

Using .map() lazily allocates the String only when the corresponding tag successfully matches.

Suggested change

	value(TypeFilter::Subtype("Food".to_string()), tag("food")),
	value(TypeFilter::Subtype("Treasure".to_string()), tag("treasure")),
	value(TypeFilter::Subtype("Clue".to_string()), tag("clue")),
	value(TypeFilter::Subtype("Blood".to_string()), tag("blood")),
	value(TypeFilter::Subtype("Map".to_string()), tag("map")),
	value(TypeFilter::Subtype("Gold".to_string()), tag("gold")),
	tag("food").map(|_| TypeFilter::Subtype("Food".to_string())),
	tag("treasure").map(|_| TypeFilter::Subtype("Treasure".to_string())),
	tag("clue").map(|_| TypeFilter::Subtype("Clue".to_string())),
	tag("blood").map(|_| TypeFilter::Subtype("Blood".to_string())),
	tag("map").map(|_| TypeFilter::Subtype("Map".to_string())),
	tag("gold").map(|_| TypeFilter::Subtype("Gold".to_string())),

References

1. Avoid eager heap allocations on hot paths (such as parser combinators) by using lazy evaluation (e.g., .map()) instead of eager combinators like value().

[mike-theDude]

Architecture Review

[MED] Regression test does not protect the production fix line. Evidence: crates/engine/src/game/combat_damage.rs:3414 — attacker_pt_counters_deal_full_power_damage_to_planeswalker calls crate::game::layers::flush_layers(&mut state) directly and then resolve_combat_damage, never driving auto_advance/advance_phase. Why it matters: the test passes by mirroring the fix in its own body, so deleting the new flush_layers call in turns.rs:1641 leaves the test green — it does not guard the regression it claims to. Suggested fix: drive the live advance_phase/auto_advance pipeline into Phase::CombatDamage (the repo already has "RUNTIME test driving advance_phase" precedents at turns.rs:2145/2211/2293) so the assertion fails if the production flush is removed.

[MED] Non-atomic PR: an unrelated parser feature is bundled with the combat-damage fix. Evidence: crates/engine/src/parser/oracle_nom/enchant.rs:50-59 and the new enchant-subtype tests in crates/engine/src/parser/oracle_keyword.rs add "Enchant creature or Food/Treasure/Clue/Blood/Map/Gold" support, while the PR title/body describe only the layer-flush fix for combat damage. Why it matters: two independent changes in one PR complicate review, bisection, and revert, and the parser change is invisible in the PR summary. Suggested fix: split the enchant artifact-subtype legs into their own PR (or at minimum document them in the PR body).

[LOW] Gemini's value() eager-allocation comment is correct in isolation but mis-calibrated and inconsistent with the established pattern. Evidence: crates/engine/src/parser/oracle_nom/enchant.rs:45-49 — the immediately-preceding basic-land legs already use value(TypeFilter::Subtype("Forest".to_string()), tag(...)), the identical idiom the new lines follow. Why it matters: switching only the new lines to .map() would make the combinator block internally inconsistent, and this is a cold card-load parse path, not a game hot loop, so the allocation cost is negligible — the bot's MEDIUM rating overstates it. Suggested fix: keep the new lines consistent with the surrounding value() legs, or convert the whole alt block to .map() in a dedicated cleanup if the perf idiom is desired project-wide.

Context verified (not findings): all CR annotations check out against docs/MagicCompRules.txt — 510.1a, 510.1, 510.2, 613.4c, 120.3c, 702.5a, 205.3g. The turns.rs call-site flush seam (vs. inside resolve_combat_damage) is justified, and the interactive-assignment re-entry paths in engine_combat.rs (427/571/641) are not exposed to stale power because the initial CombatDamage entry already flushed.

[matthewevans]

Pushed maintainer follow-up b9157e8f8 to address the review comments.

• Replaced the direct resolve_combat_damage regression with a turns.rs runtime test that drives auto_advance through Phase::CombatDamage.
• The new test starts with stale base power and dirty layers, then asserts the planeswalker loses 9 loyalty only if the production CombatDamage pre-flush runs.
• The unrelated enchant parser bundle mentioned in the review is no longer present in the current PR diff; current diff is limited to combat_damage.rs and turns.rs.

Verification:

• cargo fmt --all
• git diff --check
• pre-commit parser combinator gate: passed
• CR citations verified locally against docs/MagicCompRules.txt: 306.5b, 510.1a, 120.3c, 613.4c

Broad Rust validation is left to GitHub CI for this worktree push; the main workspace has unrelated tracked edits, so I did not switch it onto this PR for Tilt.