Skip to content

fix(engine): repeat Grindstone mill while milled cards share a color (#6091)#6588

Merged
matthewevans merged 5 commits into
phase-rs:mainfrom
andriypolanski:fix/6091-grindstone-repeat-shared-color
Jul 25, 2026
Merged

fix(engine): repeat Grindstone mill while milled cards share a color (#6091)#6588
matthewevans merged 5 commits into
phase-rs:mainfrom
andriypolanski:fix/6091-grindstone-repeat-shared-color

Conversation

@andriypolanski

@andriypolanski andriypolanski commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Closes #6091

Shipped

  • TargetFilter::LastZoneChanged — resolution-local ledger filter mirroring LastRevealed, wired through filter/targeting/quantity/coverage/ability_rw paths; object_count_matching_ids short-circuits to last_zone_changed_ids.
  • Repeat-until parserstrip_milled_shared_quality_conditional maps if [N] cards that share a [quality] were milled this way, repeat this process to RepeatContinuation::WhileCondition over ObjectCountBySharedQuality { LastZoneChanged, Max } >= N (no Unimplemented repeat sub-ability).
  • Deferral predicatecondition_depends_on_zone_change_this_way recurses QuantityCheck / QuantityExpr trees for LastZoneChanged filters (same intent as ZoneChangedThisWay deferral).
  • Tests — parser building-block + full Grindstone parse; quantity ledger unit tests; activated integration tests (Painter red combo, unlike-color reach guard, colorless stop-at-two).

Issue scope vs out of scope

In scope (done) Out of scope (unchanged)
Grindstone activated repeat loop Painter's Servant parser reopen
LastZoneChanged building block Frontend / AI / mtgish
Shared-color milled-this-way repeat stripper General this-way verb taxonomy beyond structured milled+quality combinators

Verification

  • cargo fmt --all
  • Tilt: clippy, test-engine, card-data (or direct cargo test fallback)
  • Parser combinator gate + cargo coverage / cargo semantic-audit for parser diff

Summary by CodeRabbit

  • New Features

    • Added support for Last Zone Changed targets (most recently moved to a public zone), wired through targeting, resolution, counting, and user-facing labeling.
    • Extended Oracle parsing and condition evaluation for milled/shared-quality “repeat this process” gates (Grindstone-style loops).
  • Bug Fixes

    • Corrected how Last Zone Changed is treated across filter evaluation and special-case ability/discard/attachment logic.
    • Fixed repeat/stop behavior for shared-color and colorless scenarios, including Painter’s Servant interactions.
  • Tests

    • Added integration coverage for Issue #6091 Grindstone loop behavior.

@superagent-security

Copy link
Copy Markdown

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

@superagent-security superagent-security Bot added the contributor:flagged Contributor flagged for review by trust analysis. label Jul 24, 2026
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review
📝 Walkthrough

Walkthrough

Adds TargetFilter::LastZoneChanged support across filtering, targeting, quantity evaluation, parser condition handling, engine classification, and Grindstone integration tests.

Changes

Last zone changed support

Layer / File(s) Summary
Filter and target resolution
crates/engine/src/types/ability.rs, crates/engine/src/game/filter.rs, crates/engine/src/game/targeting.rs, crates/engine/src/game/trigger_matchers.rs, crates/engine/src/types/events.rs
LastZoneChanged resolves from state.last_zone_changed_ids, matches candidate objects, participates in filter classification, and supports trigger target matching.
Shared-quality repeat condition
crates/engine/src/game/effects/mod.rs, crates/engine/src/game/quantity.rs, crates/engine/src/parser/oracle_effect/*
Milled shared-quality conditionals are lowered to ObjectCountBySharedQuality checks over LastZoneChanged and parsed into unbounded repeat conditions.
Engine classification and effect wiring
crates/engine/src/game/ability_rw.rs, crates/engine/src/game/ability_scan.rs, crates/engine/src/game/ability_utils.rs, crates/engine/src/game/cost_payability.rs, crates/engine/src/game/coverage.rs, crates/engine/src/game/effects/discard.rs, crates/engine/src/game/layers.rs, crates/engine/src/parser/oracle_static/restriction.rs, crates/mtgish-import/src/convert/condition.rs
Classifier, formatting, discard, cost, layer, restriction, and import paths explicitly handle LastZoneChanged.
Grindstone integration coverage
crates/engine/tests/integration/issue_6091_grindstone.rs, crates/engine/tests/integration/main.rs, crates/engine/src/parser/oracle_effect/tests.rs
Tests cover repeated milling for matching colors, Painter’s Servant, unlike colors, colorless cards, and full Grindstone oracle parsing.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Suggested labels: enhancement

Suggested reviewers: matthewevans, lgray

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
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the Grindstone repeat-mill fix and is specific to the main change.
Linked Issues check ✅ Passed The PR adds shared-quality repeat handling so Grindstone continues milling when milled cards share a color, matching #6091.
Out of Scope Changes check ✅ Passed The added LastZoneChanged plumbing, parser support, and tests are supporting work for the Grindstone fix rather than unrelated scope.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_effect/conditions.rs (1)

1539-1578: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Recognizer is hardcoded to the "milled" verb even though LastZoneChanged is a general zone-change ledger.

parse_shared_quality and parse_number already generalize the quality/count axes, but tag(" were milled this way") locks this to mill specifically. Since TargetFilter::LastZoneChanged (filter.rs) is verb-agnostic, consider generalizing the verb via alt() (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 win

Special-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 scanning Battlefield instead of intersecting with state.last_zone_changed_ids — the wrong population for milled/discarded/exiled cards. This also silently bypasses the filter_contains_other_than_trigger_object exclusion for any LastZoneChanged-based count. Contrast with filter_contains_last_zone_changed in effects/mod.rs, which does recurse into And/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

📥 Commits

Reviewing files that changed from the base of the PR and between 22e7716 and 1059ca9.

📒 Files selected for processing (21)
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/cost_payability.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/discard.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/filter.rs
  • crates/engine/src/game/layers.rs
  • crates/engine/src/game/quantity.rs
  • crates/engine/src/game/targeting.rs
  • crates/engine/src/game/trigger_matchers.rs
  • crates/engine/src/parser/oracle_effect/conditions.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/parser/oracle_static/restriction.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/types/events.rs
  • crates/engine/tests/integration/issue_6091_grindstone.rs
  • crates/engine/tests/integration/main.rs
  • crates/mtgish-import/src/convert/condition.rs

Comment thread crates/engine/src/game/effects/mod.rs
Comment thread crates/engine/tests/integration/issue_6091_grindstone.rs Outdated
@github-actions

github-actions Bot commented Jul 24, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main ee189aa7a4fa)

🔴 Removed (1 signature)

  • 1 card · ➖ ability/repeat · removed: repeat
    • Affected (first 3): Grindstone

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

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested — the new event ledger filter and its regressions are incomplete.

🔴 Blocker

[MED] Preserve LastZoneChanged candidate selection when it is composed with another filter. Evidence: crates/engine/src/game/quantity.rs:1896-1914 uses the event ledger only for bare TargetFilter::LastZoneChanged; And { LastZoneChanged, Typed(..) } falls through to battlefield enumeration. Detect the event-ledger population recursively, then apply the full compound filter (including trigger-object exclusions).

[MED] Make the Grindstone tests discriminate. Evidence: crates/engine/tests/integration/issue_6091_grindstone.rs:151-175 puts the blue pads after Blue/Green, and :196-245 has no trailing cards for the repeat cases. The assertions can pass if Painter's color grant or repetition is broken. Reorder the Painter fixture and add trailing cards with assertions they remain.

Recommendation: request changes; fix the compositional filter seam and add revert-sensitive regressions.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/src/game/quantity.rs`:
- Around line 1903-1921: Update the candidate-selection logic around
filter_contains_last_zone_changed so the last_zone_changed_ids fast path is used
only for filters provably restricted to positive LastZoneChanged matches.
Recursively derive candidates for Or by unioning each branch’s candidates, and
for Not by complementing against the full applicable zone population; preserve
matches_target_filter afterward. Add regression tests covering
Or(LastZoneChanged, Typed(...)) and Not(LastZoneChanged).
🪄 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: 10b8171b-c2ef-404d-a7ca-fdf52f67a9c7

📥 Commits

Reviewing files that changed from the base of the PR and between 1059ca9 and 467eba4.

📒 Files selected for processing (4)
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/filter.rs
  • crates/engine/src/game/quantity.rs
  • crates/engine/tests/integration/issue_6091_grindstone.rs
🚧 Files skipped from review as they are similar to previous changes (3)
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/tests/integration/issue_6091_grindstone.rs
  • crates/engine/src/game/filter.rs

Comment thread crates/engine/src/game/quantity.rs Outdated

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-head verdict: blocked.

🔴 Blocker

[HIGH] Do not use filter_contains_last_zone_changed as a universal candidate-population switch. Evidence: crates/engine/src/game/filter.rs:966-977 returns true through Or and Not, then crates/engine/src/game/quantity.rs:1896-1904 restricts the entire population to last_zone_changed_ids. Or(LastZoneChanged, Typed(...)) must also see the typed objects outside that ledger, and Not(LastZoneChanged) is its complement, so both are silently miscounted. Suggested fix: derive candidates compositionally (intersection for And, union for Or, complement for Not) or limit the optimization to a provably positive ledger filter; add regressions for the two counterexamples.

🟡 Non-blocking

The parser remains deliberately mill-specific; that is acceptable only while this new primitive is not claimed as a generic cross-verb grammar.

Recommendation: request changes; repair the filter-population seam and prove compound filters before approval.

@matthewevans matthewevans added the bug Bug fix label Jul 24, 2026

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/src/game/quantity.rs`:
- Around line 1910-1948: Add a verified “CR <number>: <description>” citation
beside object_count_matching_ids, documenting the governing rule for the
ledger-based LastZoneChanged candidate selection and repeat-condition quantity
resolution. Keep the resolver logic unchanged and use the project’s established
citation format.
🪄 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: 657f0023-6a5e-4043-b710-2f9bcf578872

📥 Commits

Reviewing files that changed from the base of the PR and between 467eba4 and e8d1e63.

📒 Files selected for processing (2)
  • crates/engine/src/game/filter.rs
  • crates/engine/src/game/quantity.rs
🚧 Files skipped from review as they are similar to previous changes (1)
  • crates/engine/src/game/filter.rs

Comment thread crates/engine/src/game/quantity.rs Outdated
Comment on lines +1910 to +1948
pub(crate) fn object_count_matching_ids(
state: &GameState,
filter: &TargetFilter,
filter_ctx: &FilterContext<'_>,
source_id: ObjectId,
) -> Vec<ObjectId> {
let mut ids: Vec<ObjectId> = match filter {
TargetFilter::Or { filters } => {
let mut seen = HashSet::new();
let mut out = Vec::new();
for branch in filters {
for id in object_count_matching_ids(state, branch, filter_ctx, source_id) {
if seen.insert(id) {
out.push(id);
}
}
}
out
}
TargetFilter::Not { filter: inner }
if matches!(inner.as_ref(), TargetFilter::LastZoneChanged) =>
{
let ledger: HashSet<ObjectId> = state.last_zone_changed_ids.iter().copied().collect();
object_count_zone_object_ids(state, filter)
.into_iter()
.filter(|&id| !ledger.contains(&id))
.filter(|&id| matches_target_filter(state, id, filter, filter_ctx))
.collect()
}
_ if filter_matching_set_subset_of_last_zone_changed(filter) => state
.last_zone_changed_ids
.iter()
.copied()
.filter(|&id| matches_target_filter(state, id, filter, filter_ctx))
.collect(),
_ => object_count_zone_object_ids(state, filter)
.into_iter()
.filter(|&id| matches_target_filter(state, id, filter, filter_ctx))
.collect(),

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add a verified CR citation for this rules-resolution path.

This ledger-based candidate selection changes how repeat-condition quantities resolve, but it has no CR <number>: <description> annotation. Document the verified governing rule beside this resolver so the LastZoneChanged semantics remain auditable.

🤖 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 1910 - 1948, Add a verified
“CR <number>: <description>” citation beside object_count_matching_ids,
documenting the governing rule for the ledger-based LastZoneChanged candidate
selection and repeat-condition quantity resolution. Keep the resolver logic
unchanged and use the project’s established citation format.

Sources: Coding guidelines, Path instructions

@andriypolanski
andriypolanski force-pushed the fix/6091-grindstone-repeat-shared-color branch from e8d1e63 to 8ec1c33 Compare July 24, 2026 22:53

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Current-head verdict: 1 blocker remains — request changes.

🔴 Blocker

[MED] Candidate selection is still not compositional for an Or nested below another compound filter. Evidence: crates/engine/src/game/quantity.rs:1919-1931 unions ledger candidates only when Or is the outermost filter, while crates/engine/src/game/filter.rs:987-997 treats Or as not being a ledger subset. The fallback at crates/engine/src/game/quantity.rs:1948-1951 consequently scans only default/explicit zones for And([Or(LastZoneChanged, Typed(..)), Typed(..)]), omitting a moved-to-graveyard or exile ledger member even though matches_target_filter accepts it.

The current added tests cover top-level Or (quantity.rs:7819-7875) and direct Not (:7878-7928), not this nested composition.

Suggested fix: derive the candidate universe recursively for every boolean node (or normalize the complete filter before candidate enumeration), then add a nested And(Or(LastZoneChanged, …), …) regression with an off-battlefield ledger member.

✅ Clean

The prior direct Or/Not handling and the Grindstone parser replacement are covered on this head; the parse-diff’s single intended Grindstone signature change matches the new WhileCondition parser test.

Recommendation: request changes; re-review the next head.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_effect/tests.rs (1)

32143-32155: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Add the governing CR citation.

This test encodes the zero-target behavior of “up to one target,” but its new rules note lacks the required verified CR <number>: <description> annotation. Add the relevant zero-target rule (CR 115.6). (media.wizards.com)

As per coding guidelines, rules-related engine code must carry a verified CR citation and description.

🤖 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/tests.rs` around lines 32143 - 32155,
Add the verified citation “CR 115.6: An effect can instruct a player to choose
zero or more objects.” to the rules note in the test around the per-opponent
target fanout, preserving the existing explanation and identifying the
zero-target behavior governed by this rule.

Sources: Coding guidelines, Path instructions

🤖 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.

Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 32143-32155: Add the verified citation “CR 115.6: An effect can
instruct a player to choose zero or more objects.” to the rules note in the test
around the per-opponent target fanout, preserving the existing explanation and
identifying the zero-target behavior governed by this rule.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 2e363025-90e1-4778-9c89-25db8c6e895b

📥 Commits

Reviewing files that changed from the base of the PR and between e8d1e63 and 8ec1c33.

📒 Files selected for processing (21)
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/cost_payability.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/discard.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/filter.rs
  • crates/engine/src/game/layers.rs
  • crates/engine/src/game/quantity.rs
  • crates/engine/src/game/targeting.rs
  • crates/engine/src/game/trigger_matchers.rs
  • crates/engine/src/parser/oracle_effect/conditions.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/parser/oracle_static/restriction.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/types/events.rs
  • crates/engine/tests/integration/issue_6091_grindstone.rs
  • crates/engine/tests/integration/main.rs
  • crates/mtgish-import/src/convert/condition.rs
🚧 Files skipped from review as they are similar to previous changes (19)
  • crates/engine/src/game/layers.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/tests/integration/main.rs
  • crates/engine/src/types/events.rs
  • crates/mtgish-import/src/convert/condition.rs
  • crates/engine/src/parser/oracle_static/restriction.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/cost_payability.rs
  • crates/engine/src/game/effects/discard.rs
  • crates/engine/tests/integration/issue_6091_grindstone.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/trigger_matchers.rs
  • crates/engine/src/game/targeting.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/parser/oracle_effect/conditions.rs
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/filter.rs

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
crates/engine/src/game/quantity.rs (1)

1928-1941: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle nested negations containing LastZoneChanged.

Not(And(LastZoneChanged, ...)) and Not(Or(LastZoneChanged, ...)) fall through to zone-only candidates, so off-battlefield ledger objects that fail the inner predicate are never evaluated. Build every Not universe from its normal zone population plus the recursive inner universe, then filter; add a nested-negation regression.

🤖 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 1928 - 1941, Update the
TargetFilter::Not handling in object_count_zone_object_ids so every negation
candidate universe combines the normal zone population with the recursively
computed universe for its inner filter before applying the negated predicate.
Preserve the LastZoneChanged optimization while ensuring nested And/Or negations
evaluate off-battlefield ledger objects, and add a regression test covering
nested negations with LastZoneChanged.

Sources: Coding guidelines, Path instructions

🤖 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.

Outside diff comments:
In `@crates/engine/src/game/quantity.rs`:
- Around line 1928-1941: Update the TargetFilter::Not handling in
object_count_zone_object_ids so every negation candidate universe combines the
normal zone population with the recursively computed universe for its inner
filter before applying the negated predicate. Preserve the LastZoneChanged
optimization while ensuring nested And/Or negations evaluate off-battlefield
ledger objects, and add a regression test covering nested negations with
LastZoneChanged.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: fc64de70-6f50-4dc1-9502-12f52be97458

📥 Commits

Reviewing files that changed from the base of the PR and between 8ec1c33 and 940a893.

📒 Files selected for processing (2)
  • crates/engine/src/game/filter.rs
  • crates/engine/src/game/quantity.rs
💤 Files with no reviewable changes (1)
  • crates/engine/src/game/filter.rs

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Changes requested — one candidate-universe case remains incorrect on 940a893ffbc92a4a726a676fd7619b171220cd23.

🔴 Blocker

[MED] Include the recursively derived inner population when evaluating a compound negation containing LastZoneChanged. Evidence: crates/engine/src/game/quantity.rs:1928-1941 handles only Not(LastZoneChanged) specially; Not(And(LastZoneChanged, Typed(...))) instead seeds only object_count_zone_object_ids, whose default is the battlefield at :1899-1909. matches_target_filter recognizes LastZoneChanged directly in crates/engine/src/game/filter.rs:2169-2172, so a changed off-battlefield object that fails the inner And is never a candidate for the outer Not.

Build every Not candidate universe as its normal zone population union the recursive inner universe, deduplicated before the existing full-filter match, and add a nested-negation regression with a nonmatching ledger object in a non-battlefield zone.

✅ Clean

The new recursive handling for And/Or resolves the previous non-compositional candidate-selection finding; the remaining defect is confined to the negated compound case.

Recommendation: request changes; repair the Not universe and re-review this head.

andriy-polanski and others added 5 commits July 25, 2026 00:15
…ns (phase-rs#6091)

Probe last_zone_changed_ids for nested ledger filters before applying the
full compound predicate, reorder Painter fixture so the first mill is the
blue/green pair, and assert trailing library cards remain when repeat stops.

Co-authored-by: Cursor <cursoragent@cursor.com>
…hase-rs#6091)

Derive object-count candidates compositionally (ledger seed for positive
And, union for Or, complement for Not) instead of gating on the broad
filter_contains_last_zone_changed helper.

Co-authored-by: Cursor <cursoragent@cursor.com>
…rs#6091)

Union branch candidate populations for every And/Or node before applying
matches_target_filter, so And(Or(LastZoneChanged, Typed), Typed) includes
off-battlefield ledger members.

Co-authored-by: Cursor <cursoragent@cursor.com>
…6091)

Build every Not candidate set as zone population union the recursive inner
universe so Not(And(LastZoneChanged, Typed)) includes off-battlefield ledger
members that fail the inner conjunction.

Co-authored-by: Cursor <cursoragent@cursor.com>
@andriypolanski
andriypolanski force-pushed the fix/6091-grindstone-repeat-shared-color branch from a4bc523 to c507333 Compare July 25, 2026 00:15
@matthewevans matthewevans self-assigned this Jul 25, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on c5073336ddb573f3092d36740ef5d6085f1f3b2a.

The candidate-universe repair now unions a negation's normal zone population with its recursively derived inner population before evaluating the complete filter. The regression covers the required off-battlefield ledger member, and an independent adversarial review found no remaining current-head issue.

@matthewevans
matthewevans added this pull request to the merge queue Jul 25, 2026
@matthewevans matthewevans removed their assignment Jul 25, 2026
Merged via the queue into phase-rs:main with commit be45b8c Jul 25, 2026
13 of 14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix contributor:flagged Contributor flagged for review by trust analysis.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Card Bug] Grindstone: Does not mill library together with Painter's Servant

3 participants