Skip to content

fix(engine): read entered-this-turn quantities from the entry ledger (BB-FU10) - #6323

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
lgray:fix/bbfu10-entered-this-turn-snapshot
Jul 22, 2026
Merged

fix(engine): read entered-this-turn quantities from the entry ledger (BB-FU10)#6323
matthewevans merged 1 commit into
phase-rs:mainfrom
lgray:fix/bbfu10-entered-this-turn-snapshot

Conversation

@lgray

@lgray lgray commented Jul 22, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Summary

parse_entered_this_turn_ref emitted the live-board QuantityRef::EnteredThisTurn for Oracle text whose "entered the battlefield under your control this turn" clause is an entry event, so a permanent that entered this turn and has since left stopped counting — contradicting CR 608.2i (look-back). This migrates only the entry-event surface to the ledger-backed QuantityRef::BattlefieldEntriesThisTurn { player, filter }, leaves the subject-noun surface (Tromell, CR 608.2h live set) alone, and repairs three defects the migration exposed in the entry-record matcher.

Implementation method (required)

Method: /engine-implementer

CR references

CR 608.2i (look-back — the migration's basis), CR 608.2h (once-determined live set — why Tromell must NOT migrate), CR 702.73a (Changeling CDA), CR 205.3m (creature-type namespace), CR 403.3 (permanents exist only on the battlefield), CR 109.1, CR 109.2, CR 109.5, CR 111, CR 400, CR 102.2, CR 102.3, CR 205.4b, CR 308.1, CR 603.3b, CR 603.6a, CR 611.3a, CR 701.21, CR 732.2a.

Every number was grep-verified against docs/MagicCompRules.txt before being written; the whole-diff gate reports zero unverified. The pre-existing // CR 608.2b: Disjunction miscitation at filter.rs:2833 was deliberately not propagated (neither 608.2b nor 608.2i is a disjunction rule).

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 — exit 0

  • cargo clippy --all-targets -- -D warnings — exit 0, zero diagnostics

  • cargo nextest run -p engine21258 passed / 0 failed / 8 skipped

  • cargo nextest run -p engine -E 'test(combo_infinite_pile) | test(kilo_live_offer_from_real_dump) | test(sprout_inalla_realistic_offer) | test(loop_shortcut)'96/96 passed

  • ./scripts/check-parser-combinators.sh — exit 0 (Gate G + Gate A PASS)

  • cargo combo-verify13 confirmed / 4 gated / 37 deferred / 0 failed (of 54); all 54 rows row-for-row identical to base

  • BASE-vs-POST data/card-data.json — 35477 cards both sides, 0 added, 0 removed, 5 changed (see Claimed parse impact)

  • BASE-vs-POST data/coverage-data.jsonsupported_cards 31504 both sides, supported-flip multiset diff 0 / 0

  • FORGE_TEST_FULL_DB=1 … ordering_parity_sweepunexplained=18 on both base and head, all metrics and all 18 rows byte-identical (delta zero). Pre-existing Urza-block failure, opt-in gate, not default CI.

Base and head card-data were generated from md5-identical MTGJSON inputs (AtomicCards.json 0dd2d256…, verified after both runs), so every delta above is code-caused, not input drift.

Gate A

Gate A PASS head=96d5a3b0b434052651e4a95a77941839bcd7fb18 base=836ff312ae2073c99af28d286b0c4915faa8a458

Anchored on

  • crates/engine/src/game/filter.rs:2840zone_change_record_matches_type_filter: the existing snapshot-record type-filter matcher that already threads all_creature_types live against a snapshotted record. Step 8 mirrors it exactly.
  • crates/engine/src/parser/oracle_nom/condition.rs:7725parse_or_more_entered_count: BB-FU1's already-shipped condition-side emitter of the same ledger variant, i.e. the sibling surface this PR's quantity-side migration matches.

Final review-impl

Final review-impl PASS head=96d5a3b0b434052651e4a95a77941839bcd7fb18

Claimed parse impact

  • Hobgoblin Bandit Lord
  • Geralf, the Fleshwright
  • Cloudspire Coordinator
  • Kinbinding
  • Bioengineered Future

Three further cards change behavior with no AST change, so they cannot appear in a card-data diff — measured and confirmed absent from it:

  • Cleaving Reaper
  • Lilypad Village
  • Nimble Trapfinder

They benefit from the TargetFilter::Or repair in the entry-record matcher; two of them gain a previously-impossible activation. The unlock is driven, not asserted: bbfu10_lilypad_village_activation_unlocked_by_composite_fix drives check_activation_restrictions and measures ILLEGAL → LEGAL.

Disclosures

1. A CR 732.2a firewall reclassification suppresses three shipping combo offers.
ability_scan.rs classified BattlefieldEntriesThisTurn as sibling: false, while analysis/resource.rs already clears battlefield_entries_this_turn as "append-only event journals a loop pumps". Those two statements contradict each other, and the combination lets the engine hand out a false ∞ certificate today. This PR flips the axis to sibling: true, which is what the module's own ⛔ INVARIANT requires.

Measured cost: three battlefield permanents — Park Heights Pegasus, Smuggler's Share, and The Prydwen, Steel Flagship (one card; the comma is part of its name) — stop receiving a CR 732.2a loop-shortcut offer they receive today. This is a deliberate, disclosed trade, not an oversight; restoring them under a narrower predicate is tracked as follow-up BB-FU10-N. loop_shortcut.rs carries a driven regression test that asserts the suppression and names BB-FU10-N as the item that will flip it back.

DB-wide census (measured at the pre-rebase base e48c7d6e1; the suppression itself is pinned at the current head by a committed driven test): 35454 faces, sib_true 16939 → 16945; the six added faces are exactly the predicted set and the removed set is empty. Step 1 in isolation measures 16937 — it relaxes Geralf and Hobgoblin Bandit Lord, which is precisely the false-certificate relaxation Step 0c closes.

2. Not fixed, and deliberately so. The entry-record snapshot is taken pre-layer, so only Changeling is repairable from it (the keyword travels; granted subtypes do not). 192 MTGJSON cards grant types via continuous effects and remain invisible to any snapshot-only matcher. ParsedCondition::YouHadAngelOrBerserkerEnterThisTurn still compares subtypes with a bare eq_ignore_ascii_case, so Cleaving Reaper's Changeling axis stays wrong — that arm never routes through the matcher, so this PR's chokepoint cannot reach it. Our Cleaving Reaper claim above is about the ledger-count axis only.

3. Coverage classification is filter-aware now, but only where the classifier looks. extract_card_features walks static-ability conditions and effect positions (21 corpus cards reach the ledger arm, all Handled) but treats a trigger intervening-if as opaque, so it cannot see 20 measured trigger-side cards. Tunnel Tipsterface-down creature entered … this turn — is therefore still silently supported=true with a constant-false condition. That is a pre-existing BB-FU1-era defect, disclosed here, not introduced and not fixed.

4. Instrument blindness, stated plainly. The combo corpus contains none of the three suppressed cards, so combo-verify's row-for-row identity is expected and is not evidence that no offer was suppressed. The suppression was measured directly by DB census and by a driven fixture instead.

Re-verified after rebase

Rebased onto upstream/main = 8fe91f145 (29 commits). Every gate above was re-measured at the rebased head against a freshly generated base, and every result is identical to the pre-rebase measurement — including the blast radius holding at exactly the same 5 cards, which was the drift most worth checking given the upstream tip (fix(parser): stamp Permanent on duration-less additive type grants) touches this PR's seam. A dedicated review pass confirmed no semantic interaction: this PR's condition.rs diff is comment-only, and entry_type_filter_matches reads the pre-layer entry snapshot, which never observed the grant under either duration.

Summary by CodeRabbit

  • New Features

    • Improved handling of “entered the battlefield this turn” effects, including controller-specific and combined type conditions.
    • Departed permanents that entered this turn are now counted correctly where applicable.
    • Improved support for subtype checks, including Changeling interactions.
  • Bug Fixes

    • Fixed ledger-based conditions that could previously return incorrect counts or remain inactive.
    • Prevented unsupported conditions from producing misleading results.
    • Improved loop detection when battlefield-entry effects depend on changing board state.
  • Documentation

    • Clarified rules references and behavior for battlefield-entry look-back effects.

…(BB-FU10)

`parse_entered_this_turn_ref` emitted the live-board `QuantityRef::EnteredThisTurn`
for Oracle text whose "entered the battlefield under your control this turn" clause
is an entry event, so a permanent that entered this turn and has since left stopped
counting — contradicting CR 608.2i (look-back).

Migrate only the entry-event surface to the ledger-backed
`QuantityRef::BattlefieldEntriesThisTurn { player, filter }`. The subject-noun
surface (Tromell) stays on the live read per CR 608.2h; the discriminator is the
controller attachment site, not the substring "the battlefield".

The migration exposed three defects in the entry-record matcher, all repaired here:

- `TargetFilter::Or`/`And` fell to a fail-closed wildcard, so composite filters
  counted 0. Making them monotone unlocks three shipped cards, two of which gain a
  previously-impossible activation (driven, not asserted).
- `TypeFilter::Subtype` compared strings directly, bypassing
  `subtype_matches_with_changeling` — the declared single authority — so a
  changeling entrant stopped counting for exactly the cards this change migrates
  (CR 702.73a). Now routed through it, mirroring
  `zone_change_record_matches_type_filter`.
- Filters carrying a `FilterProp` the entry snapshot cannot answer were emitted
  anyway and resolved to a silent constant 0. They now refuse the parse and lower
  to an honest `Effect::Unimplemented`.

Reclassify `BattlefieldEntriesThisTurn` as `sibling: true` for the CR 732.2a loop
firewall. `analysis/resource.rs` already clears the entry journal as a loop-pumped
append-only event, so the previous `sibling: false` let the engine hand out a false
infinite-loop certificate. This suppresses a shortcut offer for three shipping
permanents; restoring them under a narrower predicate is tracked as BB-FU10-N and
pinned by a driven regression test.

Assisted-by: ClaudeCode:claude-opus-4-8
@lgray
lgray requested a review from matthewevans as a code owner July 22, 2026 05:47
@coderabbitai

coderabbitai Bot commented Jul 22, 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: 75df04bd-197f-4457-b1c3-ef2e61622892

📥 Commits

Reviewing files that changed from the base of the PR and between 094d4e5 and 96d5a3b.

📒 Files selected for processing (15)
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/mod.rs
  • crates/engine/src/game/filter.rs
  • crates/engine/src/game/quantity.rs
  • crates/engine/src/game/restrictions.rs
  • crates/engine/src/parser/oracle.rs
  • crates/engine/src/parser/oracle_nom/condition.rs
  • crates/engine/src/parser/oracle_nom/quantity.rs
  • crates/engine/src/parser/oracle_tests.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/tests/integration/bbfu10_entered_this_turn_snapshot.rs
  • crates/engine/tests/integration/loop_shortcut.rs
  • crates/engine/tests/integration/main.rs

📝 Walkthrough

Walkthrough

The change migrates qualifying entered-this-turn quantities to battlefield-entry ledger reads, adds runtime-aware subtype matching and fail-closed coverage checks, updates loop analysis, and adds parser, engine, and integration regressions for departed entries and object-growth suppression.

Changes

Battlefield Entry Ledger Semantics

Layer / File(s) Summary
Parse entered-this-turn ledger readings
crates/engine/src/parser/oracle_nom/quantity.rs, crates/engine/src/parser/oracle.rs, crates/engine/src/parser/oracle_nom/condition.rs, crates/engine/src/types/ability.rs, crates/engine/tests/...
Controller-qualified entered-this-turn clauses now produce BattlefieldEntriesThisTurn with PlayerScope controller scoping, while live-population readings remain EnteredThisTurn; nested filter-property scanning and CR 608.2i documentation are updated.
Evaluate ledger entry filters
crates/engine/src/game/restrictions.rs, crates/engine/src/game/filter.rs, crates/engine/src/game/effects/mod.rs
Entry-record matching receives the runtime creature-type catalog, supports Changeling-aware subtype evaluation and recursive connectives, and remains fail-closed for unevaluable filters.
Propagate ledger reads through engine analysis
crates/engine/src/game/quantity.rs, crates/engine/src/game/ability_scan.rs, crates/engine/src/game/ability_utils.rs, crates/engine/src/game/coverage.rs
Ledger quantities become population-sensitive, matching entrants perturb their counts, target-slot filters recurse through ledger references, and loop-firewall scanning classifies them as sibling-mutable.
Validate migrated ledger behavior
crates/engine/tests/integration/bbfu10_entered_this_turn_snapshot.rs, crates/engine/tests/integration/loop_shortcut.rs, crates/engine/tests/integration/main.rs
Regression coverage verifies departed-entry counting, controller and composite filters, replacement-path reads, unsupported filters, live-routing exceptions, activation conditions, and Park Heights Pegasus loop-offer suppression.

Estimated code review effort: 5 (Critical) | ~120 minutes

Suggested labels: bug

Suggested reviewers: matthewevans

🚥 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 main change: entering-this-turn quantities now read from the entry ledger.
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.

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

Reviewed the current head.

I found no additional concrete correctness blocker in the migrated entry-event path. The ledger migration, the preserved live subject-noun path, and the entry-record filter guard follow the existing condition-side model.

This remains unapproved pending the in-progress CI and current-head card-data/coverage evidence. No local build was run.

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR · 4 card(s), 4 signature(s) (baseline: main 094d4e5630de)

🟡 Modified fields (4 signatures)

  • 1 card · 🔄 ability/DealDamage · changed field amount: you control Goblin entered this turnbattlefield entries this turn (Goblin, you)
    • Affected (first 3): Hobgoblin Bandit Lord
  • 1 card · 🔄 ability/PutCounter · changed field counter: EnteredThisTurn { filter: Typed(TypedFilter { type_filters: [Land], controller: Some(You), properties: [] }) } P1P1BattlefieldEntriesThisTurn { player: Controller, filter: Typed(TypedFilter { type_filters: [Land], controller: None, pr…
    • Affected (first 3): Bioengineered Future
  • 1 card · 🔄 ability/PutCounter · changed field counter: EnteredThisTurn { filter: Typed(TypedFilter { type_filters: [Subtype("Zombie")], controller: Some(You), properties: [An…BattlefieldEntriesThisTurn { player: Controller, filter: Typed(TypedFilter { type_filters: [Subtype("Zombie")], control…
    • Affected (first 3): Geralf, the Fleshwright
  • 1 card · 🔄 ability/Token · changed field token: you control Mount or you control Vehicle entered this turn× +1/+1 Pilot (Creature Pilot)battlefield entries this turn (Mount or Vehicle, you)× +1/+1 Pilot (Creature Pilot)
    • Affected (first 3): Cloudspire Coordinator

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

@matthewevans matthewevans self-assigned this Jul 22, 2026
@matthewevans matthewevans added the bug Bug fix label Jul 22, 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 entry-event grammar now selects the ledger only when control modifies the entry event, retains the live subject-noun path, and exercises current-head parser, quantity, activation, and layer-invalidation paths. The parse-diff’s four affected cards match the migrated semantic shape; no quality label is requested because the claimed and measured card sets differ.

@matthewevans
matthewevans added this pull request to the merge queue Jul 22, 2026
@matthewevans matthewevans removed their assignment Jul 22, 2026
Merged via the queue into phase-rs:main with commit 1b11783 Jul 22, 2026
14 checks passed
jsdevninja pushed a commit to jsdevninja/phase that referenced this pull request Jul 24, 2026
…(BB-FU10) (phase-rs#6323)

`parse_entered_this_turn_ref` emitted the live-board `QuantityRef::EnteredThisTurn`
for Oracle text whose "entered the battlefield under your control this turn" clause
is an entry event, so a permanent that entered this turn and has since left stopped
counting — contradicting CR 608.2i (look-back).

Migrate only the entry-event surface to the ledger-backed
`QuantityRef::BattlefieldEntriesThisTurn { player, filter }`. The subject-noun
surface (Tromell) stays on the live read per CR 608.2h; the discriminator is the
controller attachment site, not the substring "the battlefield".

The migration exposed three defects in the entry-record matcher, all repaired here:

- `TargetFilter::Or`/`And` fell to a fail-closed wildcard, so composite filters
  counted 0. Making them monotone unlocks three shipped cards, two of which gain a
  previously-impossible activation (driven, not asserted).
- `TypeFilter::Subtype` compared strings directly, bypassing
  `subtype_matches_with_changeling` — the declared single authority — so a
  changeling entrant stopped counting for exactly the cards this change migrates
  (CR 702.73a). Now routed through it, mirroring
  `zone_change_record_matches_type_filter`.
- Filters carrying a `FilterProp` the entry snapshot cannot answer were emitted
  anyway and resolved to a silent constant 0. They now refuse the parse and lower
  to an honest `Effect::Unimplemented`.

Reclassify `BattlefieldEntriesThisTurn` as `sibling: true` for the CR 732.2a loop
firewall. `analysis/resource.rs` already clears the entry journal as a loop-pumped
append-only event, so the previous `sibling: false` let the engine hand out a false
infinite-loop certificate. This suppresses a shortcut offer for three shipping
permanents; restoring them under a narrower predicate is tracked as BB-FU10-N and
pinned by a driven regression test.

Assisted-by: ClaudeCode:claude-opus-4-8
@coderabbitai coderabbitai Bot mentioned this pull request Aug 20, 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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants