fix(engine): resolve batched attack 'each of them' counter to the attach batch - #6310
Conversation
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
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:
📝 WalkthroughWalkthroughThe parser binds “each of them” to ChangesAttack trigger batch targets
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant AttackTrigger
participant TargetResolver
participant GameState
OracleParser->>AttackTrigger: lower “each of them” as ParentTarget
AttackTrigger->>TargetResolver: resolve batched ParentTarget
TargetResolver->>GameState: read declared attacker references
GameState-->>TargetResolver: return attacker TargetRefs
TargetResolver-->>AttackTrigger: return attacking object IDs
AttackTrigger->>GameState: place counters on matching Insects
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
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
🤖 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_trigger.rs`:
- Around line 2080-2088: Update
demote_batch_attack_counter_all_to_parent_in_ability to recursively traverse
abilities installed by with_modal(...), in addition to sub_ability and
else_ability, so nested modal branches also convert PutCounterAll counters. Add
a runtime regression covering a batched attack trigger whose counter instruction
is inside a selected modal mode.
- Around line 1630-1633: Preserve Vrestin’s filtered triggering attackers when
lowering and resolving the effect: in
crates/engine/src/parser/oracle_trigger.rs:1630-1633, carry the selected
attacker subset or typed filter instead of lowering to an unqualified
ParentTarget; in crates/engine/src/game/effects/counters.rs:1635-1648, resolve
the retained selected batch rather than the raw AttackersDeclared.attacker_ids
list. Keep the solution composable for filtered attack-trigger effects and
faithful to the applicable rules.
🪄 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: 2ae520eb-d7dc-4c3d-805c-12d2ab71c2ef
📒 Files selected for processing (5)
crates/engine/src/game/effects/counters.rscrates/engine/src/game/targeting.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/vrestin_menoptra_leader_5949.rs
| fn demote_batch_attack_counter_all_to_parent_in_ability(ability: &mut AbilityDefinition) { | ||
| demote_batch_attack_counter_all_to_parent(ability.effect.as_mut()); | ||
| if let Some(sub) = ability.sub_ability.as_deref_mut() { | ||
| demote_batch_attack_counter_all_to_parent_in_ability(sub); | ||
| } | ||
| if let Some(else_ability) = ability.else_ability.as_deref_mut() { | ||
| demote_batch_attack_counter_all_to_parent_in_ability(else_ability); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Traverse modal mode abilities as well.
This walker skips the abilities installed by with_modal(...). A batched attack trigger whose counter instruction is inside a selected mode remains PutCounterAll and still resolves as a no-op. Walk modal branches and add a runtime regression for that path.
As per path instructions, “new enum variants, parsers, effects, and filters must support a category of cards.”
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/engine/src/parser/oracle_trigger.rs` around lines 2080 - 2088, Update
demote_batch_attack_counter_all_to_parent_in_ability to recursively traverse
abilities installed by with_modal(...), in addition to sub_ability and
else_ability, so nested modal branches also convert PutCounterAll counters. Add
a runtime regression covering a batched attack trigger whose counter instruction
is inside a selected modal mode.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the runtime batch resolution is sound, but the parser defect is repaired at a trigger-only seam.
🔴 Blocker
crates/engine/src/parser/oracle_trigger.rs:1624-1633rewritesPutCounterAllonly after an attack trigger has already been lowered. The misclassification is created earlier in the shared targeted-imperative branch atcrates/engine/src/parser/oracle_effect/mod.rs:15092-15106, which promotes every mass-looking phrase toPutCounterAllwithout excludingTargetFilter::ParentTarget. Its sibling authority atcrates/engine/src/parser/oracle_effect/imperative.rs:12120-12139already applies exactly that exclusion for “each of them.” This trigger-only repair duplicates parser semantics, misses the same anaphoric counter instruction in other batched event families, and requires a recursive ability walker that currently omitsmode_abilities(oracle_trigger.rs:2080-2088). CR 608.2c and Vrestin’s verified Oracle text—“Whenever you attack with one or more Insects, put a +1/+1 counter on each of them.”—require the bounded antecedent set, not an attack-mode special case. Move theParentTargetexclusion into the sharedoracle_effect/mod.rsmass-classification branch (matching the existing imperative authority), remove the trigger-specific demotion walker, and retain the production runtime regression plus a building-block parser regression for the shared branch.
🟡 Non-blocking
- CodeRabbit’s claim that the Bear receives a counter is refuted by the current production path:
game/triggers.rs:357-418constructs a contextualAttackersDeclaredevent frommatching_you_attack_pairs, containing only attackers satisfying the trigger filter, and that narrowed batch is whatparent_target_refs_from_attack_trigger_contextreads. The resolver extension can therefore reuse the filtered batch safely. - The required
<!-- coverage-parse-diff -->artifact is not yet present for head09ce806aeaf7ed39bc3251ca1d7cdb79f4052526; the card-data check was still running during this review.
Recommendation: request changes at the shared counter-parser seam, keep the filtered-batch resolver and discriminating Vrestin scenario, then re-request review after the current-head parse diff is published.
Parse changes introduced by this PR · 3 card(s), 4 signature(s) (baseline: main
|
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/mod.rs`:
- Around line 15094-15100: Update the comment immediately above the `ast`
assignment to remove the incorrect CR 608.2c citation, or replace it with the
rule governing the bounded-set “each of them” anaphor. Keep the existing
explanation of `ParentTarget` and the `imperative::counter_placement_is_mass`
behavior unchanged.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 16449-16458: Update the assertion around the Effect value e to
borrow it with matches!(&e, ...) instead of moving it, while preserving the
existing pattern and diagnostic message so e remains available for {e:?}.
🪄 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: f8d0284a-fc62-4105-a032-35bdf4c8967c
📒 Files selected for processing (4)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/vrestin_menoptra_leader_5949.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/vrestin_menoptra_leader_5949.rs
|
**Blocked — the current code review findings are resolved, but enqueue is halted by the required contributor-trust check. 🔴 Blocker
✅ Clean
Recommendation: resolve the contributor-trust gate and wait for the current required checks before a fresh approval/enqueue decision. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for addressing the earlier parser and trigger-path concerns. One coverage gap remains before this can merge:
The shared ParentTarget parser change changes the parse result for Heroic Feast and Twisted Riddlekeeper as well as Vrestin Menoptra Leader, but the added runtime scenario exercises only Vrestin's attack-batch branch. The selected-target route is a different resolver path. Please add one end-to-end regression for Heroic Feast or Twisted Riddlekeeper that proves selected targets receive the counter and an unselected sibling does not, then update the PR scope to account for all three parse-diff cards.
7b05b5a to
d4a9d50
Compare
|
Hi, @matthewevans |
Maintainer updateUpdated this branch to current Current GitHub checks have restarted, and |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head maintainer review is clean. The Contributor trust check is an advisory triage signal; required protected-branch checks are green. Approving for the merge queue.
…ach batch (phase-rs#6310) * fix(engine): resolve batched attack 'each of them' counter to the attacker batch * fix * fix --------- Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
3 participants
Closes #5949
Summary
Discord report: Vrestin, Menoptra Leader's second ability ("Whenever you attack with one or more Insects, put a +1/+1 counter on each of them.") triggers but does nothing.
The trigger fires, but the counter effect misparsed. "put a +1/+1 counter on each of them" is a distributive over the declared-attackers batch (CR 508.1
AttackersDeclared) — the same shape as Champions from Beyond's "those creatures get +4/+4". Instead it lowered to the mass formEffect::PutCounterAll { target: TriggeringSource }:on each ...phrasing was promoted to the battlefield-widePutCounterAll, andTriggeringSource(viaresolve_it_pronounagainst the Insect trigger subject).At resolution
PutCounterAlliterates the battlefield callingmatches_target_filter, which returnsfalsefor bothTriggeringSourceandParentTarget(they are resolution-time anaphors, not object predicates), so it matched nothing — "fired but did nothing."Root cause
A batched attack anaphor ("each of them" / "each of those creatures") can never be a mass battlefield filter — it names the attacker batch. The proven representation is the singular
Effect::PutCounter { target: ParentTarget }, which Champions from Beyond'sEffect::Pump { target: ParentTarget }uses:ParentTargeton an untargeted batched-attack trigger resolves to the whole batch viaparent_target_refs_from_attack_trigger_context(readsAttackersDeclared.attacker_ids, already narrowed to the matching Insects by the trigger machinery). Two gaps produced the bug: the parser emitted the wrong effect/target, andEffect::PutCounter's runtime resolver did not routeParentTargetthrough the batch-aware path the wayPumpdoes.Changes
crates/engine/src/parser/oracle_trigger.rs— after the trigger body is lowered, for the attack modes whose event carries theAttackersDeclaredbatch (YouAttack/Attacks), rewritePutCounterAll { target: TriggeringSource | ParentTarget }→PutCounter { target: ParentTarget }(demote_batch_attack_counter_all_to_parent_in_ability, walks the effect/sub/else chain). Only touches the anaphor targets, so a genuine mass placement in an attack trigger ("put a +1/+1 counter on each creature you control" →PutCounterAll { target: Typed }) is left alone.crates/engine/src/game/effects/counters.rs— inresolve_defined_or_targets, resolvePutCounter { target: ParentTarget }(no chosen object target, noChooseOneOfbookkeeping) throughtargeting::parent_target_refs_from_attack_trigger_context, the same batch pathEffect::Pumpuses. Non-attackParentTargetcontexts returnNoneand fall through to the existing event-context resolution, so no other counter behavior changes.crates/engine/src/game/targeting.rs— exposeparent_target_refs_from_attack_trigger_contextaspub(crate)so the counter resolver shares the single authority (previously private toresolved_targets).crates/engine/tests/integration/vrestin_menoptra_leader_5949.rs(+main.rsmod) — new regression: (1) parse test asserts the batched attack trigger lowers toPutCounter { target: ParentTarget }; (2) runtime test attacks with Vrestin + two Insects + a non-Insect and asserts each attacking Insect gains exactly one +1/+1 counter while the non-Insect gains none.Test Plan
cargo fmt --all -- --checkcargo clippy -p engine --tests -- -D warnings(clean)cargo test -p engine --test integration -- vrestin(2 pass)cargo test -p engine --test integration -- champions(Champions from Beyond batch pump still passes)cargo test -p engine --lib -- trigger_one_or_more_comma_and_or_subtypes_combat_damage put_counter attack_it_gets(65 pass — Heroes in a Half Shell and singular-attackeritcounter unaffected)Summary by CodeRabbit
Bug Fixes
Tests
#5949covering oracle parsing and combat-trigger resolution.