Partial: Hawkeye, Avenging Archer - #6976
Conversation
github-actions
Bot
added
the
needs-maintainer
📝 WalkthroughWalkthroughThe parser extracts damage-history clauses from dies triggers and maps them to source-specific conditions. Damage records now retain optional source incarnations. Trigger evaluation matches the source, incarnation, and dying creature. Parser and Hawkeye tests cover positive and negative cases. ChangesDies-trigger damage history
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant GameState
participant TriggerParser
participant TriggerEvaluator
participant Stack
GameState->>TriggerParser: Parse Hawkeye dies trigger
TriggerParser->>TriggerEvaluator: Return damage-history condition
GameState->>TriggerEvaluator: Provide damage records and dying creature
TriggerEvaluator->>Stack: Add draw trigger when source and creature match
Stack->>GameState: Resolve card draw
Possibly related PRs
Suggested reviewers: 🚥 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: 3
🤖 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 5576-5585: Restrict the condition-hoisting branch in the trigger
parsing flow around scan_preceded and parse_dealt_damage_to_it_intervening_if to
cases where before contains no semantic effect text, ensuring only a leading
intervening-if clause is hoisted. Keep trailing forms such as “draw a card if …”
as resolution-time conditions, and add a regression test covering that form
while preserving CR 603.4 behavior.
In `@crates/engine/src/parser/swallow_check.rs`:
- Around line 4861-4872: Strengthen the swallow-detector test around the
assertions for “Condition_If” and “Duration_ThisTurn” by first asserting that
the parsed trigger contains the expected Hawkeye condition
“DealtDamageBySourceThisTurn” and that the tested trigger body reaches an
implemented parse path. Keep both existing negative diagnostic assertions,
pairing them with these positive reach guards so the test cannot pass by
bypassing the detector.
In
`@crates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rs`:
- Around line 46-56: Replace the direct damage mutation and manual SBA/trigger
calls in `kill_via_sba` and the `DamageRecord` insertion around the affected
test setup with a `GameAction` that activates Hawkeye’s `{T}` ability targeting
the victim. Let the normal ability resolution, damage handling, SBA checks,
trigger processing, and stack draining run through `GameRunner`’s production
pipeline, while preserving the test’s lethal-damage outcome.
🪄 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: ba136de1-7f2a-4480-9f55-260d1df07820
📒 Files selected for processing (6)
crates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/parser/swallow_check.rscrates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rscrates/engine/tests/integration/main.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head 4d1c01cb2019535910fa17214e4e436781e1d5a9 has four blocking correctness/test gaps.
[HIGH] Resolution-time trailing if clauses are promoted to an intervening-if. Evidence: crates/engine/src/parser/oracle_trigger.rs:5576-5585 uses scan_preceded over the entire dies trigger, so it accepts the matching damage clause wherever it appears. Why it matters: only the leading trigger condition is an intervening-if; a trailing resolution-time if must remain in the effect chain and be evaluated on resolution. Suggested fix: accept this grammar only in the leading condition position and add a trailing-if regression that proves it stays resolution-time.
[HIGH] The damage/death match can cross object incarnations. Evidence: crates/engine/src/game/triggers.rs:9524-9542 identifies both source and dying subject by ObjectId; DamageRecord at crates/engine/src/types/game_state.rs:1854-1903 has no source-incarnation identity and only an optional target incarnation. Why it matters: after a zone change/re-entry reuses an ID, historical damage can spuriously satisfy Hawkeye's condition and draw a card for a distinct object incarnation. Suggested fix: snapshot and compare the relevant source and target incarnations at damage time, then add a post-zone-change negative regression.
[HIGH] The runtime test bypasses the claimed activated-damage pipeline. Evidence: crates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rs:72-85 manually inserts a DamageRecord; it never activates Hawkeye, selects a target, or resolves the damage effect. Why it matters: the test cannot prove the new condition works with the real damage-recording path. Suggested fix: drive Hawkeye's actual activated ability through target selection and resolution, then kill that target and assert the draw.
[HIGH] The swallow-check coverage test proves only silence, not the typed semantic carrier. Evidence: the Condition_If checks assert absence of the diagnostic without asserting the parsed trigger condition/effect shape. Why it matters: a broad suppression or an unrelated carrier can make the diagnostic disappear while the Hawkeye condition is still absent or misclassified. Suggested fix: assert the parsed trigger has DealtDamageBySourceThisTurn and the draw effect, with a paired trailing-resolution-time condition case.
|
Generated for head Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head 562c5f457af6048c96662c2cc69b3cd6267e25c5 is a merge-from-main; the reviewed Hawkeye surfaces are unchanged, so these four blockers remain.
[HIGH] oracle_trigger.rs:5576-5585 scans the whole trigger and hoists a trailing resolution-time if as an intervening-if. Restrict this grammar to the leading trigger condition and add a trailing-if regression that remains resolution-time.
[HIGH] triggers.rs:9526-9541 matches source and dying object by ObjectId; DamageRecord (game_state.rs:1854-1864) has no source-incarnation snapshot. Zone-change/re-entry can therefore match historical damage to a different incarnation. Snapshot and compare source/target incarnations, with a post-zone-change negative test.
[HIGH] The integration test manually creates the decisive record: hawkeye_avenging_archer_dealt_damage_draw.rs:44-56 kills through SBA and :72-85 injects DamageRecord. It never activates Hawkeye, targets, or resolves the real damage effect. Drive that actual activation pipeline before asserting the draw.
[HIGH] swallow_check.rs:4853-4872 proves only that diagnostics disappeared. Assert the typed DealtDamageBySourceThisTurn trigger condition and draw effect, plus a paired trailing-resolution-time condition case, so suppression cannot make the test pass.
…Hawkeye dies-trigger Addresses the four review blockers on phase-rs#6976 (Hawkeye, Avenging Archer): - CR 400.7: DamageRecord now snapshots source_incarnation, and the DealtDamageBySourceThisTurn match compares it (new damage_record_source_incarnation_matches) so a re-entered source -- same ObjectId, bumped incarnation, a new object per CR 400.7 -- no longer inherits a prior incarnation's damage. Captured in the production deal-damage resolver alongside target_incarnation. Adds a post-zone-change regression. - CR 603.4: restrict the "if ~ dealt damage to it this turn" hoist to the leading effect position (before blank); a trailing resolution-time if ("draw a card if ~ dealt damage to it this turn") stays in the effect chain rather than being promoted to an intervening-if. Adds a trailing-if regression. - The Hawkeye integration test now drives the real {T} activation through the production pipeline (activate -> target -> deal damage -> SBA death -> dies trigger -> draw) instead of hand-injecting a DamageRecord. - The swallow-check test asserts the typed DealtDamageBySourceThisTurn condition and Draw effect before the negative diagnostic checks, and adds a paired trailing-resolution-time case. Verification: cargo fmt; phase-engine lib + integration Hawkeye tests green; clippy -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
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/game/effects/deal_damage.rs`:
- Around line 737-749: The damage record currently derives source_incarnation
from the post-application live object, losing the original identity when the
source leaves or re-enters. Update DamageContext creation to capture the source
incarnation, carry it through DamageContextSnapshot, and have the DamageRecord
construction reuse that value, falling back to the LKI snapshot when no live
source exists. Add a regression test for a source leaving or re-entering before
the dies-trigger exact-source lookup.
In
`@crates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rs`:
- Around line 125-139: The drain_stack helper must fail instead of silently
accepting incomplete processing: continue only until WaitingFor::Priority with
an empty stack, panic or propagate any PassPriority error, and panic when the
200-iteration limit is reached. Add a reach guard confirming the death entered
trigger processing before asserting the negative hand result, using the existing
drain_stack and test state symbols.
🪄 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: d0badc8d-fa08-4fca-832d-c6612e304509
📒 Files selected for processing (7)
crates/engine/src/game/effects/deal_damage.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/parser/swallow_check.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/parser/oracle_trigger.rs
- crates/engine/src/parser/swallow_check.rs
- crates/engine/src/parser/oracle_trigger_tests.rs
| // CR 400.7: Snapshot the source's incarnation at damage time so an | ||
| // exact-source look-back (`DealtDamageBySourceThisTurn`) does not credit | ||
| // a re-entered permanent (same ObjectId, bumped incarnation — a new | ||
| // object) with damage its prior incarnation dealt. `None` when the | ||
| // source is already gone (CR 113.7a): no live incarnation to snapshot. | ||
| let source_incarnation = src.map(|object| object.incarnation); | ||
| let mut record = DamageRecord { | ||
| source_id: ctx.source_id, | ||
| source_controller: ctx.controller, | ||
| target: t.clone(), | ||
| target_controller, | ||
| target_incarnation, | ||
| source_incarnation, |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve the source incarnation from the damage event, including LKI.
source_incarnation is derived from the current state.objects entry after damage application. If the source is absent, this stores None. If the source changed zones before this record was written, this can store a later incarnation. That breaks exact-source lookbacks such as DealtDamageBySourceThisTurn.
CR 113.7a allows a resolved ability to make a removed source perform the action using last known information, so it does not justify discarding the source incarnation. CR 400.7 also treats a re-entered object as a new object. (media.wizards.com)
Capture the incarnation when DamageContext is created. Preserve it through DamageContextSnapshot. Fall back to the LKI snapshot when the live source is absent. Add a regression test covering a source that leaves or re-enters before the dies-trigger lookup.
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 8 \
'struct DamageContext|struct DamageContextSnapshot|struct DamageRecord|source_incarnation|lki_cache|DamageContext::from_source' \
crates/engine/src crates/engine/testsAs per path instructions, crates/engine/** must preserve Comprehensive Rules behavior and use verified CR annotations.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/engine/src/game/effects/deal_damage.rs` around lines 737 - 749, The
damage record currently derives source_incarnation from the post-application
live object, losing the original identity when the source leaves or re-enters.
Update DamageContext creation to capture the source incarnation, carry it
through DamageContextSnapshot, and have the DamageRecord construction reuse that
value, falling back to the LKI snapshot when no live source exists. Add a
regression test for a source leaving or re-entering before the dies-trigger
exact-source lookup.
| fn drain_stack(runner: &mut GameRunner) { | ||
| for _ in 0..200 { | ||
| if matches!(runner.state().waiting_for, WaitingFor::OrderTriggers { .. }) { | ||
| engine::game::triggers::drain_order_triggers_with_identity(runner.state_mut()); | ||
| continue; | ||
| } | ||
| match &runner.state().waiting_for { | ||
| WaitingFor::Priority { .. } if runner.state().stack.is_empty() => break, | ||
| _ => { | ||
| if runner.act(GameAction::PassPriority).is_err() { | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Fail when stack processing does not complete.
Line 134 discards PassPriority errors. Line 126 also exits after 200 iterations without an assertion. The negative test can then pass with an unchanged hand because the trigger pipeline did not complete.
Return only after WaitingFor::Priority and an empty stack. Panic or return an error on PassPriority failure and when the iteration limit expires. Add a reach guard that proves this death reached trigger processing before the negative hand assertion.
🤖 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/hawkeye_avenging_archer_dealt_damage_draw.rs`
around lines 125 - 139, The drain_stack helper must fail instead of silently
accepting incomplete processing: continue only until WaitingFor::Priority with
an empty stack, panic or propagate any PassPriority error, and panic when the
200-iteration limit is reached. Add a reach guard confirming the death entered
trigger processing before asserting the negative hand result, using the existing
drain_stack and test state symbols.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head still has two rules/architecture blockers and one non-discriminating regression test.
🔴 Blocker
crates/engine/src/parser/oracle_trigger.rs:5634-5648 leaves a trailing if in the trigger remainder, while crates/engine/src/parser/oracle_effect/conditions.rs:3223-3273 cannot represent the exact dealt-damage grammar that only oracle_trigger.rs:5757-5766 recognizes. The parser test at crates/engine/src/parser/oracle_trigger_tests.rs:18593-18618 therefore accepts an unconditioned Draw, which is false-green rather than proof of the conditional trigger.
CR 603.4 says: “this rule only applies to an ‘if’ that immediately follows a trigger condition.” The remaining if here is an effect condition, so it needs event-bound, resolution-time representation rather than the intervening-if path. Please thread a condition keyed to the trigger event/source through resolution, or retain an honest Effect::unimplemented; add runtime positive and negative tests that prove the condition is evaluated at resolution.
🔴 Blocker
The trigger path reads the live source (crates/engine/src/game/triggers.rs:9215, 9535-9542, 10362-10375) without an epoch/LKI captured in TriggerSourceContext; damage also records no source snapshot when the source is absent. A source that leaves and re-enters can therefore be treated as the original source. Thread a source epoch through the DamageContext snapshot and add production-pipeline regressions for source-gone and re-entry cases.
🟡 Non-blocking
crates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rs:125-139 swallows PassPriority errors/exhaustion before asserting only hand state. Tighten the helper to assert terminal priority/empty stack and add reach guards, so the test fails if the trigger never resolves.
The parse-diff sticky evidence is stale (it identifies head 562c…, not this 5ce359f…), so current engine/parser scope evidence is still missing.
Recommendation: request changes. Please implement the event-bound condition and LKI snapshot paths with discriminating runtime coverage, then refresh the parse-diff evidence for the resulting head.
2 participants
Summary
Fixes a parse-fidelity defect on Hawkeye, Avenging Archer.
Issue: audit-flagged: DroppedCondition — a condition on one of its abilities is silently dropped from the parsed AST; find the dropped intervening-if/condition and restore it.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— pass (exit 0)./scripts/check-parser-combinators.sh (Gate A)— pass (Gate G PASS + Gate A PASS; Family-D self-test and A-F scan both ran and passed via msys2 python3 after masking the WindowsApps python3 stub)cargo clippy-strict— pass (exit 0, -D warnings, zero warnings)cargo test -p phase-engine— fail (exit 101): 18468/18469 pass; the sole failure is a pre-existing, unrelated Windows path-separator artifact in an untouched census test; all 4 Hawkeye-specific tests pass (2 lib + 2 integration)cargo export-cards data --stats --sidecar-dir client/public --output client/public/card-data.json && cp client/public/card-data.json data/card-data.json— pass (exit 0; added --output because the given recipe's --sidecar-dir does not write the main card-data.json and the cp would otherwise copy a stale file; both files fresh, Hawkeye key present)cargo coverage— pass (exit 0; Hawkeye, Avenging Archer supported=true gap_count=0)cargo semantic-audit— pass (exit 0; Hawkeye, Avenging Archer has 0 findings)Scope Expansion
None. Misparse reproduced; fixed the class-level parser gap (dies-trigger "if dealt damage to it this turn" intervening-if) plus tests, no card-name special-casing.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit