Skip to content

Fix Culling Scales - #6789

Merged
matthewevans merged 7 commits into
phase-rs:mainfrom
jofortin:card/culling-scales
Jul 29, 2026
Merged

Fix Culling Scales#6789
matthewevans merged 7 commits into
phase-rs:mainfrom
jofortin:card/culling-scales

Conversation

@jofortin

@jofortin jofortin commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Summary

Fixes the dropped-target-restriction misparse (docs/parser-misparse-backlog.md root cause 1, "relative-clause / filter restriction on target dropped") for Culling ScalesAt the beginning of your upkeep, destroy target nonland permanent with the lowest mana value.

A postnominal superlative qualifier — with the {greatest|highest|least|lowest|smallest} {power|toughness|mana value} — was silently dropped whenever it was not followed by an among <set> clause. Culling Scales emitted Typed { type_filters: [Permanent, Non(Land)], properties: [] } with zero Effect::Unimplemented and zero parse warnings: the card read as fully supported while its ability could destroy any nonland permanent. The among-bearing form already worked (39 corpus cards), so this was a missing production, not a missing concept.

CR 109.2: a type description with no zone clause and no "card" means permanents on the battlefield, so with no explicit set the ranked population is the enclosing noun phrase. Because the suffix appears before that phrase closes, a detection pass records the head and the FilterProp is materialized once the phrase is complete — snapshot before append, or the prop nests inside its own population and resolve_filter_threshold recurses without bound.

No new engine surface: reuses FilterProp::{Cmc, PtComparison} + QuantityRef::Aggregate, all already evaluated at every boundary reached. types/ability.rs is untouched.

Files changed

  • crates/engine/src/parser/oracle_nom/primitives.rs
  • crates/engine/src/parser/oracle_nom/condition.rs
  • crates/engine/src/parser/oracle_nom/filter.rs
  • crates/engine/src/parser/oracle_target.rs
  • crates/engine/src/parser/oracle_ir/context.rs
  • crates/engine/src/parser/oracle_ir/snapshots/…__diagnostic_ignored_remainder.snap
  • crates/engine/tests/integration/culling_scales_lowest_mana_value_target.rs
  • crates/engine/tests/integration/favor_of_the_mighty_greatest_mana_value_protection.rs
  • crates/engine/tests/integration/main.rs

Track

Non-developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: /engine-implementer

The pipeline ran with genuinely isolated agents: an independent planner, three independent plan-review rounds, and an independent review-impl against the committed diff. Each reviewer received only the artifact under review plus CLAUDE.md and the relevant skill — never the authoring context. Severity converged 8 findings (3 HIGH) → 6 (2 HIGH) → 9 (0 HIGH); the final review-impl returned 6 findings (0 HIGH), all addressed below. Implementation itself was applied in the orchestrating thread from the reviewed plan's consolidated checklist.

CR references

Every number grep-verified in docs/MagicCompRules.txt (sub-rules carry no trailing period, so grep "^109\.2a ", not "^109\.2a\.").

  • CR 109.2 — the authorizing rule: a description including a card type means a permanent on the battlefield, which is what licenses the enclosing noun phrase as the ranked population and the battlefield as its default zone.
  • CR 109.2a — the carve-out: a description containing "card" plus a zone means cards in that zone. A different population this change does not model, hence the structural guard.
  • CR 601.2c — the controller announces one legal target; with Comparator::EQ against the extremum, every tied object is legal (matching Culling Scales' own reminder text).
  • CR 608.2b — targets are re-checked on resolution, so the restriction is live rather than snapshotted.
  • CR 611.3a — a static ability's continuous effect isn't locked in; it is reapplied as state changes. This is what the Favor of the Mighty runtime test pins.
  • CR 613.1f — Layer 6, ability-adding, where the protection grant lands.
  • CR 208.1 / CR 202.3 — power/toughness and mana value: the two property axes.
  • CR 205 — type filters, for the exhaustive TypeFilter match.
  • CR 400.1 — zones, for the non-battlefield predicate.
  • CR 704.5b — deck-out, for the test scaffolding rationale.

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 --check — clean.

  • cargo clippy-strict (clippy --all-targets -- -D warnings, whole workspace) — clean, 0 warnings.

  • cargo test -p phase-engineok. 17968 passed; 0 failed; 6 ignored (lib) · ok. 4189 passed; 0 failed; 2 ignored (integration) · ok. 8 passed and ok. 9 passed (bin unit tests).

  • ./scripts/check-parser-combinators.sh — see Gate A. No // allow-noncombinator anywhere in this change.

  • ./scripts/check-prelowered-ratchet.sh — Gate P PASS.

  • cargo coverage / cargo semantic-auditCI-owned: ./scripts/setup.sh --agent needs jq and pnpm, neither installable in this environment, so client/public/card-data.json cannot be generated here. Substituted by the direct probe sweep under Claimed parse impact.

  • Revert-discrimination, per test row (production change reverted alone, suite re-run, restored):

    • bare_superlative_lowest_mana_value_ranks_over_enclosing_noun_phrase and …carries_controller_onto_population → fail; among_form_population_still_comes_from_the_explicit_set stays green, confirming it covers the untouched path.
    • culling_scales_offers_only_the_lowest_mana_value_nonland_permanent → fails on "MV 4 is not the lowest".
    • favor_of_the_mighty_protects_only_the_greatest_mana_value_creature → fails on the exclusion assertion, because the dropped restriction over-grants protection to every creature.
    • multi_type_relative_clause_ranks_every_leg_against_the_whole_population and …_ranks_over_the_disjunctive_population_at_runtime → both fail under either multi-type regression, each patched in and re-run: (a) declining the multi-type case makes the MV 5 creature legal again; (b) ranking over [Permanent] alone lets the MV 0 Land set the bar, so nothing is legal and the trigger never pauses.

Gate A

Gate A PASS head=30fef29e4dee9ac148e09c044e79cb2f709deb23 base=4f2ff6eba08f78da44201c86a2661939a325be78

Anchored on

  • crates/engine/src/parser/oracle_nom/filter.rsparse_pt_comparison, the existing with <property> <comparison> suffix combinator that parse_superlative_property_head is placed beside and modelled on (same tag + sub-combinator + guard composition, same module, same OracleResult shape).
  • crates/engine/src/parser/oracle_target.rsparse_superlative_property_suffix, the pre-existing among-form authority whose head grammar this change factors out and whose superlative_property_filter_prop construction is reused unchanged for the bare form.
  • crates/engine/src/parser/oracle_nom/condition.rsparse_superlative_adjective / parse_property_keyword, the pre-existing atoms relocated to primitives.rs so the condition layer and the target layer share one table instead of two drifting copies.
  • crates/engine/src/parser/oracle_ir/context.rspush_diagnostic's existing TargetFallback dedupe, extended to the sibling IgnoredRemainder variant on the same rationale.

Final review-impl

Final review-impl PASS head=30fef29e4dee9ac148e09c044e79cb2f709deb23

An isolated reviewer returned 6 findings (0 HIGH) against the committed diff; all are addressed in 99330f9. It independently confirmed as sound: no new engine surface (every emitted variant pre-exists, types/ability.rs untouched); all CR citations resolve and describe the annotated code; the population snapshot precedes the append so there is no self-nesting; the sacrifice-pool boundary is already runtime-pinned for this exact shape at game/effects/sacrifice.rs:1682 with the player_scope fan-out stamping scoped_player, so Szat's Will's population and pool resolve to the same player; the condition-layer cards (Triumph of Ferocity/Cruelty, Thickest in the Thicket, Abzan Beastmaster, Summon: Fenrir, Padeem, Might Makes Right) parse from raw text in oracle_nom/condition.rs and probe unchanged; Favor of the Mighty is genuinely complete; and Drop of Honey / Porphyry Nodes / Purging Scythe are untouched (a pre-existing definite-article gap, ParentTarget for every suffix).

Findings and resolutions:

  1. [MED] The CR 109.2a refusal branch left a silently over-broad filter. destroy target creature with the greatest power in your graveyard consumed the superlative at detection and then refused to emit it once the zone prop accumulated — restriction gone, card still supported. Fixed: nonbattlefield_zone_clause_lies_ahead looks ahead with the authoritative parse_zone_suffix at each word boundary, so detection declines to consume at all and the phrase stays unclaimed. New test with a positive twin so the guard cannot refuse everything.

  2. [MED] The population snapshot omitted relative_core_type_filters. Fixed across two follow-up commits after review, and the history matters because the first two attempts were both wrong:

    • First attempt: refuse materialization when the accumulator is non-empty. CodeRabbit flagged this as incomplete — detection had already consumed the superlative, so the target continued unranked.
    • Second attempt (094be96): fold a single relative type into the population. Correct for the single-type case, but the maintainer flagged that the multi-type case still consumed-then-dropped the restriction, so that's an artifact or creature targeted any artifact or creature.
    • Final (30fef29): the population carries the relative types as one conjunctive TypeFilter::AnyOf, since base ∧ (A ∨ B) == (base ∧ A) ∪ (base ∧ B) is exactly the union of the Or legs. The prop rides in properties, which the branch cross-product replicates onto every leg. The len() <= 1 guard is gone, so there is no longer any path that drops the aggregate for a battlefield population. TypeFilter::AnyOf pre-exists and is evaluated at runtime by type_filter_matches (game/filter.rs:3093); type_filter_includes_card already recurses through it, so the CR 109.2a "card" carve-out cannot be smuggled past inside a disjunction. Parser + runtime coverage, both revert-discriminating against both failure modes.
  3. [LOW] Duplicate diagnostics. push_diagnostic now dedupes IgnoredRemainder as it already did TargetFallback. The snapshot delta is the audit trail — Fathom Fleet Swordjack emitted one identical diagnostic twice and now emits it once. Key renamed to …_unmodelled_population since it now covers both refusal reasons.

  4. [LOW] Dead assertion. The Culling Scales Non(Land) check had land == None always. A basic land is now added at MV 0 — below the tie — so the exclusion can only come from the type conjunction.

  5. [LOW] A third site still hand-rolls the head (oracle_nom/condition.rs, without the word-boundary guard). Not fixed here — deliberately out of scope: it is the condition layer, seven cards route through it, and consolidating it is a behaviour change to code this PR otherwise only imports from. Recorded as follow-up.

  6. [LOW] The Favor of the Mighty test forces a full layer rebuild (mark_full()) and creates the third creature via zones::create_object rather than a real ETB, so it does not exercise the incremental-escalation path production takes. Not fixed here — the assertion it makes (protection moves) is still revert-discriminating; tightening it to the real ETB path is recorded as follow-up.

  7. [NON-BLOCKING, maintainer] CR 109.2a cited for the generic non-battlefield-zone guard. Correct — verified at docs/MagicCompRules.txt:584, CR 109.2a requires a description containing both "card" and a zone name. The battlefield default itself is CR 109.2 (line 582: applies only when the description names no zone and contains no "card"/"spell"/"source"/"scheme"), so that is the rule the zone guard is reasoning about. Fixed in 30fef29: the six generic-zone sites now cite CR 109.2, naming CR 109.2a only for the "card" leg. The type_filter_includes_card doc block already contrasted the two correctly and is unchanged.

Claimed parse impact

Measured, not estimated. The parser was run over all 21 bare-form and all 39 among-form corpus cards, passing each card's real types/subtypes (this matters: Triumph of Gerrard's Saga chapters are unreachable without subtypes: ["Saga"], and an earlier measurement that omitted them was wrong).

4 cards fixed, spanning three distinct consumption boundaries:

Card Boundary Emitted
Culling Scales target legality (CR 608.2b) Cmc { EQ, Aggregate{Min, ManaValue} } over [Permanent, Non(Land)]
Triumph of Gerrard target legality, Saga chapters PtComparison { Power, EQ, Aggregate{Max, Power} }, population controller: You
Favor of the Mighty static affected, CR 613 layers same shape; Unimplemented 1 → 0
Szat's Will sacrifice pool build ScopedPlayer on both the outer filter and the nested population

The remaining 17 bare-form cards are unchanged and excluded for independent reasons — 5 are already handled by the condition-layer authority; Drop of Honey / Porphyry Nodes / Purging Scythe / Tariff / Juxtapose / Desecrator Hag collapse to ParentTarget on a pre-existing definite-article gap that is suffix-independent; Item Crate sits behind an unimplemented random-token mechanic; Padeem and Might Makes Right need condition-layer parameterization. All probe-verified, not assumed.

No among-form card changed shape, and the full suite — which includes the among-form regression tests — is green.

The card-data value diff (./scripts/gen-card-data.sh) is CI-owned for the jq/pnpm reason above; the parse-diff sticky comment is the authoritative version of the table.

Scope Expansion

Two deliberate additions beyond the parser seam, both reviewer-driven and disclosed rather than silent:

  1. oracle_ir/context.rs — one-line dedupe extension (finding 3). It changes one committed snapshot, and that snapshot delta is the evidence of what it does.
  2. The atom relocation from oracle_nom/condition.rs to oracle_nom/primitives.rs. Strictly this PR could have added a second copy of the head grammar; sharing one table instead is what the single-authority doctrine asks for, and it is why the smallest arm (zero corpus attestation) is carried over — so the consolidation loses no grammar either predecessor recognized.

No new type, variant, field, trait, or serialized shape. game/filter.rs, game/quantity.rs, game/targeting.rs, types/ability.rs, the WASM bridge, the frontend and phase-ai are untouched.

Validation Failures

None blocking. Two review findings are deliberately deferred rather than fixed (items 5 and 6 above), each with its reason stated inline. The plan additionally specified a Szat's Will runtime test at the sacrifice-pool boundary and oracle_ir / oracle_static snapshot rows; the reviewer judged the missing Szat's Will test an acceptable gap because that shape is already runtime-pinned at game/effects/sacrifice.rs:1682, and I did not add the snapshot rows. Both are recorded as follow-ups rather than claimed.

CI Failures

None.

Summary by CodeRabbit

  • New Features

    • Added support for “with the greatest/highest/lowest/least/smallest [property]” clauses without an among set.
    • Improved handling of superlative properties across noun phrases, controllers, zones, and type restrictions.
    • Added word-boundary checks to prevent partial property matches.
  • Bug Fixes

    • Prevented duplicate ignored-remainder diagnostics.
    • Corrected superlative targeting and protection behavior as game state changes.
  • Tests

    • Added integration coverage for lowest and greatest mana-value scenarios.

Jonathan Fortin and others added 4 commits July 29, 2026 09:16
… noun phrase

WIP CHECKPOINT — production change complete and verified; the plan's test matrix
(parser rows, snapshots, runtime rows per boundary) is NOT yet written, so this
is not yet PR-ready.

Backlog root cause 1: "with the {greatest|highest|least|lowest|smallest}
{power|toughness|mana value}" was silently dropped unless followed by an
"among <set>" tail. Culling Scales' "destroy target nonland permanent with the
lowest mana value" emitted `properties: []` — zero Unimplemented, zero warnings,
and able to destroy ANY nonland permanent.

CR 109.2: with no explicit set, the ranked population IS the enclosing noun
phrase. Because the suffix is seen before that phrase closes, detection records
the head and materialization builds the FilterProp after the phrase completes —
snapshot before append, or the prop nests inside its own population.

- `oracle_nom/primitives.rs`: `parse_superlative_adjective` / `parse_property_keyword`
  relocated here from `condition.rs` so the condition and target layers share one
  atom instead of parallel tables (+ the `smallest` arm, zero corpus attestation,
  kept so the consolidation loses no recognized grammar).
- `oracle_nom/filter.rs`: `parse_superlative_property_head` — single authority for
  the head, with a `not(alphanumeric1)` word-boundary guard the bare form needs and
  the `among` form got implicitly from its following tag.
- `oracle_target.rs`: `parse_superlative_property_suffix` delegates the head and
  now owns ONLY the explicit "among" clause; the 2x3 cross-product alt is deleted.
- CR 109.2a carve-out is structural, checked twice — a fail-fast pre-check at
  detection so a "card" phrase is never consumed, and the authority at
  materialization over the final accumulators, because the zone passes run in
  between. A refused phrase records `IgnoredRemainder` rather than vanishing.

No new engine surface: reuses `FilterProp::{Cmc,PtComparison}` +
`QuantityRef::Aggregate`, all already evaluated at every boundary reached.

Verified: 4 cards fixed — Culling Scales (`Cmc EQ Aggregate{Min,ManaValue}`),
Szat's Will (`ScopedPlayer` on both the outer filter and the nested population),
Triumph of Gerrard (Saga chapters, needs Saga subtypes to reach), Favor of the
Mighty (static `affected`, 1 -> 0 Unimplemented). `cargo fmt --check` clean,
`cargo clippy-strict` clean workspace-wide, `cargo test -p phase-engine` 17924
lib + 4167 integration passing, Gate A/G/P pass. The graveyard-card guard refuses
`creature card with the greatest power in your graveyard` and emits no Aggregate.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…ries

Six parser rows in `oracle_target.rs` plus one runtime test per boundary the
emitted filter actually reaches. Every row was revert-verified: reverting the
materialization block flips it red.

Parser rows:
- Culling Scales' verbatim clause ranks over its ENCLOSING noun phrase, and the
  population itself carries NO properties (a population nesting the superlative
  inside itself would make `resolve_filter_threshold` recurse without bound).
- "creature you control with the greatest power" puts the controller on BOTH the
  candidate filter and the population — ranking a controller-scoped candidate
  against a global population picks the wrong creature.
- the `among` form still takes its population from the EXPLICIT set: regression
  guard for the 37 corpus cards that already worked.
- CR 109.2a: a card-in-graveyard phrase emits no superlative, reach-guarded on the
  zone prop still being parsed so it cannot pass on a total parse failure.
- the head's `not(alphanumeric1)` boundary rejects "powerstone", with a positive
  twin.
- all 5 superlative adjectives x 3 properties map to the right aggregate.

Runtime, boundary 1 (target legality, CR 608.2b) —
`culling_scales_lowest_mana_value_target.rs`: drives the real upkeep trigger and
asserts on the engine's own `legal_targets`. The MV 1 pair is legal, MV 4 / MV 6
are not, the Land is excluded by `Non(Land)`. Two notes for the next reader: the
fixture needs a deliberate TIE at the minimum, because a single legal target is
auto-chosen and never surfaces `TriggerTargetSelection`; and `add_creature` does
not set `mana_cost`, so without explicit costs everything ties at MV 0 and the
test proves nothing. On revert: "MV 4 is not the lowest" fails.

Runtime, boundary 2 (static `affected`, CR 613 layers) —
`favor_of_the_mighty_greatest_mana_value_protection.rs`: this card flips to
*supported*, so an AST assertion would not be enough. Protection lands on the
greatest-mana-value creature, not the others, and MOVES when a larger creature
enters (CR 611.3a — the affected set is re-derived, not locked in). On revert the
dropped restriction over-grants protection to EVERY creature and the exclusion
assertion fails.

Full suite: 17930 lib + 4169 integration passing. fmt clean, clippy-strict clean
workspace-wide, Gate A/G/P pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…able population

Addresses the independent review-impl findings on this branch.

[MED] The CR 109.2a materialization guard was reachable and left a silently
over-broad filter: "destroy target creature with the greatest power in your
graveyard" consumed the superlative at the detection pass, then the guard refused
to emit it once the zone prop had accumulated — so the restriction vanished while
the card still reported as supported, the exact defect this change exists to
remove. The zone passes run AFTER detection, so the fix is a look-ahead:
`nonbattlefield_zone_clause_lies_ahead` tries the authoritative
`parse_zone_suffix` at each word boundary of the remaining phrase (via the shared
word-boundary scan primitive, so no bespoke zone vocabulary), and detection
declines to consume at all. The phrase is then left unclaimed rather than
consumed-and-dropped.

[MED] The population snapshot omits `relative_core_type_filters`, which a trailing
"that's an artifact" clause fills in after the detection pass — so
"permanent with the greatest mana value that's an artifact" would have ranked
`[Permanent, Artifact]` candidates against a `[Permanent]` population, i.e. a
different set from the enclosing noun phrase (CR 109.2). Materialization now
refuses when that accumulator is non-empty. Zero corpus attestation today; this
keeps it that way rather than ranking over the wrong set.

[LOW] `push_diagnostic` now dedupes `IgnoredRemainder` as it already did
`TargetFallback` — both can be pushed from a combinator a speculative `alt`
re-enters on a discarded alternative. The snapshot delta shows the effect: Fathom
Fleet Swordjack was emitting one byte-identical "it's attacking" diagnostic twice
and now emits it once. The diagnostic key is renamed to
`..._unmodelled_population`, since it now covers both refusal reasons.

[LOW] The Culling Scales `Non(Land)` assertion was dead — `land` was always
`None` because nothing was on the battlefield. A basic land is now added at MV 0,
BELOW the tie, so the exclusion can only come from the type conjunction and never
from losing the mana-value comparison.

Two new parser rows cover the refusal paths the review found unexercised: the
zone look-ahead (with a positive twin so the guard cannot refuse everything) and
the relative-clause refusal (reach-guarded on the clause reaching the candidates).

Full suite: 17967 lib + 4188 integration passing. fmt clean, clippy-strict clean
workspace-wide, Gate A/G/P pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jofortin
jofortin requested a review from matthewevans as a code owner July 29, 2026 14:54
@github-actions github-actions Bot added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 29, 2026
@coderabbitai

coderabbitai Bot commented Jul 29, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 310188f7-c0f6-4672-a3f4-d4c02bc7947f

📥 Commits

Reviewing files that changed from the base of the PR and between 094be96 and 370da4c.

📒 Files selected for processing (3)
  • crates/engine/src/parser/oracle_target.rs
  • crates/engine/tests/integration/culling_scales_lowest_mana_value_target.rs
  • crates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/engine/tests/integration/main.rs
  • crates/engine/src/parser/oracle_target.rs

📝 Walkthrough

Walkthrough

Changes

Superlative parsing is centralized and extended to support bare postnominal forms with deferred authority validation. Diagnostic deduplication now includes ignored remainders, and parser/runtime regression tests cover target selection and dynamic protection reassignment.

Changes

Superlative target parsing

Layer / File(s) Summary
Shared parser primitives
crates/engine/src/parser/oracle_nom/primitives.rs, crates/engine/src/parser/oracle_nom/condition.rs, crates/engine/src/parser/oracle_nom/filter.rs
Superlative adjectives and property keywords are centralized, and superlative heads enforce word boundaries.
Deferred target superlative materialization
crates/engine/src/parser/oracle_target.rs, crates/engine/src/parser/oracle_ir/context.rs
Bare superlatives are deferred and validated against noun-phrase authority; invalid cases produce deduplicated IgnoredRemainder diagnostics, while explicit among clauses use the shared parser.
Runtime regression coverage
crates/engine/src/parser/oracle_target.rs, crates/engine/tests/integration/culling_scales_lowest_mana_value_target.rs, crates/engine/tests/integration/favor_of_the_mighty_greatest_mana_value_protection.rs, crates/engine/tests/integration/main.rs
Parser and integration tests cover superlative parsing, lowest-mana-value targeting, greatest-mana-value protection reassignment, and test-module registration.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested reviewers: matthewevans

Sequence Diagram(s)

sequenceDiagram
  participant OracleText
  participant OracleTargetParser
  participant ParseContext
  participant GameRunner
  OracleText->>OracleTargetParser: parse superlative target clause
  OracleTargetParser->>OracleTargetParser: defer and validate ranked population
  OracleTargetParser->>ParseContext: emit ranked property or IgnoredRemainder
  GameRunner->>OracleTargetParser: evaluate parsed targeting rule
  OracleTargetParser-->>GameRunner: provide ranked legal targets
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 is concise and accurately names a primary affected card in the change set.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ 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: 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/parser/oracle_target.rs`:
- Around line 3576-3608: Update the pending bare-superlative handling around
superlative_property_filter_prop so a non-empty relative_core_type_filters
clause cannot consume the superlative and then continue with an unranked target.
Either reject the phrase before the parser advances past the superlative, or
include relative_core_type_filters in the ranked population; preserve the
existing valid battlefield-population behavior.
🪄 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: 9b35c77f-3ba4-412a-b205-b036af482309

📥 Commits

Reviewing files that changed from the base of the PR and between 4f2ff6e and 99330f9.

⛔ Files ignored due to path filters (1)
  • crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__diagnostic_snapshots__diagnostic_ignored_remainder.snap is excluded by !**/*.snap, !**/snapshots/**
📒 Files selected for processing (8)
  • crates/engine/src/parser/oracle_ir/context.rs
  • crates/engine/src/parser/oracle_nom/condition.rs
  • crates/engine/src/parser/oracle_nom/filter.rs
  • crates/engine/src/parser/oracle_nom/primitives.rs
  • crates/engine/src/parser/oracle_target.rs
  • crates/engine/tests/integration/culling_scales_lowest_mana_value_target.rs
  • crates/engine/tests/integration/favor_of_the_mighty_greatest_mana_value_protection.rs
  • crates/engine/tests/integration/main.rs

Comment thread crates/engine/src/parser/oracle_target.rs
@github-actions

github-actions Bot commented Jul 29, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 4 card(s), 6 signature(s) (baseline: main 9f5ed5c662b9)

🟢 Added (1 signature)

  • 1 card · ➕ static/Continuous · added: Continuous (affects=mv max mana value of creature creature, mods=grant Protection, grant Protection, grant Protection, grant Protection, grant Protection)
    • Affected (first 3): Favor of the Mighty

🔴 Removed (1 signature)

  • 1 card · ➖ ability/static_structure · removed: static_structure
    • Affected (first 3): Favor of the Mighty

🟡 Modified fields (4 signatures)

  • 1 card · 🔄 ability/Destroy · changed field target: permanent non-landmv min mana value of permanent non-land permanent non-land
    • Affected (first 3): Culling Scales
  • 1 card · 🔄 ability/PutCounter · changed field target: you control creaturepower =max power of you control creature you control creature
    • Affected (first 3): Triumph of Gerrard
  • 1 card · 🔄 ability/Sacrifice · changed field target: scoped player controls creaturepower =max power of scoped player controls creature scoped player controls creature
    • Affected (first 3): Szat's Will
  • 1 card · 🔄 ability/grant Flying, grant FirstStrike, grant Lifelink · changed field target: you control creaturepower =max power of you control creature you control creature
    • Affected (first 3): Triumph of Gerrard

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

@matthewevans matthewevans self-assigned this Jul 29, 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.

Request changes — the bare-superlative parser still consumes a rules-bearing qualifier and then returns the unranked target as supported.

🔴 Blocker

crates/engine/src/parser/oracle_target.rs:3576-3627 consumes with the greatest … into pending_bare_superlative, but deliberately declines to materialize FilterProp::{Cmc, PtComparison} when relative_core_type_filters is non-empty. The else merely records IgnoredRemainder; normal target construction continues with the unranked TypedFilter after the relative filter is appended. Consequently, target permanent with the greatest mana value that's an artifact is accepted as a target for any artifact rather than only the greatest-mana-value artifact. The new unit test at oracle_target.rs:17366-17390 codifies that false-green shape by asserting that no aggregate restriction is present.

Please keep the qualifier semantically live: either carry the final relative type filters into the snapshotted aggregate population, or decline before consuming the bare-superlative head so the unsupported rider remains visible to coverage. This needs the corresponding parser and runtime/discriminating coverage; it is not a maintainer-fixup-sized change.

The current parse-diff artifact (4 changed cards) confirms this is production parser surface, so the IgnoredRemainder cannot be treated as harmless diagnostic-only evidence.

🟡 Non-blocking

crates/engine/src/parser/oracle_target.rs:2918-2921 and :5008-5052 apply CR 109.2a to the generic non-battlefield-zone guard. The verified text of CR 109.2a is limited to a description that includes both the word “card” and a zone name; the generic zone scan should cite CR 109.2 (or omit a CR annotation), reserving CR 109.2a for the card-plus-zone branch.

Recommendation: request changes. Preserve or strictly fail the bare qualifier, add a revert-discriminating test for the final target shape, then refresh the parse-diff evidence on the new head.

@matthewevans matthewevans removed their assignment Jul 29, 2026
…ulation

CodeRabbit finding on PR phase-rs#6789 (Major, functional correctness): the previous
commit refused to MATERIALIZE the superlative when `relative_core_type_filters`
was non-empty, but the detection pass had already CONSUMED it — so
"permanent with the greatest mana value that's an artifact" continued with an
unranked target, the same silent drop this PR exists to remove.

CodeRabbit offered two remedies; taking the second, because the first is worse.
Refusing pre-consumption (which I implemented and then reverted) leaves the whole
tail unparsed and drops the TYPE clause as well — a regression against `main`,
which at least kept `[Permanent, Artifact]`.

So the relative clause is now folded INTO the population: the population's type
set is `base_type_filters + relative_core_type_filters`, making it equal to the
candidate set, which is what CR 109.2 asks for. Ranking and candidacy then agree
object-for-object.

A MULTI-type relative clause is a disjunction that `type_filter_branches` spreads
across several branches, which one conjunctive population cannot express, so that
shape is still declined (`len() <= 1`). Zero corpus attestation for either form.

The zone look-ahead is unchanged and still refuses pre-consumption — that
population genuinely is not modelled here, so declining is correct there.

`trailing_relative_type_clause_is_folded_into_the_population` now asserts the
population carries the same type set as the candidates, keeping its reach-guard on
the clause reaching the candidate filter.

Full suite: 17967 lib + 4188 integration passing. fmt clean, clippy-strict clean,
Gate A/G/P pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@matthewevans matthewevans self-assigned this Jul 29, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 29, 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.

Request changes — the new head fixes the single-type relative clause, but still accepts multi-type relative clauses after silently dropping their superlative restriction.

🔴 Blocker

crates/engine/src/parser/oracle_target.rs:2917-2931 consumes the bare with the greatest … head before the trailing relative core types are known. At :3589-3621, relative_core_type_filters.len() > 1 takes the diagnostic-only branch; then :3624-3642 emits separate ordinary TypedFilter branches without the Aggregate property. Thus an Oracle phrase such as target permanent with the greatest mana value that's an artifact or creature is parsed as every eligible artifact or creature, rather than the greatest-mana-value member(s) of that same OR population. IgnoredRemainder is not a strict failure and cannot make that supported target shape safe.

Please either represent the aggregate over the same disjunctive candidate population, or preserve this unsupported rider as an explicit strict failure before consuming it. Add a reach-guarded parser/runtime regression that proves the multi-type phrase reaches this branch and fails if the aggregate/strict-failure behavior is reverted.

🟡 Non-blocking

crates/engine/src/parser/oracle_target.rs:2918-2921, :3571-3578, :3612-3615, and :5022-5034 cite CR 109.2a for a generic non-battlefield-zone guard. The verified rule text limits 109.2a to a description containing both “card” and a zone name; use the general CR 109.2 / CR 400.1 rationale here, reserving 109.2a for the card-plus-zone case.

✅ Clean

The current head correctly folds a single relative core type into the aggregate population; this review is limited to the remaining multi-type branch and the stale citation.

Recommendation: request changes. Preserve the superlative semantics for the complete candidate population (or strictly fail it), add a revert-discriminating test, and correct the generic zone annotation.

@matthewevans matthewevans removed their assignment Jul 29, 2026
…the whole population

Maintainer review on PR phase-rs#6789 (blocker). The previous head folded a SINGLE-type
trailing relative clause into the ranked population but declined the MULTI-type
case, and declining happened at materialization — after the head was already
consumed. So `target permanent with the greatest mana value that's an artifact or
creature` parsed as `Or { [Permanent+Artifact], [Permanent+Creature] }` with NO
aggregate property at all: every eligible artifact or creature was a legal target,
not just the greatest-mana-value member(s). `IgnoredRemainder` is not a strict
failure and cannot make that supported shape safe. Confirmed by probe before fixing.

The population is now representable exactly, with no new engine surface: a
multi-type relative clause becomes ONE conjunctive `TypeFilter::AnyOf` member,
because `base ∧ (A ∨ B) == (base ∧ A) ∪ (base ∧ B)` — precisely the union of the
`Or` legs `type_filter_branches` builds. The prop is pushed into `properties`,
which the branch cross-product replicates onto every leg, so each leg ranks against
the WHOLE population rather than its own type. `TypeFilter::AnyOf` already has full
runtime authority (`game/filter.rs:3093` `type_filter_matches`), and
`type_filter_includes_card` already recurses through it, so the CR 109.2a "card"
carve-out cannot be smuggled past inside a disjunction.

The `relative_core_type_filters.len() <= 1` guard is therefore gone, and with it the
only path by which the aggregate could be dropped for a battlefield population.

Also per the non-blocking finding: CR 109.2a is limited to a description containing
BOTH "card" and a zone name (verified at docs/MagicCompRules.txt:584). The generic
non-battlefield-zone guard is licensed by CR 109.2 (line 582 — the battlefield
default applies only when no zone is named), so the six generic-zone sites are
recited to CR 109.2, with CR 109.2a named only for the "card" leg. The structural
carve-out doc block at `type_filter_includes_card` already cited both correctly and
is unchanged.

Coverage:
- `multi_type_relative_clause_ranks_every_leg_against_the_whole_population` (parser)
  — reach-guarded on the 2-leg `Or`, asserts each leg's population is exactly
  `[Permanent, AnyOf([Artifact, Creature])]`.
- `multi_type_relative_clause_ranks_over_the_disjunctive_population_at_runtime`
  (runtime) — the MV 0 Land is the discriminator that separates the two failure
  modes: dropping the aggregate makes the MV 5 creature legal, while ranking over
  `[Permanent]` alone lets the Land set the bar so nothing is legal and the trigger
  never pauses. Both modes were patched in and confirmed to fail.

Full suite: 17968 lib + 4189 integration passing. fmt clean, clippy-strict clean,
Gate A/G/P pass.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@jofortin

Copy link
Copy Markdown
Contributor Author

Both findings fixed in 30fef29. Thank you for catching the multi-type branch — and for the earlier review too; taken together they walked this from a false-green to something actually sound, and I'd rather have that on record than have it merge.

🔴 Blocker — multi-type relative clause

You're right, and I confirmed it by probe before touching anything rather than reasoning about it:

target permanent with the greatest mana value that's an artifact or creature
  => Or { [Typed(Permanent, Artifact)], [Typed(Permanent, Creature)] }     // no Aggregate anywhere

The restriction was consumed and gone. IgnoredRemainder does not make that safe — agreed, it is a diagnostic, not a strict failure.

I took your first option (represent the aggregate over the same disjunctive population) rather than the strict-failure one, because the population turns out to be exactly representable with no new engine surface. A multi-type relative clause becomes one conjunctive TypeFilter::AnyOf member:

let mut population_types = base_type_filters.clone();
match relative_core_type_filters.as_slice() {
    [] => {}
    [only] => population_types.push(only.clone()),
    many => population_types.push(TypeFilter::AnyOf(many.to_vec())),
}

base ∧ (A ∨ B) == (base ∧ A) ∪ (base ∧ B) — which is precisely the union of the Or legs type_filter_branches builds. The prop then rides in properties, and the branch cross-product replicates it onto every leg, so each leg ranks against the whole population and not just its own type.

Two things I checked before emitting the shape, since a population without runtime authority would just be a different flavour of the same defect:

  • TypeFilter::AnyOf is evaluated at runtime by type_filter_matches (game/filter.rs:3093) — it pre-exists and is not new surface.
  • type_filter_includes_card already recurses through AnyOf, so the CR 109.2a "card" carve-out cannot be smuggled past inside a disjunction.

The relative_core_type_filters.len() <= 1 guard is now gone entirely, so there is no remaining path that drops the aggregate for a battlefield population.

Revert-discriminating coverage

Two rows, and I patched both failure modes back in and re-ran to prove each is caught, rather than asserting it:

Regression patched in What fails
decline the multi-type case (the 094be96 behaviour) parser row on the population assertion; runtime row on "MV 5 is not the lowest — dropping the aggregate makes this legal again"
population = base_type_filters only runtime row: the upkeep trigger must pause for target selection, got Priority { player: PlayerId(1) }
  • multi_type_relative_clause_ranks_every_leg_against_the_whole_population (parser) — reach-guarded on the 2-leg Or so the per-leg assertions cannot pass vacuously, then asserts each leg's population is exactly [Permanent, AnyOf([Artifact, Creature])].
  • multi_type_relative_clause_ranks_over_the_disjunctive_population_at_runtime (runtime, via the real upkeep-trigger pipeline and the engine's own legal_targets) — the MV 0 Land is the discriminator that separates the two modes: dropping the aggregate makes the MV 5 creature legal, while ranking over [Permanent] alone lets the Land set the bar so no artifact or creature matches it, no target is legal, and the trigger never pauses at all.

🟡 Non-blocking — CR 109.2a

Correct, and this was my error. Verified: docs/MagicCompRules.txt:584 — CR 109.2a needs a description containing both "card" and the name of a zone. The rule the generic zone guard is actually reasoning about is CR 109.2 (line 582): the battlefield default applies only to a description that names no zone and contains no "card"/"spell"/"source"/"scheme", so naming a zone is what withdraws it.

The six generic-zone sites now cite CR 109.2, with CR 109.2a named only for the "card" leg (:2902, :2918, :3571, :3618, the nonbattlefield_zone_clause_lies_ahead doc, and the look-ahead test). The structural carve-out doc block at type_filter_includes_card already contrasted CR 109.2 against CR 109.2a correctly and is unchanged. I left the pre-existing :7703 "creature card" citation alone as out of scope.

Verification at 30fef29

cargo fmt --all --check clean · cargo clippy-strict 0 warnings · cargo test -p phase-engine 17968 lib + 4189 integration passing · Gate A PASS head=30fef29e4dee9ac148e09c044e79cb2f709deb23 · Gate G PASS · Gate P PASS. PR body updated with the new head, the corrected finding-2 history, and the parse-diff evidence will refresh on this head.

@matthewevans matthewevans self-assigned this Jul 29, 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: the current head preserves the superlative restriction over the complete candidate population, including disjunctive relative types, with discriminating parser and runtime coverage.

@matthewevans matthewevans added the quality For high-quality minimal to no-churn PRs label Jul 29, 2026
@matthewevans
matthewevans enabled auto-merge July 29, 2026 16:19
@matthewevans matthewevans removed their assignment Jul 29, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 29, 2026
Merged via the queue into phase-rs:main with commit 219e04d Jul 29, 2026
15 checks passed
@jofortin
jofortin deleted the card/culling-scales branch August 3, 2026 05:09
@coderabbitai coderabbitai Bot mentioned this pull request Aug 9, 2026
4 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) quality For high-quality minimal to no-churn PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants