Fix Hawkeye, Avenging Archer - #6976
Conversation
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:
📝 WalkthroughWalkthroughThe parser extracts damage-history clauses from dies triggers and maps them to source-specific conditions. Damage records 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: 4 (Complex) | ~45 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 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: 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 <[email protected]>
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
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.
JacobWoodson
changed the title
matthewevans
removed
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head 2dea4bf8c3d7dcaa533e7b431bd84f4346d2b597.
[HIGH] Compile blocker — should_resolve_subability_on_optional_decline has an exhaustive match ability.condition at crates/engine/src/game/effects/mod.rs:3189, but omits AbilityCondition::TriggerEventTargetDamagedBySourceThisTurn. CI run 31300016008 reports E0004 in lint, both Rust test shards, card-data, and WASM. Please add this condition to the ordinary => false arm: it is not an optional-decline branch selector.
[MED] Fixture update — the sole explicit DamageContextSnapshot fixture in crates/engine/tests/integration/flame_spill_excess_damage.rs:524-537 does not initialize the newly required source_incarnation field. Add the appropriate fixture value (None for this synthetic no-live-source snapshot) so the snapshot round-trip test remains complete.
After the compilation fixes, please provide fresh parse-diff evidence for the resulting exact head. The existing 5ce359 parse evidence predates these changes and is stale.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head 0619851271f13cee36949e679226d453d5c49c82.
[HIGH] The sole explicit DamageContextSnapshot fixture in crates/engine/tests/integration/flame_spill_excess_damage.rs:524-537 still omits the newly required source_incarnation field. DamageContextSnapshot now declares that field in crates/engine/src/types/ability.rs:10223-10228; add source_incarnation: None for this synthetic no-live-source snapshot so the serialization round-trip covers the complete schema.
After that fix, please provide a current-head parser/coverage artifact. The available sticky evidence is for 5ce359…, while Card data is still pending for this head.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verified the review hardening on 078cb88: the negative path now proves the SBA death and requires the trigger pipeline to settle. Parser gates and CI are green.
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head f842161f5a6a331032942ec25192f2986be696b0.
[HIGH] damage_record_source_incarnation_matches in crates/engine/src/game/triggers.rs:10759-10769 bypasses identity comparison whenever source_context.identity.expected_zone != Zone::Battlefield. That lets an old damage record match a later re-entry incarnation of the same object ID, contrary to CR 400.7. TriggerSourceContext is explicitly the pre-reset event-time projection (crates/engine/src/types/game_state.rs:441-452), and source_read correctly requires both its ObjectIncarnationRef and expected zone (:648-665); this helper must honor that same authority instead of treating every off-battlefield context as a match.
Compare against the pre-zone-change ObjectIncarnationRef and add a negative regression for a source that deals damage, changes zones/re-enters, then co-dies or otherwise reaches the off-battlefield trigger context. The old record must not satisfy the new incarnation's condition.
Please also regenerate parse/coverage evidence for the resulting exact head; the current artifact is stale (9e…).
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/game/ability_rw.rs`:
- Around line 6005-6007: Update the annotation above
AbilityCondition::TriggerEventTargetDamagedBySourceThisTurn to remove the
incorrect CR 603.3b citation and replace it with a CR rule that directly
describes frozen event/source identity semantics, or state the behavior as an
engine invariant without a CR number. Keep the wording and citation consistent
with the canonical AbilityCondition documentation in ability.rs.
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 3254-3256: Add verified “CR <number>: <description>” annotations
at both affected sites in crates/engine/src/game/effects/mod.rs: lines
3254-3256, documenting why the intervening trigger condition cannot select an
optional-effect decline branch; and lines 11514-11542, documenting the enforced
damage-history and intervening-condition behavior. Use authoritative,
behavior-specific Comprehensive Rules references and place each annotation with
the corresponding rules logic.
In `@crates/engine/src/parser/oracle_effect/conditions.rs`:
- Around line 3184-3188: Replace the full-clause all_consuming(tag(...)) branch
in the trigger-condition parser with a composable nom parser built from the
existing source, damage-action, event-target, and turn-qualifier parsers. Keep
all_consuming at the caller level and preserve the ctx.in_trigger guard, so
equivalent grammar variants are parsed through the typed condition components
rather than matched as one verbatim string.
🪄 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: 508a88d6-60dc-4e4a-87ef-0cd0b02d8ddc
📒 Files selected for processing (17)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/deal_damage.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/parser/swallow_check.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/flame_spill_excess_damage.rscrates/engine/tests/integration/hawkeye_avenging_archer_dealt_damage_draw.rscrates/engine/tests/integration/main.rscrates/phase-ai/src/policies/tests/removal_lethality.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_trigger.rs
- crates/engine/src/parser/oracle_trigger_tests.rs
- crates/engine/src/game/triggers.rs
- crates/engine/src/parser/swallow_check.rs
- crates/engine/src/game/effects/deal_damage.rs
- crates/engine/src/types/game_state.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-reviewed current head f842161: Hawkeye source-incarnation handling preserves simultaneous-death LKI and rejects live re-entry; refreshed CI is green.
|
Maintainer conflict hold — current head GitHub reports this branch The existing green checks and approval do not clear the pending source-incarnation re-entry concern: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested for head 9c8565ddae1304e1b56b05ba2c3a51f9262c318a.
[HIGH] The prior source-identity defect remains. damage_record_source_incarnation_matches (crates/engine/src/game/triggers.rs:10885-10894) accepts any recorded source incarnation whenever expected_zone != Battlefield; the Hawkeye trigger consumes that result at :10049-10066. An old damage record can therefore satisfy a later re-entry incarnation of the same object ID, violating CR 400.7.
TriggerSourceContext is the exact pre-reset source projection (crates/engine/src/types/game_state.rs:441-452) and source_read correctly demands both its ObjectIncarnationRef and expected zone (:648-665). Compare the damage record against that pre-move ObjectIncarnationRef regardless of the current/expected zone. Add a discriminating negative re-entry/co-death regression: damage by the old incarnation must not satisfy the later source incarnation's trigger.
Please regenerate parse/coverage evidence for the resulting exact head; the available artifact is for f842….
|
Maintainer hold for current head The source-identity fix is now present and resolves the prior blocker. The current parse-diff artifact remains sticky on the earlier baseline/head, so it is not approval evidence for this revision. Rust/AI/performance gates are also still running. Next step: once those required checks complete successfully, please refresh the parse-diff artifact for this exact head and current |
|
Maintainer hold — current head Fresh maintainer review finds the implementation clean. Approval remains gated only on external evidence: Rust lint and both test shards, Paired-seed AI, and the Decision-cost performance gate are still in progress; the available Next step: let those required checks complete successfully and publish/refresh the parse-diff artifact for this exact head against current |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off: exact source-incarnation checks now preserve simultaneous-death LKI while rejecting stale re-entries; the trailing damage rider uses composable shared nom grammar.
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