Fix Valakut Exploration — parse the existential exiled-with intervening-if, restoring the swallowed damage clause - #7047
Conversation
…ng-if, restoring the swallowed damage clause
Valakut Exploration's end-step trigger was a fully silent double misparse:
the existential intervening-if ("if there are cards exiled with this
enchantment") was unrecognized — the exiled-with condition family knew
only the subject-first surfaces, and the counted "there are N ..." path
demands a numeral — so the condition dropped to null, AND the unstripped
"if " prefix suppressed the ", then" clause split, silently swallowing
the trailing "then this enchantment deals that much damage to each
opponent" conjunct. Zero warnings, zero Unimplemented: a Valakut
Exploration that never dealt damage and swept nothing (its bare "them"
had no antecedent).
One seam clears both symptoms plus the class:
- parse_card_exiled_with_source_condition gains the existential axis as a
comparator-carrying prefix alt: "there are cards exiled with ~" (GE 1)
and "there are no cards exiled with ~" (EQ 0), composed onto the
existing family (CR 406.6/607.2a/603.4). The counted surfaces keep
their number path; the "there are no cards in your graveyard/library"
neighbor family backtracks at tag("exiled with ") — pinned by unit
negatives.
- The hoisted condition introduces the linked-exile pool as the PLURAL
anaphor antecedent (new ParseContext.plural_object_pronoun_ref, wired
beside the singular object_pronoun_ref; CR 608.2c number agreement) —
"put them ..." now lowers through the existing mass branch as
ChangeZoneAll{origin: Exile, target: ExiledBySource} (Bomat Courier's
proven runtime shape; links consumed per CR 607.2a + CR 406.6), and the
restored ", then" split lowers the damage clause to
DamageEachPlayer{Ref(EventContextAmount)} (Shadowheart's shape) fed by
the sweep's last_effect_count stamp. Number-scoping keeps River Song's
Diary's singular "it" chain byte-identical (pinned).
Full-pool diff: exactly four movers — valakut exploration (full clear),
evercoat ursine (condition + swallow-warning clear), the mysterious
sphere (sweep binds to the pool), search the city (EQ-0 gate attaches).
Fifteen sibling/lookalike cards pinned byte-identical. Coverage:
swallowed-clause warnings -1, Unimplemented count unchanged (honest
partials stay red). Backlog hygiene: Valakut Exploration removed from
root-cause phase-rs#2 (590 -> 589; totals updated).
Runtime note: T1/T2/T4 damage totals are CHARACTERIZATION assertions with
explicit markers — the parse emits the rules-correct shape, but the
engine's per-player previous-effect-count table feeds DamageEachPlayer
each recipient's own swept-card count rather than the total (a
pre-existing amount-channel gap this chain is the first to exercise;
engine-side fix out of scope for a parser PR). T1 still
revert-discriminates via the sweep and the trigger gate (CR 603.4 —
empty pool must not fire, proven).
The mandated fixture regen surfaced arashin sovereign in the
ordering-parity sweep (fixture staleness, byte-identical parse) —
adjudicated as a documented-conservative over-prompt (unprofiled
PutOnTopOrBottom in the RwProfile::conservative() catch-all; CR 603.6c
first-zone check keeps members' writes disjoint), same adjudication as
the sibling phase-rs#7031 branch.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughChangesThe parser recognizes source-linked exile conditions and carries Linked-exile parser flow
Trigger ordering parity documentation
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleTrigger
participant ParseContext
participant OracleEffect
participant ExilePool
participant IntegrationTest
OracleTrigger->>ParseContext: store ExiledBySource plural antecedent
ParseContext->>OracleEffect: resolve them or themselves
OracleEffect->>ExilePool: select source-linked exiled cards
ExilePool-->>OracleEffect: return matching cards
ExilePool->>IntegrationTest: sweep cards to owners' graveyards
Possibly related issues
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (6)
crates/engine/src/parser/oracle_trigger.rs (2)
9183-9188: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the wildcard arm with an exhaustive match.
reads_linked_exile_poolends with_ => false. If a futureTriggerConditionvariant nests sub-conditions, this predicate silently returnsfalsefor it and the plural antecedent is dropped with no compiler error. List the remaining variants explicitly so the compiler flags new nesting shapes.As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".🤖 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_trigger.rs` around lines 9183 - 9188, Update reads_linked_exile_pool to replace the wildcard arm with explicit matches for every remaining TriggerCondition variant, returning false for non-nesting variants. Preserve recursive handling for And, Or, and Not, allowing the compiler to flag any future variants that require traversal.Source: Coding guidelines
9178-9182: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winBind the plural antecedent only for the presence pole.
reads_linked_exile_poolignores the comparator (comparator: _) and also recurses throughNot. A trigger-level intervening-if on the ABSENCE pole — "if there are no cards exiled with ~" (CardsExiledBySource EQ 0), whichparse_card_exiled_with_source_conditionnow produces — therefore installsTargetFilter::ExiledBySourceas the body's plural antecedent. The body then lowers to a mass move over a pool the gate proved empty, which resolves as a silent no-op instead of an honest gap.No shipped card reaches this today: Search the City carries its "no cards" gate at clause level, not on the trigger. The absence pole is now parseable at trigger level, so make the polarity explicit rather than relying on which cards happen to print it.
♻️ Proposed polarity gate
TriggerCondition::QuantityComparison { lhs, - comparator: _, + comparator, rhs, - } => expr_reads_linked_exile_pool(lhs) || expr_reads_linked_exile_pool(rhs), + } => { + // CR 608.2k: only a NON-EMPTY pool can be a plural antecedent; + // the `EQ 0` absence pole introduces no referent. + !matches!(comparator, Comparator::EQ) + && (expr_reads_linked_exile_pool(lhs) || expr_reads_linked_exile_pool(rhs)) + }🤖 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_trigger.rs` around lines 9178 - 9182, Update expr_reads_linked_exile_pool so it reports a linked exile pool only for the presence polarity, not for absence checks such as CardsExiledBySource EQ 0. Use the comparator when handling QuantityComparison and preserve polarity through Not rather than treating every recursive match as a linked-pool read; ensure absence-pole trigger conditions do not install TargetFilter::ExiledBySource as the body’s plural antecedent.crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs (1)
40-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCite the tracking issue in the characterization comments.
These assertions lock in rules-incorrect damage amounts on purpose: the module doc states the correct deltas are -2 to each opponent, and
assert_eq!pins -1 and 0. The intent is documented and the comments say to update the values "when the engine-side channel discrimination lands", but no comment names the tracking issue. The PR description records it as#7046. Add that reference at the module-doc gap section and at each characterization site so the follow-up is discoverable from the test that must change.Also applies to: 147-162
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs` around lines 40 - 54, The characterization comments in the module documentation and each affected assertion site must reference tracking issue `#7046`. Update the ENGINE GAP section and the comments accompanying the characterization assertions around the Valakut sweep damage cases, including the additionally flagged section, so they explicitly identify `#7046` while preserving the existing explanation and assertion values.crates/engine/src/parser/oracle_nom/condition.rs (1)
12339-12346: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winScope the graveyard/library negatives to the arm under test.
These two negatives assert that the whole
parse_inner_conditiondispatcher rejects "there are no cards in your graveyard" and "... in your library". Gorilla Titan and Immortal Coil print exactly that text, so a future arm that legitimately parses it will fail this test even though the exiled-with arm still behaves correctly. Assert againstparse_card_exiled_with_source_conditioninstead; the claim being locked is that this arm does not claim the different-zone family.♻️ Proposed test scoping
assert!( - parse_inner_condition("there are no cards in your graveyard").is_err(), + parse_card_exiled_with_source_condition("there are no cards in your graveyard").is_err(), "graveyard-zone existential must not be claimed by the exiled-with arm" ); assert!( - parse_inner_condition("there are no cards in your library").is_err(), + parse_card_exiled_with_source_condition("there are no cards in your library").is_err(), "library-zone existential must not be claimed by the exiled-with arm" );🤖 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_nom/condition.rs` around lines 12339 - 12346, Scope the two graveyard and library negative assertions to parse_card_exiled_with_source_condition instead of the parse_inner_condition dispatcher. Keep the existing inputs and failure expectations so the test only verifies that the exiled-with arm does not claim other-zone existential conditions.crates/engine/src/parser/oracle_trigger_tests.rs (1)
26727-26737: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winParameterize one traversal helper and widen it past sub/else links.
chain_has_unimplementedwalks onlysub_abilityandelse_ability. AnEffect::Unimplementednested inside aCreateDelayedTrigger's boxed inner def, or inside a modal mode, is invisible to it. These calls are the reach guards that keep the negative assertions in this block non-vacuous, so a blind spot weakens exactly the claim they exist to prove. Neither chain under test carries a delayed trigger today, so this is robustness rather than a live gap.
the_mysterious_sphere_sweep_binds_to_pool_and_create_stays_redthen re-implements the same traversal as a nestedchain_has_named_unimplemented(lines 26995-27005) with a name predicate. One helper taking an optional expected name covers both call sites and keeps the traversal fix in a single place.♻️ Proposed single traversal helper
-/// Recursive Unimplemented walk over an ability chain (sub + else branches). -fn chain_has_unimplemented(ability: &AbilityDefinition) -> bool { - matches!(*ability.effect, Effect::Unimplemented { .. }) - || ability - .sub_ability - .as_deref() - .is_some_and(chain_has_unimplemented) - || ability - .else_ability - .as_deref() - .is_some_and(chain_has_unimplemented) -} +/// Recursive Unimplemented walk over an ability chain (sub, else, and delayed +/// inner defs). `name` restricts the match to one Unimplemented name. +fn chain_has_unimplemented_named(ability: &AbilityDefinition, name: Option<&str>) -> bool { + let head = match (&*ability.effect, name) { + (Effect::Unimplemented { name: n, .. }, Some(expected)) => n == expected, + (Effect::Unimplemented { .. }, None) => true, + _ => false, + }; + let delayed = match &*ability.effect { + Effect::CreateDelayedTrigger { effect, .. } => { + chain_has_unimplemented_named(effect, name) + } + _ => false, + }; + head + || delayed + || ability + .sub_ability + .as_deref() + .is_some_and(|s| chain_has_unimplemented_named(s, name)) + || ability + .else_ability + .as_deref() + .is_some_and(|s| chain_has_unimplemented_named(s, name)) +} + +fn chain_has_unimplemented(ability: &AbilityDefinition) -> bool { + chain_has_unimplemented_named(ability, None) +}🤖 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_trigger_tests.rs` around lines 26727 - 26737, Update chain_has_unimplemented to accept an optional expected ability name and recursively traverse sub_ability, else_ability, CreateDelayedTrigger boxed inner definitions, and modal modes while checking Effect::Unimplemented. Replace the nested chain_has_named_unimplemented in the_mysterious_sphere_sweep_binds_to_pool_and_create_stays_red with this parameterized helper, preserving unnamed and named matching behavior at both call sites.crates/engine/src/parser/oracle_effect/mod.rs (1)
33378-33390: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlign the plural bare-pronoun contract with the singular sibling.
is_bare_object_pronounaccepts"them"/"themselves"too, so its caller contract is shared rather than this being a singular-only predicate. Document that callers must pass a trimmed lowercase pronoun token and add a sync assertion foris_bare_plural_object_pronounso future pronoun-list changes catch regressions.🤖 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/mod.rs` around lines 33378 - 33390, Update the caller contract for the bare object-pronoun helpers, including the code around is_bare_plural_object_pronoun, to document that inputs must be trimmed lowercase pronoun tokens and that the shared contract includes “them” and “themselves.” Add a synchronization assertion covering is_bare_plural_object_pronoun so changes to its pronoun list are detected consistently with is_bare_object_pronoun.
🤖 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/triggers_ordering_parity_tests.rs`:
- Around line 215-222: Clarify the Arashin Sovereign commute invariant in the
nearby comment by explicitly stating that its batches are intended to have a
single controller, or cite the governing rule that guarantees same-controller
resolution for this batch class. Do not imply the broader sweep’s Phase plus
OnlyDuringYourTurn condition establishes this property.
In `@docs/parser-misparse-backlog.md`:
- Around line 6-7: Reconcile the aggregate metrics in
docs/parser-misparse-backlog.md with the ranked table and detailed root-cause
sections by regenerating all counts, headings, lists, and totals from a single
authoritative source. Include root cause 29, resolve the discrepancies for root
causes 19, 27, 30, and 31, and ensure the reported appearances total matches the
auditable section and table sums.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 33378-33390: Update the caller contract for the bare
object-pronoun helpers, including the code around is_bare_plural_object_pronoun,
to document that inputs must be trimmed lowercase pronoun tokens and that the
shared contract includes “them” and “themselves.” Add a synchronization
assertion covering is_bare_plural_object_pronoun so changes to its pronoun list
are detected consistently with is_bare_object_pronoun.
In `@crates/engine/src/parser/oracle_nom/condition.rs`:
- Around line 12339-12346: Scope the two graveyard and library negative
assertions to parse_card_exiled_with_source_condition instead of the
parse_inner_condition dispatcher. Keep the existing inputs and failure
expectations so the test only verifies that the exiled-with arm does not claim
other-zone existential conditions.
In `@crates/engine/src/parser/oracle_trigger_tests.rs`:
- Around line 26727-26737: Update chain_has_unimplemented to accept an optional
expected ability name and recursively traverse sub_ability, else_ability,
CreateDelayedTrigger boxed inner definitions, and modal modes while checking
Effect::Unimplemented. Replace the nested chain_has_named_unimplemented in
the_mysterious_sphere_sweep_binds_to_pool_and_create_stays_red with this
parameterized helper, preserving unnamed and named matching behavior at both
call sites.
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 9183-9188: Update reads_linked_exile_pool to replace the wildcard
arm with explicit matches for every remaining TriggerCondition variant,
returning false for non-nesting variants. Preserve recursive handling for And,
Or, and Not, allowing the compiler to flag any future variants that require
traversal.
- Around line 9178-9182: Update expr_reads_linked_exile_pool so it reports a
linked exile pool only for the presence polarity, not for absence checks such as
CardsExiledBySource EQ 0. Use the comparator when handling QuantityComparison
and preserve polarity through Not rather than treating every recursive match as
a linked-pool read; ensure absence-pole trigger conditions do not install
TargetFilter::ExiledBySource as the body’s plural antecedent.
In `@crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs`:
- Around line 40-54: The characterization comments in the module documentation
and each affected assertion site must reference tracking issue `#7046`. Update the
ENGINE GAP section and the comments accompanying the characterization assertions
around the Valakut sweep damage cases, including the additionally flagged
section, so they explicitly identify `#7046` while preserving the existing
explanation and assertion values.
🪄 Autofix
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: ff494ef5-bd94-4674-9f04-244b5f48e7d3
⛔ Files ignored due to path filters (2)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__valakut_exploration_ir.snapis excluded by!**/*.snap,!**/snapshots/**crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__valakut_exploration_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (11)
crates/engine/src/game/triggers_ordering_parity_tests.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/src/parser/oracle_ir/snapshot_tests.rscrates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/main.rscrates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rsdocs/parser-misparse-backlog.md
Parse changes introduced by this PRBaseline pending for |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The Valakut Exploration parser marks a rules-incorrect damage implementation as supported. Evidence: Scryfall’s current Oracle text says the end-step sweep is followed by damage equal to the total cards moved to each opponent; crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs:1457-1478 explicitly characterizes the shipped result as -1 to one card owner and 0 to another opponent for a two-card sweep, while crates/engine/src/game/effects/mod.rs:7481-7486 collapses mass-move counts to a per-owner maximum/table. CR 608.2c requires applying the ordered English instructions, so “that much” must read the completed sweep total for every opponent. Why it matters: the card becomes parser-supported while knowingly resolving a multiplayer damage clause incorrectly. Suggested fix: retain a strict-failure marker for this damage rider until event-context counts provide a scalar total for mass moves, or include the necessary engine correction and end-to-end total-damage regression in this PR.
[MED] The plural linked-exile antecedent is introduced even when the condition proves the pool empty. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:8047-8059 parses “there are no cards exiled with ~” as CardsExiledBySource == 0, but crates/engine/src/parser/oracle_trigger.rs:9176-9201 detects only that the quantity appears and unconditionally supplies TargetFilter::ExiledBySource, ignoring both comparator and Not. Why it matters: a later plural “them” can bind as if a known-empty linked-exile pool were a valid antecedent, producing unsupported trigger semantics instead of honest strict failure. Suggested fix: derive this antecedent only when the condition semantically entails a nonempty linked-exile pool (including correct boolean/negation handling), or preserve strict failure for forms whose condition cannot establish that guarantee; add an empty-pool comparator regression.
matthewevans
removed
the
needs-maintainer
Review feedback (CodeRabbit): the row's commute argument read as if the sweep's Phase+OnlyDuringYourTurn privacy predicate established the batch's single controller. It never did — the property holds for every batch class by construction: begin_trigger_ordering partitions ordering groups per trigger_order_controller (CR 603.3b — each player orders only the triggers they control; cross-controller placement is APNAP-fixed, not chosen), which the sweep's batch rows model as ControllerUniformity::Uniform (team-pooled groups fail closed to Mixed). Comment-only; ordering_parity_sweep re-run green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head retains two correctness blockers.
🔴 Blocker
[HIGH] Valakut Exploration is marked supported while its end-step damage is knowingly wrong. Evidence: the card's Oracle text says, “put them into their owner's graveyard, then this enchantment deals that much damage to each opponent”; crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs:147-162 asserts -1 for one opponent and 0 for another after two cards are swept. crates/engine/src/game/effects/mod.rs:7481-7486 stores a per-player table and its maximum, while crates/engine/src/game/quantity.rs:3696-3710 gives each damage recipient that recipient's table entry before the scalar fallback. CR 608.2c says to follow the instructions in written order and apply their English meaning, so “that much” here is the completed sweep total (2) for each opponent, not each opponent's ownership share. Why it matters: the PR changes the card from unsupported to silently rules-incorrect, including in multiplayer. Suggested fix: keep this rider strict-failed until the engine can preserve the scalar total for this mass move, or include that engine correction and a full-pipeline -2/-2 regression here.
[MED] The new plural antecedent treats an empty linked-exile condition as proof of a pool to which “them” can refer. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:8047-8059 preserves Comparator::EQ, 0 for “there are no cards exiled with ~”, but crates/engine/src/parser/oracle_trigger.rs:9176-9201 only detects that CardsExiledBySource appears and maps every such condition — including negation — to TargetFilter::ExiledBySource. Why it matters: a later plural pronoun can be lowered as a linked-pool sweep even though the intervening condition establishes that the pool is empty; the parser accepts a semantic relation it has not proved. Suggested fix: derive the antecedent only when the condition entails a nonempty pool, with comparator-aware and negation-aware traversal, and add an empty-pool regression.
Recommendation: request changes. Resolve the amount-channel correctness issue and preserve condition polarity before this parser coverage is accepted.
…or plural antecedents Maintainer review (matthewevans, phase-rs#7047) requested both changes: [HIGH] The mass-move-fed "that much" damage rider parsed to a shape the engine resolves rules-incorrectly (per-recipient own-count instead of the CR 608.2c sweep total — 0 to every opponent in Valakut Exploration's native pattern), while coverage counted the card supported. The rider now lowers to the honest strict-failure marker Effect::unimplemented(MASS_MOVE_TOTAL_DAMAGE_GAP, <verbatim clause>) via a typed gate at the single chain-assembly seam, scoped to the one categorically-broken pairing: ChangeZoneAll immediately followed by DamageEachPlayer reading Ref(EventContextAmount). Corpus census: exactly one member (Valakut). Scalar consumers (Draw/DealDamage-to-object/Mill/ LoseLife/SearchLibrary) stay un-gated — every corpus pool is single-owner by CR 400.3, so max-over-owners equals the true total there (the Whirlpool Drake class regresses nothing; pinned by hostile fixtures). Deletes when phase-rs#7046 lands the completed-sweep scalar-total channel; the zero-delta runtime assertions and the exact-unit marker test are the tripwires forcing that flip to be conscious. Coverage: supported -1 (Valakut honestly red on the rider), Unimplemented +1, warnings ±0. [MED] trigger_plural_object_pronoun_ref_for_intervening_if derived the plural pool antecedent whenever CardsExiledBySource appeared in the condition, ignoring comparator and Not — an EQ-0 "no cards" gate would bind "them" to a provably-empty pool. The derivation is now entailment-gated over the full composite algebra (GE n>=1 / GT n>=0 / EQ n>=1 / NE 0 at positive polarity; And-any / Or-all-nonempty; Not never derives, conservatively). Eleven unit regressions; zero corpus movers (every wiring-reachable condition is positive GE — scanned). Also: QuantityExpr::any_ref relocated verbatim from the game crate's private traversal (one authority, one-line delegation kept); the census test's line pin re-established with a DRIFT LOG entry per its in-file precedent (-16 line shift, content sha-identical); T1's planned unimplemented_oracle_ids assertion was empirically falsified and dropped - resolve_chain_body short-circuits Unimplemented before resolve_effect, so the telemetry insert is dead code on the stack-resolution path (a pre-existing engine gap, root-caused with line refs in the module doc). Full-pool diff: exactly one mover (the Valakut rider -> marker). The lowered snapshot updates; the IR snapshot is byte-identical (pinned). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv
|
Both findings addressed in [HIGH] — taken as your option (a). The mass-move-fed damage rider now lowers to an honest strict-failure marker ( [MED] — fixed as specified. The plural-antecedent derivation is now entailment-gated over the full composite algebra: One pre-existing engine gap surfaced during the round and is documented in-file with exact line refs rather than worked around: PR body and handoff block updated; full suite, combinator gate, clippy, fmt all green at |
There was a problem hiding this comment.
Actionable comments posted: 5
🤖 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_effect/assembly.rs`:
- Around line 1988-2007: Broaden the coverage gate in assemble_effect_chain
beyond DamageEachPlayer so every EventContextAmount consumer cannot execute
against per-owner mass-move counts. Use the aggregate completed-move total where
supported, or emit Effect::Unimplemented for scalar consumers when ChangeZoneAll
may span multiple owners. Add an integration scenario with linked cards owned by
two players verifying the scalar follower uses the aggregate total rather than
the maximum owner count. Update the associated rule annotations to describe
mass-move aggregation instead of CR 608.2c and CR 400.3.
In `@crates/engine/src/parser/oracle_trigger_tests.rs`:
- Around line 27103-27110: Update the landfall assertion in the trigger test to
require `landfall.execute` with `expect("landfall execute ability")` before
calling `count_unimplemented_in_chain`, so a missing execute body fails the
test. Add or preserve a positive reach guard demonstrating that the input
reached the code under test.
- Around line 26818-26833: Update the trigger antecedent entailment helper used
by trigger_plural_object_pronoun_ref_for_intervening_if to normalize negated
fixed-count CardsExiledBySource comparisons to their inverse comparator before
consulting the existing entailment table. Preserve conservative behavior for
unsupported Not conditions, and add coverage ensuring Not(EQ 0) returns
Some(TargetFilter::ExiledBySource) rather than None.
- Around line 26998-27007: Update the explanatory annotation for the Valakut
Exploration trigger near the `Effect::Unimplemented` marker: remove the
incorrect `CR 608.2k` reference from the linked-exile pool and plural-anaphor
explanation, retain `CR 406.6` and `CR 607.2a` for that relationship, and cite
`CR 608.2c` only for the written-order behavior of `then`.
In `@crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs`:
- Around line 177-199: In
crates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs at
lines 177-199, inspect the queued three-player end-step ability before stack
advancement and assert its continuation is Effect::Unimplemented with
MASS_MOVE_TOTAL_DAMAGE_GAP. Apply the same positive reach assertion to the
landfall-created end-step ability at lines 275-285 and the per-source scenario
at lines 354-362, before each corresponding stack advancement; retain the
existing zero-life-delta checks.
🪄 Autofix
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: b031e1d6-1b8a-4087-8131-b80b998b1270
⛔ Files ignored due to path filters (1)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__valakut_exploration_lowered.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (8)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/engine.rscrates/engine/src/parser/oracle_effect/assembly.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/ability.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/parser/oracle_trigger.rs
# Conflicts: # crates/engine/src/game/engine.rs
|
Maintainer update: current head is |
|
Review follow-up completed on the refreshed head:
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head: linked-exile parsing is rules-annotated, strict-fails the known aggregate gap, and has parser plus runtime coverage.
|
Current-head review is held at
The current implementation review found no additional code finding, and the prior approval is retained. Auto-merge is disabled until a parse-diff artifact explicitly bound to this head and a fully green required-check rollup are available; re-review/enqueue can then resume without a branch change. |
|
Current-head review is held at
The current implementation review found no additional code finding: this head only escapes braces in two assertion messages. The prior approval is retained. Auto-merge is disabled until a parse-diff artifact explicitly bound to this head and a fully green required-check rollup are available; re-review/enqueue can then resume without a branch change. |
|
Current-head review is held at
The current implementation review found no additional contributor-code finding: this head merges current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head recheck at afbe880: the post-approval delta is a maintainer merge plus escaped assertion-message braces; the reviewed linked-exile strict-failure path and its runtime reach guards are unchanged. Required checks are green, and the PR remains queued.
2 participants
Summary
Fixes Valakut Exploration's silent double misparse (a §3.1 backlog item, root-cause #2): the end-step trigger's existential intervening-if ("if there are cards exiled with this enchantment") was unrecognized — condition dropped to
null— and, same root cause, the unstripped"if "prefix suppressed the", then"clause split, silently swallowing the damage clause. Zero warnings, zero Unimplemented: the card swept nothing and never dealt damage. One seam clears both: the exiled-with condition family gains the existential prefix axis ("there are [no] cards exiled with ~"→ GE 1 / EQ 0), and the hoisted condition introduces the linked-exile pool as the plural-anaphor antecedent, so"put them …"lowers to the provenChangeZoneAll{Exile, ExiledBySource}sweep and the restored", then"split lowers the damage clause toDamageEachPlayer{Ref(EventContextAmount), Opponent}. Relates to #7046 (engine-side amount-channel gap, disclosed below).Files changed
crates/engine/src/parser/oracle_nom/condition.rs— the existential prefix axis + unit tests (incl. sibling negatives)crates/engine/src/parser/oracle_ir/context.rs—plural_object_pronoun_ref(number-scoped sibling ofobject_pronoun_ref)crates/engine/src/parser/oracle_trigger.rs—trigger_plural_object_pronoun_ref_for_intervening_if+ one wiring linecrates/engine/src/parser/oracle_effect/mod.rs—is_bare_plural_object_pronoun+ the plural-pool arm routed through the existing mass branchcrates/engine/src/parser/oracle_trigger_tests.rs— SHAPE tests (Valakut, Evercoat Ursine, Search the City, The Mysterious Sphere, River Song's Diary pin)crates/engine/src/parser/oracle_ir/snapshot_tests.rs+ two Valakut.snapfilescrates/engine/tests/integration/valakut_exploration_end_step_exile_sweep.rs+main.rsmod linecrates/engine/tests/fixtures/integration_cards.json— regenerated viascripts/gen-test-fixture.pycrates/engine/src/game/triggers_ordering_parity_tests.rs— oneDOCUMENTED_OVER_PROMPTledger row (arashin sovereign, fixture-regen surfaced; same adjudication as the Fix Sauron, Dino Devotee — restore modal mode-body subject filtering #7037 branch)docs/parser-misparse-backlog.md— §3.1 list hygiene (Valakut removed from root-cause chore: update coverage stats and badges #2; counts updated)Track
Developer
LLM
Model: claude-fable-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 404.1, 406.6, 603.4, 603.5, 603.6c, 607.2a, 608.2c, 608.2k (all grep-verified against
docs/MagicCompRules.txt; 406.6/607.2a authorize the linked-exile reads, 603.4 the intervening-if, 608.2c/608.2k the anaphora)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./scripts/check-parser-combinators.sh— Gate A/G PASS at headcargo clippy --all-targets -- -D warnings— cleancargo test -p phase-engine— 23,127 passed, 0 failed (includes the enlarged ordering-parity sweep with the ledger entry consumed,over_prompt_hitexact-set green)Full-pool parse diff (post vs same-SHA baseline) — movers exactly {valakut exploration, evercoat ursine, the mysterious sphere, search the city}, per-card deltas as planned; 15 sibling/lookalike cards byte-identical; swallowed-clause warnings −1 (Evercoat), Unimplemented count unchanged
cargo coverage— supported 31673→31674;cargo semantic-audit— all four movers cleanDiscriminating-test proof (run twice: implementation and review) — with the existential prefixes neutralized: P2 fails (
condition: None), T1 fails (cards stay in Exile), T3 fails (the ungated trigger fires,stack: ["Valakut Exploration"]); restored byte-identical (sha-verified), all greenLocal-only note: the
phase-aitiming testvelocity_score_projection_deadline_is_live_on_a_traversing_fixturefails on this dev machine at pristine base (hardware-speed artifact, same as documented on Fix Sauron, Dino Devotee — restore modal mode-body subject filtering #7037); deferring to CI as that check's ownerGate A
Gate A PASS head=a3d54304891b70a75a53b4164f3c57452b5193c0 base=ce15372c5b44f4a4699ce577ae62d801b9c9fd9c
Anchored on
parse_there_are_conditions: the counted existential sibling whose number path the new axis composes beside (same family, same registration block)trigger_object_pronoun_ref_for_intervening_if: the singular antecedent-introduction seam the new plural helper mirrors one line belowFinal review-impl
Final review-impl PASS head=a3d54304891b70a75a53b4164f3c57452b5193c0
Claimed parse impact
valakut exploration(condition + sweep + damage chain restored),evercoat ursine(condition attached; SwallowedClause warning cleared with reach-guards),the mysterious sphere(sweep binds to the linked pool),search the city(EQ-0 gate attaches to the sacrifice sub). No other card's parse bytes change (full-pool diff verified; the fixture regen additionally absorbs upstream staleness — see Scope Expansion).Scope Expansion
Two regen-driven absorptions, same phenomena reviewed on the #7037 branch: (1) the
integration_cards.jsonregen absorbed pre-existing staleness (~91 added keys, metadata/parse refreshes; identity fields untouched; byte-identical to the sibling branch's reviewed regen except this fix's own movers and two cards whose shapes come from base commit #7034); (2) the enlarged sweep corpus surfacedarashin sovereignin the ordering-parity proof-gate — adjudicated as a documented-conservative over-prompt (unprofiledPutOnTopOrBottomin the fail-closedRwProfile::conservative()catch-all), ledger row identical to the #7037 branch's reviewed row.Resolved per maintainer review (2026-08-06): the damage rider is now an honest strict-failure marker. The mass-move-fed "that much" damage clause no longer parses to an executable shape — a typed gate at the chain-assembly seam (scoped to exactly
ChangeZoneAllimmediately followed byDamageEachPlayer{Ref(EventContextAmount)}; corpus census: one member) lowers it toEffect::unimplemented("mass_move_total_damage", <verbatim clause>), following theunbound_subject/#6965 precedent. Valakut Exploration therefore counts unsupported in coverage (supported −1, Unimplemented +1, warnings ±0) until #7046 lands the completed-sweep scalar-total channel; the zero-delta runtime assertions and the exact-unit marker test are the tripwires that force that flip to be a conscious, tested change. Scalar consumers (the Whirlpool-Drake Draw class, DealDamage-to-object, Mill, LoseLife, SearchLibrary) stay un-gated — every corpus pool is single-owner by CR 400.3 so max-over-owners equals the true total there — pinned by hostile fixtures (Shadowheart / Whirlpool Drake / Caldera Breaker / fixed-amount). The review's second finding (the plural-antecedent derivation ignored comparator andNot) is fixed by entailment-gating the derivation over the full composite algebra, with eleven unit regressions and zero corpus movers. One pre-existing engine gap surfaced and was documented in-file with line refs rather than papered over:resolve_chain_bodyshort-circuitsUnimplementedbeforeresolve_effect, so theunimplemented_oracle_idstelemetry insert is dead code on the stack-resolution path.Validation Failures
None.
CI Failures
None at time of opening (the local
phase-aitiming-test artifact is documented under Verification).Pipeline report
Plan-review loop: 2 rounds to CLEAN (round 1: "no blocking gaps" + 4 documentation findings; round 2: CLEAN). Implementation: authoring executor + completion executor + 1 fix round (the parity ledger row). Implementation review: CLEAN at the committed head, with the reviewer independently re-running the neutralization experiment, auditing all 1,275 fixture refreshes against the sibling branch's reviewed regen, and adjudicating the amount-channel deviation (disclosures above are per that adjudication).
Pipeline-reviewed head: 1a57414
Current branch head: 1a57414
Pipeline status: current
Current-head review: clean at 1a57414
🤖 Generated with Claude Code
https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv
Summary by CodeRabbit
New Features
Bug Fixes
Documentation