fix(engine): repeat Grindstone mill while milled cards share a color (#6091)#6588
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
superagent-security
Bot
added
the
contributor:flagged
📝 WalkthroughWalkthroughAdds ChangesLast zone changed support
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Grindstone
participant State
participant Quantity
participant RepeatProcess
Grindstone->>State: record last_zone_changed_ids
State->>Quantity: provide changed object population
Quantity->>RepeatProcess: evaluate shared-quality count
RepeatProcess->>Grindstone: continue or stop milling
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_effect/conditions.rs (1)
1539-1578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRecognizer is hardcoded to the "milled" verb even though
LastZoneChangedis a general zone-change ledger.
parse_shared_qualityandparse_numberalready generalize the quality/count axes, buttag(" were milled this way")locks this to mill specifically. SinceTargetFilter::LastZoneChanged(filter.rs) is verb-agnostic, consider generalizing the verb viaalt()(milled/discarded/exiled/revealed) the same way the rest of this file generalizes other "this way" outcome verbs, so the next card with this shared-quality-repeat shape on a different verb doesn't need a new near-duplicate function.As per coding guidelines,
crates/engine/**: "Build reusable card-class building blocks rather than one-card special cases; new enum variants, parsers, effects, and filters must support a category of cards."🤖 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/conditions.rs` around lines 1539 - 1578, Generalize strip_milled_shared_quality_conditional so its recognizer accepts the supported “this way” outcome verbs (milled, discarded, exiled, and revealed) instead of only matching “were milled this way.” Keep the existing shared-quality parsing, QuantityCheck construction, LastZoneChanged filter, and remainder handling unchanged.Source: Coding guidelines
crates/engine/src/game/quantity.rs (1)
1896-1904: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winSpecial-case only covers bare
LastZoneChanged; compound filters silently fall through to the wrong zone.The early return only fires for the literal
TargetFilter::LastZoneChanged. If it's ever composed (e.g.And([LastZoneChanged, Typed(..)])for "if two lands milled this way share a color"),extract_zones()won't recognize the ledger and defaults to scanningBattlefieldinstead of intersecting withstate.last_zone_changed_ids— the wrong population for milled/discarded/exiled cards. This also silently bypasses thefilter_contains_other_than_trigger_objectexclusion for any LastZoneChanged-based count. Contrast withfilter_contains_last_zone_changedineffects/mod.rs, which does recurse intoAnd/Or/Not— the "detects dependency" side is more general than this "resolves the population" side.Not a bug for Grindstone's bare-filter usage, but worth hardening before the next card composes this filter.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/quantity.rs` around lines 1896 - 1904, Update object_count_matching_ids to resolve LastZoneChanged recursively within compound TargetFilter variants such as And, Or, and Not, using state.last_zone_changed_ids as the candidate population before applying the remaining filter logic. Preserve filter_contains_other_than_trigger_object exclusion behavior for composed filters and keep existing handling for filters without a LastZoneChanged dependency.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 2688-2746: Make filter_contains_last_zone_changed and
quantity_ref_depends_on_zone_change_this_way exhaustive over their respective
enums, removing the _ => false fallbacks. Add explicit false arms for variants
that cannot contain a relevant filter, recurse through
TargetFilter::TrackedSetFiltered, and include every QuantityRef variant carrying
a TargetFilter, including the listed turn-based and zone-change variants, so
nested LastZoneChanged dependencies are detected.
In `@crates/engine/tests/integration/issue_6091_grindstone.rs`:
- Around line 164-175: Reorder the setup in the test around
add_colored_library_card so the six blue pad cards are added before the Green
Second and Blue Top pair, making that pair the first cards milled and ensuring
the test exercises Painter’s red effect. Optionally add assertions after
setup_painter_red confirming both top cards have red.
---
Nitpick comments:
In `@crates/engine/src/game/quantity.rs`:
- Around line 1896-1904: Update object_count_matching_ids to resolve
LastZoneChanged recursively within compound TargetFilter variants such as And,
Or, and Not, using state.last_zone_changed_ids as the candidate population
before applying the remaining filter logic. Preserve
filter_contains_other_than_trigger_object exclusion behavior for composed
filters and keep existing handling for filters without a LastZoneChanged
dependency.
In `@crates/engine/src/parser/oracle_effect/conditions.rs`:
- Around line 1539-1578: Generalize strip_milled_shared_quality_conditional so
its recognizer accepts the supported “this way” outcome verbs (milled,
discarded, exiled, and revealed) instead of only matching “were milled this
way.” Keep the existing shared-quality parsing, QuantityCheck construction,
LastZoneChanged filter, and remainder handling unchanged.
🪄 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: 1b17987f-ac1d-4688-923b-209d94464fca
📒 Files selected for processing (21)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/cost_payability.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/discard.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/filter.rscrates/engine/src/game/layers.rscrates/engine/src/game/quantity.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_static/restriction.rscrates/engine/src/types/ability.rscrates/engine/src/types/events.rscrates/engine/tests/integration/issue_6091_grindstone.rscrates/engine/tests/integration/main.rscrates/mtgish-import/src/convert/condition.rs
| fn filter_contains_last_zone_changed(filter: &TargetFilter) -> bool { | ||
| match filter { | ||
| TargetFilter::LastZoneChanged => true, | ||
| TargetFilter::And { filters } | TargetFilter::Or { filters } => { | ||
| filters.iter().any(filter_contains_last_zone_changed) | ||
| } | ||
| TargetFilter::Not { filter } => filter_contains_last_zone_changed(filter), | ||
| _ => false, | ||
| } | ||
| } | ||
|
|
||
| fn quantity_expr_depends_on_zone_change_this_way(expr: &QuantityExpr) -> bool { | ||
| match expr { | ||
| QuantityExpr::Ref { qty } => quantity_ref_depends_on_zone_change_this_way(qty), | ||
| QuantityExpr::DivideRounded { inner, .. } | ||
| | QuantityExpr::Multiply { inner, .. } | ||
| | QuantityExpr::ClampMin { inner, .. } | ||
| | QuantityExpr::Offset { inner, .. } => { | ||
| quantity_expr_depends_on_zone_change_this_way(inner) | ||
| } | ||
| QuantityExpr::Sum { exprs } | QuantityExpr::Max { exprs } => exprs | ||
| .iter() | ||
| .any(quantity_expr_depends_on_zone_change_this_way), | ||
| QuantityExpr::UpTo { max } => quantity_expr_depends_on_zone_change_this_way(max), | ||
| QuantityExpr::Power { exponent, .. } => { | ||
| quantity_expr_depends_on_zone_change_this_way(exponent) | ||
| } | ||
| QuantityExpr::Difference { left, right } => { | ||
| quantity_expr_depends_on_zone_change_this_way(left) | ||
| || quantity_expr_depends_on_zone_change_this_way(right) | ||
| } | ||
| QuantityExpr::Fixed { .. } => false, | ||
| } | ||
| } | ||
|
|
||
| fn quantity_ref_depends_on_zone_change_this_way(qty: &QuantityRef) -> bool { | ||
| match qty { | ||
| QuantityRef::ObjectCount { filter } | ||
| | QuantityRef::ObjectCountDistinct { filter, .. } | ||
| | QuantityRef::ObjectCountBySharedQuality { filter, .. } | ||
| | QuantityRef::CountersOnObjects { filter, .. } | ||
| | QuantityRef::Aggregate { filter, .. } | ||
| | QuantityRef::ControlledByEachPlayer { filter, .. } | ||
| | QuantityRef::DistinctColorsAmongPermanents { filter } | ||
| | QuantityRef::DistinctCounterKindsAmong { filter } | ||
| | QuantityRef::EnteredThisTurn { filter } | ||
| | QuantityRef::BattlefieldEntriesThisTurn { filter, .. } => { | ||
| filter_contains_last_zone_changed(filter) | ||
| } | ||
| QuantityRef::DistinctCardTypes { | ||
| source: crate::types::ability::CardTypeSetSource::Objects { filter }, | ||
| } | ||
| | QuantityRef::DistinctSubtypes { | ||
| source: crate::types::ability::CardTypeSetSource::Objects { filter }, | ||
| .. | ||
| } => filter_contains_last_zone_changed(filter), | ||
| _ => false, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Non-exhaustive _ => false wildcards on large enums, contrary to this file's own idiom.
Both new predicates use a catch-all _ => false instead of enumerating every variant, unlike the rest of the codebase's TargetFilter/FilterProp matchers (e.g. affected_filter_uses_object_population in filter.rs), which deliberately list every arm — even the false ones — so a missing variant is a compile error, not a silent gap.
filter_contains_last_zone_changedmissesTargetFilter::TrackedSetFiltered { filter, .. }, which wraps a nested filter exactly likeAnd/Or/Not.quantity_ref_depends_on_zone_change_this_wayonly checks 12 of the many filter-carryingQuantityRefvariants; siblings likeAttackedThisTurn,SacrificedThisTurn,ZoneChangeCountThisTurn,ZoneChangeAggregateThisTurn,TokensCreatedThisTurn,SpellsCastThisTurn,DamageDealtThisTurnall carry aTargetFilterand are unchecked.
Since this predicate drives the new zone-change-deferral logic, a false negative here means a future LastZoneChanged-composed repeat condition silently skips deferral and reads a stale ledger.
As per coding guidelines, crates/engine/**/*.rs: "Use exhaustive matches without wildcard fallbacks for known enums so the compiler detects missing variants."
♻️ Direction for making both matches exhaustive
fn filter_contains_last_zone_changed(filter: &TargetFilter) -> bool {
match filter {
TargetFilter::LastZoneChanged => true,
TargetFilter::And { filters } | TargetFilter::Or { filters } => {
filters.iter().any(filter_contains_last_zone_changed)
}
TargetFilter::Not { filter } => filter_contains_last_zone_changed(filter),
- _ => false,
+ TargetFilter::TrackedSetFiltered { filter, .. } => {
+ filter_contains_last_zone_changed(filter)
+ }
+ // ... enumerate every remaining TargetFilter variant explicitly as `=> false`
+ // (mirrors affected_filter_uses_object_population's exhaustive style).
}
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/engine/src/game/effects/mod.rs` around lines 2688 - 2746, Make
filter_contains_last_zone_changed and
quantity_ref_depends_on_zone_change_this_way exhaustive over their respective
enums, removing the _ => false fallbacks. Add explicit false arms for variants
that cannot contain a relevant filter, recurse through
TargetFilter::TrackedSetFiltered, and include every QuantityRef variant carrying
a TargetFilter, including the listed turn-based and zone-change variants, so
nested LastZoneChanged dependencies are detected.
Source: Coding guidelines
| // One blue + one green on top: without Painter they do not share a color; | ||
| // with Painter red they both gain red and repeat. | ||
| add_colored_library_card(&mut scenario, P1, "Blue Top", ManaCostShard::Blue); | ||
| add_colored_library_card(&mut scenario, P1, "Green Second", ManaCostShard::Green); | ||
| for idx in 0..6 { | ||
| add_colored_library_card( | ||
| &mut scenario, | ||
| P1, | ||
| &format!("Blue Pad {idx}"), | ||
| ManaCostShard::Blue, | ||
| ); | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Put the blue/green pair on top so this actually proves Painter’s effect.
The six blue pads are added last, so they are milled first as three blue-blue pairs. Even without Painter, the final blue-green pair is milled only after those repeats, yielding all eight cards. This test therefore passes if layer evaluation never grants red. Add the pads first, then add Green Second and Blue Top; optionally assert both cards have red after setup_painter_red.
🤖 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/issue_6091_grindstone.rs` around lines 164 -
175, Reorder the setup in the test around add_colored_library_card so the six
blue pad cards are added before the Green Second and Blue Top pair, making that
pair the first cards milled and ensuring the test exercises Painter’s red
effect. Optionally add assertions after setup_painter_red confirming both top
cards have red.
Source: Path instructions
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
2 participants
Closes #6091
Shipped
TargetFilter::LastZoneChanged— resolution-local ledger filter mirroringLastRevealed, wired through filter/targeting/quantity/coverage/ability_rw paths;object_count_matching_idsshort-circuits tolast_zone_changed_ids.strip_milled_shared_quality_conditionalmapsif [N] cards that share a [quality] were milled this way, repeat this processtoRepeatContinuation::WhileConditionoverObjectCountBySharedQuality { LastZoneChanged, Max } >= N(no Unimplemented repeat sub-ability).condition_depends_on_zone_change_this_wayrecursesQuantityCheck/QuantityExprtrees forLastZoneChangedfilters (same intent asZoneChangedThisWaydeferral).Issue scope vs out of scope
LastZoneChangedbuilding blockVerification
cargo fmt --allclippy,test-engine,card-data(or directcargo testfallback)cargo coverage/cargo semantic-auditfor parser diffSummary by CodeRabbit
New Features
Bug Fixes