Fix Ardenvale Paladin - #6780
Open
keloide wants to merge 3 commits into
Open
Conversation
…s-with replacements
The Adamant cycle parsed its "enters with a +1/+1 counter" clause as a
CR 614.1c replacement but dropped the sentence-initial "If <condition>, "
gate, leaving ReplacementDefinition.condition = None. The engine therefore
gave every copy of these creatures the counter regardless of what mana was
actually spent — silently-wrong behaviour that nothing flagged at runtime.
Coverage reported it as Swallow:Condition_If on six cards.
Root cause: oracle_replacement.rs peeled a TRAILING " unless <cond>" gate
(extract_enters_with_unless_suffix) and a trailing " if <cond>" gate
(extract_enters_with_only_if_suffix), but nothing peeled a LEADING one, and
the payload scan skips everything before "enters with".
Fixed for the class, not the cards:
* extract_enters_with_leading_if_gate mirrors the existing trailing
extractor one-for-one, including its fail-closed tri-state. Condition
recognition delegates to parse_inner_condition (the single authority), so
the ENTIRE static-condition grammar is now reachable from the
sentence-initial position, where previously zero shapes were. An
unrecognised gate returns Unparsed and the clause falls through to
Effect::unimplemented rather than committing a replacement with the gate
erased.
* CR 207.2c ability-word labels are peeled via parse_known_ability_word_name,
which enumerates exactly the CR 207.2c list. CR 207.2d flavour words are
deliberately NOT peeled: the loose 4-word heuristic would peel "Sensational
Save" and regress Scarlet Spider, Ben Reilly to Unparsed.
* CastManaSpentMetric::OfColor parameterises the existing metric axis
(Total | DistinctColors | FromSource) rather than adding a
ReplacementCondition sibling, so the parsed condition flows through the
untouched replacement_condition_from_static into OnlyIfQuantity. Zero new
ReplacementCondition variants, zero new condition-evaluation arms.
* parse_color_mana_spent_threshold reuses parse_amount_threshold, so the
comparator axis (at least N / N or more / N or fewer) is carried rather
than hardcoded to GE.
Coverage: six cards flip to supported (Ardenvale, Embereth, Garenbrig,
Locthwain and Vantress Paladin, plus Necromantic Summons), zero cards
regress. Dust Animus keeps supported=true and gains its gate, so it is
correctly conditional for the first time. Henge Walker ("mana of the same
color" — a max-over-colors metric) and Red and Black Legacy now fail closed
and stay honestly red instead of silently granting their counters.
Measured against a pre-change baseline of all 35516 cards: 0 true->false,
6 false->true, net +6.
CR 614.1c, CR 614.12, CR 207.2c, CR 207.2d, CR 106.3, CR 601.2h, CR 400.7d.
CR 603.4 is deliberately not cited in code: its intervening-if rule is
scoped to triggered abilities, and this gates a replacement effect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
needs-maintainer
Contributor
📝 WalkthroughWalkthroughChangesAdamant parser and runtime flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant ConditionParser
participant GateExtractor
participant GateResolver
participant RuntimeEvaluator
OracleText->>ConditionParser: parse color-specific mana threshold
OracleText->>GateExtractor: extract sentence-initial if gate
ConditionParser->>GateResolver: provide canonical quantity condition
GateExtractor->>GateResolver: provide leading-if outcome
GateResolver->>RuntimeEvaluator: attach resolved replacement condition
RuntimeEvaluator->>RuntimeEvaluator: evaluate color mana and gate payload
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adamant_enters_with_leading_if_gate.rs`:
- Around line 132-368: Extend the integration coverage around the existing cast
helpers and gate assertions to include Adamant cases for blue, black, and green,
with both qualifying and non-qualifying color payments verifying the expected
payload behavior. Add production-pipeline rejection tests for the documented
Henge Walker/Red and Black Legacy unsupported gates, confirming their gated
payloads do not apply unconditionally. Prefer table-driven rows and reuse
existing scenario/cast helpers and assertion patterns.
🪄 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: 11a29477-0ff0-471d-838e-f6351bf05480
📒 Files selected for processing (14)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/coverage.rscrates/engine/src/game/layers.rscrates/engine/src/game/quantity.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/adamant_enters_with_leading_if_gate.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.md
Comment on lines
+132
to
+368
| /// R1 — POSITIVE reach-guard for the whole white family. {W}{W}{W}{W} pays | ||
| /// {3}{W}: white spent = 4 >= 3, so the counter applies. Discriminates | ||
| /// `OfColor` from `DistinctColors` (which is 1 here, below the threshold). | ||
| #[test] | ||
| fn ardenvale_paladin_four_white_applies_counter() { | ||
| let (outcome, paladin) = cast_paladin( | ||
| "Ardenvale Paladin", | ||
| ARDENVALE_PALADIN, | ||
| ManaCostShard::White, | ||
| 2, | ||
| 3, | ||
| &[(ManaType::White, 4)], | ||
| ); | ||
| outcome.assert_counters(paladin, CounterType::Plus1Plus1, 1); | ||
| } | ||
|
|
||
| /// R2 — **THE PRIMARY REVERT DISCRIMINATOR.** One white + three colorless pays | ||
| /// {3}{W}: white spent = 1 < 3, so NO counter. Total mana spent is 4 >= 3, so | ||
| /// this row also discriminates `OfColor` from `CastManaSpentMetric::Total`. | ||
| /// | ||
| /// Drop the leading-if peel and the replacement becomes unconditional → 1 | ||
| /// counter → this assertion fails. Paired reach-guard: R1 above. | ||
| #[test] | ||
| fn ardenvale_paladin_white_below_threshold_no_counter() { | ||
| let (outcome, paladin) = cast_paladin( | ||
| "Ardenvale Paladin", | ||
| ARDENVALE_PALADIN, | ||
| ManaCostShard::White, | ||
| 2, | ||
| 3, | ||
| &[(ManaType::White, 1), (ManaType::Colorless, 3)], | ||
| ); | ||
| outcome.assert_counters(paladin, CounterType::Plus1Plus1, 0); | ||
| } | ||
|
|
||
| /// R6 — pins the comparator (GE, not GT) and the literal threshold 3. Exactly | ||
| /// three white → counter applies; exactly two white → it does not. | ||
| #[test] | ||
| fn ardenvale_paladin_threshold_is_greater_or_equal_three() { | ||
| let (at_threshold, paladin) = cast_paladin( | ||
| "Ardenvale Paladin", | ||
| ARDENVALE_PALADIN, | ||
| ManaCostShard::White, | ||
| 2, | ||
| 3, | ||
| &[(ManaType::White, 3), (ManaType::Colorless, 1)], | ||
| ); | ||
| at_threshold.assert_counters(paladin, CounterType::Plus1Plus1, 1); | ||
|
|
||
| let (below, paladin) = cast_paladin( | ||
| "Ardenvale Paladin", | ||
| ARDENVALE_PALADIN, | ||
| ManaCostShard::White, | ||
| 2, | ||
| 3, | ||
| &[(ManaType::White, 2), (ManaType::Colorless, 2)], | ||
| ); | ||
| below.assert_counters(paladin, CounterType::Plus1Plus1, 0); | ||
| } | ||
|
|
||
| /// R4 — POSITIVE reach-guard for the red family, and proof the color is read | ||
| /// per-card rather than hardcoded to the first card fixed (white). Three red + | ||
| /// one colorless pays {3}{R}: red spent = 3 >= 3 → counter. | ||
| #[test] | ||
| fn embereth_paladin_three_red_applies_counter() { | ||
| let (outcome, paladin) = cast_paladin( | ||
| "Embereth Paladin", | ||
| EMBERETH_PALADIN, | ||
| ManaCostShard::Red, | ||
| 3, | ||
| 1, | ||
| &[(ManaType::Red, 3), (ManaType::Colorless, 1)], | ||
| ); | ||
| outcome.assert_counters(paladin, CounterType::Plus1Plus1, 1); | ||
| } | ||
|
|
||
| /// R3 — the gate reads the card's OWN color. {W}{W}{W}{R} pays Embereth's | ||
| /// {3}{R}: red = 1 < 3 (no counter) even though WHITE = 3 would have passed | ||
| /// Ardenvale's gate, and total = 4 would have passed a `Total` gate. | ||
| /// Paired reach-guard: R4 above. | ||
| #[test] | ||
| fn embereth_paladin_reads_red_not_white() { | ||
| let (outcome, paladin) = cast_paladin( | ||
| "Embereth Paladin", | ||
| EMBERETH_PALADIN, | ||
| ManaCostShard::Red, | ||
| 3, | ||
| 1, | ||
| &[(ManaType::White, 3), (ManaType::Red, 1)], | ||
| ); | ||
| outcome.assert_counters(paladin, CounterType::Plus1Plus1, 0); | ||
| } | ||
|
|
||
| /// R5 — the row that separates `OfColor` from `DistinctColors` at a point where | ||
| /// `DistinctColors` PASSES. {W}{U}{B}{R} pays Embereth's {3}{R}: four distinct | ||
| /// colors (>= 3, so a `DistinctColors` gate would fire) but red = 1 < 3, so the | ||
| /// correct `OfColor` gate does not. Paired reach-guard: R4 above. | ||
| #[test] | ||
| fn embereth_paladin_four_distinct_colors_still_no_counter() { | ||
| let (outcome, paladin) = cast_paladin( | ||
| "Embereth Paladin", | ||
| EMBERETH_PALADIN, | ||
| ManaCostShard::Red, | ||
| 3, | ||
| 1, | ||
| &[ | ||
| (ManaType::White, 1), | ||
| (ManaType::Blue, 1), | ||
| (ManaType::Black, 1), | ||
| (ManaType::Red, 1), | ||
| ], | ||
| ); | ||
| outcome.assert_counters(paladin, CounterType::Plus1Plus1, 0); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // R7 — Dust Animus: a leading-if gate that is NOT a mana-spent threshold. | ||
| // Proves the peel makes the WHOLE `parse_inner_condition` grammar reachable | ||
| // from the sentence-initial position, not just the one new arm. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Cast Dust Animus ({1}{W}) out of an exact pool while controlling | ||
| /// `untapped_lands` untapped and `tapped_lands` tapped Plains. The lands are | ||
| /// never tapped for mana (the pool is pre-staged), so their tap state is | ||
| /// controlled purely by the fixture. | ||
| fn cast_dust_animus(untapped_lands: usize, tapped_lands: usize) -> (CastOutcome, ObjectId) { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let animus = scenario | ||
| .add_creature_to_hand_from_oracle(P0, "Dust Animus", 1, 1, DUST_ANIMUS) | ||
| .with_mana_cost(ManaCost::Cost { | ||
| generic: 1, | ||
| shards: vec![ManaCostShard::White], | ||
| }) | ||
| .id(); | ||
| let mut to_tap = Vec::new(); | ||
| for _ in 0..untapped_lands { | ||
| scenario.add_basic_land(P0, ManaColor::White); | ||
| } | ||
| for _ in 0..tapped_lands { | ||
| to_tap.push(scenario.add_basic_land(P0, ManaColor::White)); | ||
| } | ||
| scenario.with_mana_pool(P0, pool(&[(ManaType::White, 1), (ManaType::Colorless, 1)])); | ||
| let mut runner = scenario.build(); | ||
| for land in to_tap { | ||
| runner.state_mut().objects.get_mut(&land).unwrap().tapped = true; | ||
| } | ||
|
|
||
| assert_gate_is_attached(&runner, animus, "Dust Animus"); | ||
|
|
||
| let outcome = runner.cast(animus).resolve(); | ||
| assert_eq!( | ||
| outcome.zone_of(animus), | ||
| Zone::Battlefield, | ||
| "Dust Animus must resolve onto the battlefield" | ||
| ); | ||
| (outcome, animus) | ||
| } | ||
|
|
||
| /// R7a — POSITIVE reach-guard: five untapped lands satisfies the gate, so Dust | ||
| /// Animus keeps both counter payloads. This is the "the peel did not break the | ||
| /// already-green card" row (Dust Animus is `supported=true` today, but was | ||
| /// silently UNCONDITIONAL — the gate was swallowed). | ||
| #[test] | ||
| fn dust_animus_five_untapped_lands_applies_counters() { | ||
| let (outcome, animus) = cast_dust_animus(5, 0); | ||
| outcome.assert_counters(animus, CounterType::Plus1Plus1, 2); | ||
| assert_eq!( | ||
| outcome.counters(animus, CounterType::Keyword(KeywordKind::Lifelink)), | ||
| 1, | ||
| "the lifelink counter rides the same gated payload" | ||
| ); | ||
| } | ||
|
|
||
| /// R7b — REVERT DISCRIMINATOR (second polarity). Six lands, one of them TAPPED, | ||
| /// leaves four untapped: below the "five or more untapped lands" threshold, so | ||
| /// no counters. Drop the peel and the payload applies unconditionally → 2 +1/+1 | ||
| /// counters → this fails. Also discriminates `FilterProp::Untapped` from a bare | ||
| /// land count: a count-only reading would see six lands and fire. | ||
| /// Paired reach-guard: R7a above. | ||
| #[test] | ||
| fn dust_animus_only_four_untapped_lands_no_counters() { | ||
| let (outcome, animus) = cast_dust_animus(4, 2); | ||
| outcome.assert_counters(animus, CounterType::Plus1Plus1, 0); | ||
| assert_eq!( | ||
| outcome.counters(animus, CounterType::Keyword(KeywordKind::Lifelink)), | ||
| 0, | ||
| "the whole gated payload is suppressed, not just the +1/+1 counters" | ||
| ); | ||
| } | ||
|
|
||
| // --------------------------------------------------------------------------- | ||
| // R8 — the Adamant ABILITY RIDER. The same grammar change re-routes 11 riders | ||
| // from `AbilityCondition::ManaColorSpent` to the generic | ||
| // `QuantityCheck { ManaSpentToCast { .., OfColor } }`. This is the mandatory | ||
| // runtime guard on that AST churn: the observable damage must not move. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| fn cast_slaying_fire(payment: &[(ManaType, usize)]) -> CastOutcome { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let fire = scenario | ||
| .add_spell_to_hand_from_oracle(P0, "Slaying Fire", true, SLAYING_FIRE) | ||
| .with_mana_cost(ManaCost::Cost { | ||
| generic: 2, | ||
| shards: vec![ManaCostShard::Red], | ||
| }) | ||
| .id(); | ||
| scenario.with_mana_pool(P0, pool(payment)); | ||
| let mut runner = scenario.build(); | ||
| assert!( | ||
| !runner.state().objects[&fire] | ||
| .abilities | ||
| .iter() | ||
| .any(|a| matches!(&*a.effect, Effect::Unimplemented { .. })), | ||
| "Slaying Fire must parse with zero Effect::Unimplemented, got {:?}", | ||
| runner.state().objects[&fire].abilities | ||
| ); | ||
| runner.cast(fire).target_player(P1).resolve() | ||
| } | ||
|
|
||
| /// R8 positive: three red mana satisfies the Adamant rider → 4 damage, not 3. | ||
| #[test] | ||
| fn slaying_fire_three_red_deals_four() { | ||
| let outcome = cast_slaying_fire(&[(ManaType::Red, 3)]); | ||
| outcome.assert_life_delta(P1, -4); | ||
| } | ||
|
|
||
| /// R8 negative (paired with the positive above): one red + two colorless is | ||
| /// three TOTAL mana but only one RED, so the rider does not fire → 3 damage. | ||
| /// Discriminates `OfColor` from `Total` on the ability-rider path exactly as R2 | ||
| /// does on the replacement path. | ||
| #[test] | ||
| fn slaying_fire_one_red_deals_three() { | ||
| let outcome = cast_slaying_fire(&[(ManaType::Red, 1), (ManaType::Colorless, 2)]); | ||
| outcome.assert_life_delta(P1, -3); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Cover all color metrics and the fail-closed cases.
The suite proves only white/red accepted paths. Blue, black, and green Adamant gates—and the documented fail-closed Henge Walker/Red and Black Legacy cases—can regress while these tests remain green. Add table-driven cast-pipeline rows for the remaining colors and rejection rows that prove unsupported gates do not apply payloads unconditionally.
As per coding guidelines, reusable building blocks must be tested across their parameter range; as per path instructions, tests must exercise the failure path through the production pipeline.
🤖 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/adamant_enters_with_leading_if_gate.rs`
around lines 132 - 368, Extend the integration coverage around the existing cast
helpers and gate assertions to include Adamant cases for blue, black, and green,
with both qualifying and non-qualifying color payments verifying the expected
payload behavior. Add production-pipeline rejection tests for the documented
Henge Walker/Red and Black Legacy unsupported gates, confirming their gated
payloads do not apply unconditionally. Prefer table-driven rows and reuse
existing scenario/cast helpers and assertion patterns.
Sources: Coding guidelines, Path instructions
Resolve the generated parser-backlog count conflict using current main values. Co-authored-by: @Lcola98 <75585494+Lcola98@users.noreply.github.com>
Member
|
Maintainer update status for current head Next step: grant/update maintainer write access for this fork or apply that merge resolution from a credential with fork push access; then the PR must return to implementation review on its new head before approval. |
Member
|
Update to the prior hold: the initial fork push used an unavailable environment token. Repeating the same already-reviewed local resolution through the authenticated GitHub CLI credential succeeded. Current head is now |
Member
Parse changes introduced by this PR · 22 card(s), 21 signature(s) (baseline: main
|
Member
|
Current-head review hold for Next step: regenerate or reconcile the current-head parse-diff artifact, then this PR will receive the full implementation review before any approval or queue action. |
matthewevans
requested changes
Jul 29, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Changes requested — the implementation is at the right generic seam, but its runtime evidence does not cover the full newly supported color axis.
🟡 Test coverage gap
[MED] CastManaSpentMetric::OfColor { color } is parameterized for all five colors, while the production cast-pipeline integration test exercises only white and red. Evidence: crates/engine/src/types/ability.rs:5428 introduces the generic ManaColor parameter; crates/engine/tests/integration/adamant_enters_with_leading_if_gate.rs:132-245 contains white/red rows only; the current-head CodeRabbit review identifies the same gap. Why it matters: blue, black, or green lowering/resolution can regress without a runtime failure, despite this PR changing their parser output in the parse-diff artifact. Suggested fix: add table-driven qualifying and below-threshold pipeline rows for blue, black, and green, plus cast-pipeline witnesses that the documented unsupported Henge Walker and Red and Black Legac gates do not publish their payloads unconditionally.
Recommendation: request changes for the complete parameter-range runtime matrix; retain the existing shared OfColor design.
Silverflame Ritual's Adamant rider grants a continuous static effect
("creatures you control gain vigilance") rather than an ability-level
effect, so its gate lands on `StaticDefinition.condition` (CR 613 layer
6) instead of the ability's own `condition`. The CI parse-diff bot reads
the ability level only, so that layer move renders as
`conditional: 3+ White spent -> ∅`, which reads like a dropped gate --
the exact bug class this file guards.
It is not a drop: the condition is present and enforced. Verified at
runtime in both polarities, because nothing pinned this subclass. The
existing R8 rider guard (Slaying Fire) cannot catch it -- that payload
is ability-level -- so a future refactor really could drop the static's
condition with every other test here staying green.
Both new tests carry a positive reach-guard asserting the card's UNGATED
first line resolved (the +1/+1 counter), so the "no vigilance" negative
cannot pass vacuously on a spell that never resolved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/adamant_enters_with_leading_if_gate.rs`:
- Around line 369-466: Extend the static-grant coverage around
cast_silverflame_ritual and the R9 tests beyond white by adding a
different-color positive/negative gate case, preserving the counter assertion
and verifying vigilance remains gated. Add a production-pipeline regression
fixture with a malformed or unparseable StaticDefinition.condition and assert
the static grant fails closed rather than applying unconditionally.
🪄 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: 401c939a-88b0-4cb3-bf03-47b87a3209a9
📒 Files selected for processing (1)
crates/engine/tests/integration/adamant_enters_with_leading_if_gate.rs
Comment on lines
+369
to
+466
|
|
||
| // --------------------------------------------------------------------------- | ||
| // R9 — the Adamant rider whose payload is a CONTINUOUS STATIC GRANT, not an | ||
| // ability-level effect. This subclass routes DIFFERENTLY from R8: the gate | ||
| // lands on `StaticDefinition.condition` (CR 613 layer 6 keyword grant) rather | ||
| // than on the ability's own `condition`, because the payload is a continuous | ||
| // effect over a set of permanents. | ||
| // | ||
| // Why this test exists: the CI parse-diff bot reports Silverflame Ritual's | ||
| // ABILITY-level `conditional` as `3+ White spent → ∅`, which reads like a | ||
| // dropped gate — the exact bug class this file guards. It is NOT a drop; the | ||
| // condition moved down one layer onto the static. The bot's signature reads the | ||
| // ability level only, so a layer move renders as `∅`. R8 cannot catch this | ||
| // because Slaying Fire's payload is ability-level. Nothing else pins it, so a | ||
| // future refactor really COULD drop the static's condition and every existing | ||
| // test here would stay green while "creatures you control gain vigilance" | ||
| // became unconditional. | ||
| // --------------------------------------------------------------------------- | ||
|
|
||
| /// Silverflame Ritual {3}{W} — verbatim Oracle text (`data/card-data.json`). | ||
| const SILVERFLAME_RITUAL: &str = "Put a +1/+1 counter on each creature you control.\nAdamant — If \ | ||
| at least three white mana was spent to cast this spell, \ | ||
| creatures you control gain vigilance until end of turn."; | ||
|
|
||
| /// Casts Silverflame Ritual with `payment` and returns the runner plus the | ||
| /// controller's creature, so the caller can read granted keywords off the board | ||
| /// AFTER resolution (a continuous grant is not visible in `CastOutcome` deltas). | ||
| fn cast_silverflame_ritual(payment: &[(ManaType, usize)]) -> (GameRunner, ObjectId) { | ||
| let mut scenario = GameScenario::new(); | ||
| scenario.at_phase(Phase::PreCombatMain); | ||
| let bear = scenario.add_vanilla(P0, 2, 2); | ||
| let ritual = scenario | ||
| .add_spell_to_hand_from_oracle(P0, "Silverflame Ritual", true, SILVERFLAME_RITUAL) | ||
| .with_mana_cost(ManaCost::Cost { | ||
| generic: 3, | ||
| shards: vec![ManaCostShard::White], | ||
| }) | ||
| .id(); | ||
| scenario.with_mana_pool(P0, pool(payment)); | ||
| let mut runner = scenario.build(); | ||
| assert!( | ||
| !runner.state().objects[&ritual] | ||
| .abilities | ||
| .iter() | ||
| .any(|a| matches!(&*a.effect, Effect::Unimplemented { .. })), | ||
| "Silverflame Ritual must parse with zero Effect::Unimplemented, got {:?}", | ||
| runner.state().objects[&ritual].abilities | ||
| ); | ||
| runner.cast(ritual).resolve(); | ||
| (runner, bear) | ||
| } | ||
|
|
||
| /// R9 positive reach-guard: three white mana satisfies the rider → the creature | ||
| /// gains vigilance. Also proves the UNGATED half of the card resolved (the | ||
| /// +1/+1 counter), so a total failure to resolve cannot masquerade as a pass. | ||
| #[test] | ||
| fn silverflame_ritual_three_white_grants_vigilance() { | ||
| let (runner, bear) = cast_silverflame_ritual(&[(ManaType::White, 3), (ManaType::Colorless, 1)]); | ||
| let obj = &runner.state().objects[&bear]; | ||
| assert_eq!( | ||
| obj.counters | ||
| .get(&CounterType::Plus1Plus1) | ||
| .copied() | ||
| .unwrap_or(0), | ||
| 1, | ||
| "the ungated first line must always resolve — if this is 0 the spell never resolved \ | ||
| and the vigilance assertion below would be vacuous" | ||
| ); | ||
| assert!( | ||
| obj.has_keyword(&Keyword::Vigilance), | ||
| "three white mana satisfies the Adamant rider, so the static grant must apply" | ||
| ); | ||
| } | ||
|
|
||
| /// R9 negative (paired with the reach-guard above): one white + three colorless | ||
| /// is four TOTAL mana but only one WHITE, so the rider must NOT fire. The | ||
| /// +1/+1 counter still lands, proving the spell resolved and the absence of | ||
| /// vigilance is a real gate decision rather than a non-resolution. | ||
| /// | ||
| /// This is the assertion that would fail if the static's condition were ever | ||
| /// dropped — i.e. if the `∅` the CI bot displays ever became literally true. | ||
| #[test] | ||
| fn silverflame_ritual_one_white_does_not_grant_vigilance() { | ||
| let (runner, bear) = cast_silverflame_ritual(&[(ManaType::White, 1), (ManaType::Colorless, 3)]); | ||
| let obj = &runner.state().objects[&bear]; | ||
| assert_eq!( | ||
| obj.counters | ||
| .get(&CounterType::Plus1Plus1) | ||
| .copied() | ||
| .unwrap_or(0), | ||
| 1, | ||
| "the ungated first line must still resolve, so the vigilance check below is not vacuous" | ||
| ); | ||
| assert!( | ||
| !obj.has_keyword(&Keyword::Vigilance), | ||
| "only one white mana was spent — the Adamant static grant must stay gated off" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
R9 again covers only white; no fail-closed pin for the static-grant gate path.
The mechanics here are sound (correct mana totals matching the printed cost, a genuine OfColor vs. total-mana discriminator, and a proper positive reach-guard pairing the negative assertion). However, this new subclass reproduces the exact gap flagged on the prior commit: only one color (white) is exercised, and unlike the enters-with class (pinned via Henge Walker), no malformed/unparseable-gate regression test exists for this static-grant code path, so a future regression that makes the StaticDefinition.condition unconditional for a different color, or a parse failure that fails open instead of closed for this class, would go undetected.
As per coding guidelines, reusable building blocks must be tested across their parameter range; as per path instructions, test adequacy for crates/engine/tests/** is a high-frequency finding and negative assertions need production-pipeline coverage of the failure mode, not just one color.
🤖 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/adamant_enters_with_leading_if_gate.rs`
around lines 369 - 466, Extend the static-grant coverage around
cast_silverflame_ritual and the R9 tests beyond white by adding a
different-color positive/negative gate case, preserving the counter assertion
and verifying vigilance remains gated. Add a production-pipeline regression
fixture with a malformed or unparseable StaticDefinition.condition and assert
the static grant fails closed rather than applying unconditionally.
Sources: Coding guidelines, Path instructions
matthewevans
requested changes
Jul 29, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Changes requested — the new generic per-color runtime path lacks discriminating coverage for most of its accepted class.
🔴 Blocker
[MED] CastManaSpentMetric::OfColor is parameterized for every ManaColor, but the production integration test exercises only white and red. Evidence: crates/engine/src/types/ability.rs:5442-5446 exposes the generic OfColor { color } metric, and crates/engine/src/parser/oracle_nom/condition.rs:6639-6654 accepts every color through parse_color; the current-head parse-diff adds Apocalypse Hydra and Bringer of Green Zenith's Twilight, while crates/engine/tests/integration/adamant_enters_with_leading_if_gate.rs:132-245 covers only white/red cast-pipeline rows. A blue/black/green mapping or threshold regression can therefore ship despite the shared parser/metric being claimed as a general building block. Suggested fix: add runtime cast-pipeline rows for the remaining color families, including a below-threshold/nonqualifying payment and the X >= 5 Apocalypse Hydra/Bringer path; assert the gate payload is applied only when the selected color qualifies.
✅ Clean
The generic parser seam is appropriate: it delegates the color axis to the existing parse_color combinator rather than enumerating Oracle strings.
Recommendation: add the missing production-pipeline class coverage, then request a new review on the updated head.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fix Ardenvale Paladin and its class. The Adamant cycle parsed its "enters with a +1/+1 counter" clause as a CR 614.1c replacement effect but silently dropped the sentence-initial
If <condition>,gate, leavingReplacementDefinition.condition = None. The engine therefore gave every copy of these creatures the counter regardless of what mana was actually spent — silently-wrong game behaviour that nothing flagged at runtime. Coverage surfaced it asSwallow:Condition_Ifon six cards.Root cause (
crates/engine/src/parser/oracle_replacement.rs): the module peeled a trailing" unless <cond>"gate (extract_enters_with_unless_suffix) and a trailing" if <cond>"gate (extract_enters_with_only_if_suffix), but nothing peeled a leading one — and the payload scan skips everything before"enters with".Fixed for the class, not the cards:
extract_enters_with_leading_if_gatemirrors the existing trailing extractor one-for-one, including its fail-closed tri-state. Condition recognition delegates toparse_inner_condition(the single authority), so the entire static-condition grammar is now reachable from the sentence-initial position, where previously zero shapes were. An unrecognised gate returnsUnparsedand the clause falls through toEffect::unimplementedrather than committing a replacement with the gate erased.parse_known_ability_word_name— the curatedABILITY_WORD_NAMESlist (the CR 207.2c ability words plus three curated CR 207.2d markers), not the loose 4-wordstrip_ability_wordheuristic. That distinction is load-bearing: the heuristic would peel any short em-dash label, including Scarlet Spider, Ben Reilly's "Sensational Save", exposing its body to the"if "test and regressing a green card toUnparsed. The three curated flavour markers are peelable, so this is a bounded widening rather than a strict CR 207.2c gate — accepted explicitly: no corpus card writes that shape, and a future one would fail closed toEffect::unimplementedrather than silently drop its gate.CastManaSpentMetric::OfColor { color }parameterises the existing metric axis (Total | DistinctColors | FromSource) instead of adding aReplacementConditionsibling, so the parsed condition flows through the untouchedreplacement_condition_from_staticinto the existingOnlyIfQuantity. Zero newReplacementConditionvariants, zero new condition-evaluation arms.parse_color_mana_spent_thresholdreuses the sharedparse_amount_thresholdauthority, so the comparator axis (at least N/N or more/N or fewer) is carried rather than hardcoded toGE.Cards fixed (6): Ardenvale, Embereth, Garenbrig, Locthwain and Vantress Paladin, plus Necromantic Summons. Dust Animus keeps
supported=trueand gains its gate — it is correctly conditional for the first time. Henge Walker ("mana of the same color" — a max-over-colors metric, out of scope) and Red and Black Legacy now fail closed and stay honestly red instead of silently granting their counters.Files changed
crates/engine/src/types/ability.rscrates/engine/src/game/quantity.rscrates/engine/src/game/layers.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/triggers.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/tests/integration/adamant_enters_with_leading_if_gate.rscrates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.mdTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 614.1c— "[This permanent] enters with . . ." effects are replacement effects (the authorizing rule)CR 614.12— how enters-the-battlefield replacements apply; co-authorizes the self-ETB scopeCR 207.2c— ability words have no rules meaning; the list contains "adamant" and "spell mastery"CR 207.2d— flavour words are tailored per-ability and not listed in the CR; "Sensational Save" is one, and is absent from the curated peel listCR 106.3+CR 601.2h— mana production and paying the total cost (CastManaSpentMetric::OfColor)CR 400.7d— "An ability of a permanent can reference information about the spell that became that permanent as it resolved, including … what mana was spent to pay those costs" (authorizes reading the entering object's payment record)CR 603.4is deliberately not cited in code. Its intervening-ifrule is explicitly scoped to triggered abilities ("this rule only applies to an 'if' that immediately follows a trigger condition"); this gates a replacement effect.CR 616.1was dropped after review — it had been cited for a multi-replacement fixture that does not exist.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— cleancargo clippy -p engine --all-targets -- -D warnings— exit 0, zero warningscargo test -p engine— exit 0; 21,943 passed, 0 failed (17828 lib + 4098 integration + 8 + 9; 15 ignored)cargo test -p engine --test integration adamant_enters_with_leading_if_gate— 10 passed, 0 failed./scripts/gen-card-data.sh— regenerated; coverage delta measured against a pre-change baseline of all 35,516 cards: 0true→false, 6false→true, net +6Revert-discrimination proof — neutralising
extract_enters_with_leading_if_gateto always returnNoLeadingIfmakes the negative-polarity tests fail (expected 0, got 1for Ardenvale below threshold;expected 0, got 2for Dust Animus) while the positive reach-guards stay green. The fix's failure mode is demonstrated, not assumed.Gate A
Gate A PASS head=8ca079a1f6c6e8a9cc6911a111129ece38b8b253 base=04e9c93c3c4f4f2d8cd7cd8884f370392759c15e
Anchored on
crates/engine/src/parser/oracle_replacement.rs:4366—extract_enters_with_unless_suffix, the existing trailing-gate extractor for enters-with clauses; the new leading-gate peel at:4469mirrors it one-for-one, including the fail-closed tri-state (EntersWithUnlessOutcomeat:4346→EntersWithLeadingIfOutcomeat:4421).crates/engine/src/parser/oracle_nom/condition.rs:6598—parse_colors_of_mana_spent_threshold, the sibling mana-spent threshold combinator (parse_amount_threshold+ subject anaphora →QuantityComparison{ManaSpentToCast{…}}); the newparse_color_mana_spent_thresholdat:6639differs only in the metric leaf.Final review-impl
Final review-impl PASS head=8ca079a1f6c6e8a9cc6911a111129ece38b8b253
Claimed parse impact
Ardenvale Paladin, Embereth Paladin, Garenbrig Paladin, Locthwain Paladin, Vantress Paladin, Necromantic Summons (
supported: false → true). Dust Animus gains a replacementconditionwhile stayingsupported: true, gap_count: 0. Henge Walker and Red and Black Legacy keep their gap counts; their handler changes fromSwallow:Condition_IftoEffect:unimplemented(silently-wrong → honestly red). Eleven Adamant rider cards (Once and Future, Foreboding Fruit, Silverflame Ritual, Cauldron's Gift, Slaying Fire, Sundering Stroke, Rally for the Throne, Unexplained Vision, Searing Barrage, Turn into a Pumpkin, Outmuscle) change condition AST shape fromAbilityCondition::ManaColorSpentto the equivalentQuantityCheck{ManaSpentToCast{OfColor}}with no behaviour change — pinned by the Slaying Fire runtime test in both polarities.Scope Expansion
None. One leaf variant on an already-parameterized axis, one parser seam, its single runtime arm, and three compiler-forced match joins.
Validation Failures
None blocking. Two disclosures the maintainer should have, because both concern the reliability of self-reports rather than the code:
TEMP-AB-PROBEearly-return was still disabling the entire fix, and 8 of 10 tests were failing. Because card data had been regenerated before that probe was re-inserted, coverage still looked correct — so this would have shipped as a PR that silently did nothing. Caught by re-running the tests rather than trusting the report; the probe and two throwawaytmp_probe_*tests were removed, and every claim above was then re-verified first-hand.04e9c93baseline — a 127-commit gap. Its flagged list includedAven Courier, which is upstream commit7a35ef2 "Fix Aven Courier (#6686)". Re-measured against the correct baseline, all criteria pass.Review process actually run: 2 plan-review rounds (
/review-engine-plan) and 4 implementation-review rounds (/review-impl), each in an isolated agent context, never self-review. Severity decayed monotonically: BLOCKER+3 MAJOR → 3 MAJOR → 3 LOW → 2 LOW → 1 LOW → CLEAN. The plan rounds caught a green card (Dust Animus) sitting unexamined in the new code's blast radius and a set of fail-closed guards that were dead code; the implementation rounds caught only documentation-accuracy defects (a false CR-207.2c invariant, and "revert-fail" annotations on two tests that never invoke the parser). No finding after the probe removal changed program behaviour.Scope note on the review rounds: the
/review-implagents read the code and verified claims againstdocs/MagicCompRules.txtanddata/card-data.json, but did not execute tests (Tilt is down; they correctly declined to shell out to cargo and reported that as "could not find out", not as a pass). All execution evidence in the Verification section was produced by the caller directly.CI Failures
None.
Summary by CodeRabbit
if <condition>,gates in replacement effects.ifgate parsing.