Partial: Add Delayed/created "deals combat damage to a player/opponent" trigger (CreateDelayedTrigger + WheneverEvent) with tracked-set/self source binding mechanic - #6884
Conversation
…t" trigger (CreateDelayedTrigger + WheneverEvent) with tracked-set/self source binding mechanic
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:
📝 WalkthroughWalkthroughDelayed ChangesDelayed trigger parsing and contracts
Delayed trigger execution
Estimated code review effort: 4 (Complex) | ~60 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
|
Generated for head Parse changes introduced by this PR · 14 card(s), 4 signature(s) (baseline: main
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/delayed_trigger.rs (1)
209-213: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winGate origin stamping to one-shot delayed triggers.
WheneverEventresolvesTriggeringSourcefrom each firing event, but this call fixesChangeZone.originto the creation event’s destination zone. A later firing from another zone can therefore skip the zone move. Add the sameone_shot_refgate and test different creation and firing zones.🤖 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/delayed_trigger.rs` around lines 209 - 213, Update the origin-stamping block around ability_refs_triggering_source and stamp_triggering_source_origins_in_ability_chain so it only runs when the delayed trigger has a one_shot_ref. Preserve the existing triggering-source and destination-zone checks for one-shot triggers, and add coverage where the creation and firing events occur in different zones to ensure the firing zone is used.
🧹 Nitpick comments (2)
crates/engine/src/game/effects/delayed_trigger.rs (1)
178-181: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
matches!computation forone_shot_refandone_shot.
one_shot_ref(Line 178-181) andone_shot(Line 278-281) compute the identical expression:!matches!( condition, crate::types::ability::DelayedTriggerCondition::WheneverEvent { .. } )
condition's variant is not reassigned between these two points. Compute the boolean once and reuse it for both the snapshot branch and theDelayedTrigger.one_shotfield, so future edits to one site cannot silently diverge from the other.♻️ Proposed consolidation
- let one_shot_ref = !matches!( - condition, - crate::types::ability::DelayedTriggerCondition::WheneverEvent { .. } - ); + let one_shot = !matches!( + condition, + crate::types::ability::DelayedTriggerCondition::WheneverEvent { .. } + ); let snapshot_targets = - if one_shot_ref && super::ability_refs_triggering_source(&delayed_ability) { + if one_shot && super::ability_refs_triggering_source(&delayed_ability) {and remove the later redundant
let one_shot = ...at Line 278-281, reusing the single binding atinstall_delayed_trigger.Also applies to: 278-281
🤖 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/delayed_trigger.rs` around lines 178 - 181, Compute the delayed-trigger one-shot boolean once in install_delayed_trigger, retaining the existing one_shot_ref binding near the snapshot logic, and remove the later redundant one_shot computation before constructing DelayedTrigger. Reuse that single binding for both the snapshot branch and the DelayedTrigger.one_shot field.crates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rs (1)
198-205: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a firing assertion on the intervening turn.
The test proves the rider is still present in
state.delayed_triggersafter the creating turn's cleanup. Presence does not prove the rider can still fire. The module doc states the load-bearing behavior is that the rider fires on the opponent's turn. A regression that keeps the entry but makes the firing path reject a non-EndOfTurnexpiry would still pass this test.Drive an unblocked attacker on the opponent's turn and assert the controller drew a card, or assert the trigger reached the stack.
As per path instructions: "A test must exercise the FAILURE path the fix prevents and drive the engine through its production pipeline".
🤖 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/kang_dynasty_until_next_turn_rider.rs` around lines 198 - 205, Extend the test after advance_past_turn in the intervening-turn scenario to drive an unblocked attacker through the normal combat/trigger pipeline, then assert the rider fired by verifying the controller drew a card or the trigger reached the stack. Keep the existing delayed_triggers persistence assertion, and ensure the attack exercises the failure path where a non-EndOfTurn expiry could otherwise be rejected.Source: Path instructions
🤖 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/delayed_trigger.rs`:
- Around line 67-90: The empty-parent-target guard in the delayed-trigger
handling must inspect all three WheneverEvent filters: valid_source, valid_card,
and valid_target. Update the condition around
DelayedTriggerCondition::WheneverEvent to skip installation when any field is
Some(TargetFilter::ParentTarget) and ability.targets is empty, matching
bind_contextual_filter_to_condition’s handling. Add regression coverage
alongside whenever_event_empty_parent_target_set_skips_install for ParentTarget
in valid_card and valid_target.
In `@crates/engine/src/game/triggers.rs`:
- Around line 8066-8123: Update consumed-event recording in
engine_priority::run_post_action_pipeline so expanded damage occurrences store
events[event_index].clone()—the originating raw CombatDamageDealtToPlayer
event—instead of the synthetic DamageDealt event. Keep the existing matcher
behavior, including eligibility for DamageKindFilter::Any and exclusion of
non-DamageDone/SelfRef triggers.
In `@crates/engine/src/parser/oracle_ir/context.rs`:
- Around line 47-55: Replace the boolean field in the parser context with a
typed trigger-condition scope enum, such as TriggerConditionScope::{Printed,
Delayed}. Update every context construction, comparison, mutation, and
restoration site—including try_parse_whenever_this_turn—to use the enum variants
while preserving the existing delayed-only recognition behavior.
In `@crates/engine/src/parser/oracle_tests.rs`:
- Line 10542: Update the three delayed-trigger tests in
crates/engine/src/parser/oracle_tests.rs at lines 10542-10542, 10955-10955, and
10980-10980 to retain and assert the WheneverEvent expiry fields instead of
discarding them. Verify expiry metadata for the spell-cast, phase-based, and
zone-change triggers while preserving their existing nested-trigger assertions.
In `@crates/engine/tests/integration/human_torch_combat_damage_redirect.rs`:
- Around line 147-154: Replace the standalone negative assertion in the
delayed-trigger test with a positive assertion that pins the expected parsed
trigger shape while separately verifying it is not SelfRef. Ensure the assertion
requires valid_source to be Some with the expected non-SelfRef target, so an
upstream parse failure producing None cannot satisfy the test.
In `@crates/engine/tests/integration/love_on_the_battlefield_combat_counters.rs`:
- Around line 267-281: Add a positive reach-guard to the three-attacker test
around run_combat, asserting the defending player’s life decreased by the
expected combat damage before the existing no-draw, no-counter, and
no-first-strike assertions. Reuse the test’s established player-life
state/accessor symbols, ensuring the guard proves the attack resolved and the
Comparator::EQ constraint was exercised.
---
Outside diff comments:
In `@crates/engine/src/game/effects/delayed_trigger.rs`:
- Around line 209-213: Update the origin-stamping block around
ability_refs_triggering_source and
stamp_triggering_source_origins_in_ability_chain so it only runs when the
delayed trigger has a one_shot_ref. Preserve the existing triggering-source and
destination-zone checks for one-shot triggers, and add coverage where the
creation and firing events occur in different zones to ensure the firing zone is
used.
---
Nitpick comments:
In `@crates/engine/src/game/effects/delayed_trigger.rs`:
- Around line 178-181: Compute the delayed-trigger one-shot boolean once in
install_delayed_trigger, retaining the existing one_shot_ref binding near the
snapshot logic, and remove the later redundant one_shot computation before
constructing DelayedTrigger. Reuse that single binding for both the snapshot
branch and the DelayedTrigger.one_shot field.
In `@crates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rs`:
- Around line 198-205: Extend the test after advance_past_turn in the
intervening-turn scenario to drive an unblocked attacker through the normal
combat/trigger pipeline, then assert the rider fired by verifying the controller
drew a card or the trigger reached the stack. Keep the existing delayed_triggers
persistence assertion, and ensure the attack exercises the failure path where a
non-EndOfTurn expiry could otherwise be rejected.
🪄 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: b75aa22d-9834-4180-8972-795681e44fc8
📒 Files selected for processing (23)
UsersjacobAppDataLocalTempclaudeC--Users-jacob-source-repos-phase-card-runs!a248f4-b4f3-4c24-a7a6-d1f9f7693f55scratchpadantman.jsoncrates/engine/src/game/coverage.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/triggers.rscrates/engine/src/game/turns.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/parser/swallow_check.rscrates/engine/src/parser/swallow_evidence.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/human_torch_combat_damage_redirect.rscrates/engine/tests/integration/hunters_insight_combat_draw.rscrates/engine/tests/integration/issue_3294_good_king_mog_chapter_ii.rscrates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rscrates/engine/tests/integration/love_on_the_battlefield_combat_counters.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/stensian_sanguinist_prepare.rscrates/mtgish-import/src/convert/action.rs
|
Maintainer hold for current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the delayed-trigger class is not yet safe to merge: two runtime paths can over-fire, and the measured parser expansion is not reconciled with the claimed scope.
🔴 Blocker
crates/engine/src/game/triggers.rs:8086-8120 expands one aggregate CombatDamageDealtToPlayer into synthetic DamageDealt events, but :8277-8285 records that synthetic event with the aggregate occurrence index. filter_consumed_trigger_events_from at :8037-8055 later compares both event equality and occurrence, so the raw aggregate remains available to a subsequent priority scan and can fire the same delayed trigger twice. Record events[event_index].clone() as the consumed identity; retain the synthetic event only as trigger context, and add a production regression that advances priority/resolution and proves one firing per source.
crates/engine/src/game/effects/delayed_trigger.rs:67-91 suppresses an empty ParentTarget only for valid_source, while bind_contextual_filter_to_condition at :490-500 rewrites valid_card, valid_source, and valid_target. An up-to-N parent selecting zero can therefore turn an unbound valid-card/valid-target into Any and install an over-firing delayed trigger. Guard all three slots and add sibling fixtures.
🔴 Required scope evidence
The current head’s full parse artifact measures 14 cards/four signatures, not the PR’s stated three unlocked cards: eight duration removals, four duration changes, and two attacker-filter changes. Classify every affected card/signature and add runtime coverage or retain strict failure for unsupported forms before this parser expansion can land.
🟡 Test evidence
Strengthen the new negative tests with a positive branch/reach assertion: human_torch_combat_damage_redirect.rs:147-154 only proves non-SelfRef, and love_on_the_battlefield_combat_counters.rs:267-280 uses only negative outcomes.
Recommendation: request changes; resubmit with the two runtime corrections, reconciled parser blast radius, and discriminating positive reach guards.
…hase-rs#6884) Resolve the two runtime over-fire paths and test-evidence gaps raised in review of the delayed "deals combat damage" WheneverEvent mechanic. Blocker 1 — double-fire (triggers.rs): collect_matching_delayed_triggers recorded the synthetic per-source DamageDealt event as the consumed identity while computing its occurrence from the raw aggregate CombatDamageDealtToPlayer at event_index. Because filter_consumed_trigger_events_from compares both event equality and occurrence, the aggregate never matched the consumed set and survived into a later priority scan, re-firing the trigger. Record events[event_index] (the raw aggregate) as the consumed identity and keep the synthetic event as per-firing trigger context only. New building-block regression multi_fire_combat_damage_consumes_raw_aggregate_not_synthetic fails on the old code and passes on the fix. Blocker 2 — empty ParentTarget over-fire (delayed_trigger.rs): bind_contextual_filter_to_condition rewrites all three WheneverEvent filter slots, so an "up to N target" parent that chose zero could turn a bare ParentTarget in valid_card/valid_target (not just valid_source) into TargetFilter::Any and install an over-firing trigger. The install guard now inspects all three slots; added sibling fixtures for valid_card and valid_target. CodeRabbit maintainability + test evidence: - Replace the in_delayed_trigger bool with a typed TriggerConditionScope enum (Printed/Delayed) per the codebase's typed-enum convention. - Compute the delayed-trigger one_shot boolean once and reuse it; gate the TriggeringSource origin-stamp to one-shot triggers (a multi-fire WheneverEvent re-resolves TriggeringSource per firing, so freezing the creation-event zone would skip a later firing's zone move). - Assert the WheneverEventExpiry fields in the three spell-temporal parser tests instead of discarding them. - Strengthen negative tests with positive reach guards: human_torch now pins the coverage-honest Unknown parse shape (the possessive subject is not a recognized anaphor) and love_on_the_battlefield's three-attacker test asserts the defending player's life dropped (attack actually resolved). Verification: cargo fmt; cargo clippy -p phase-engine --all-targets (clean); cargo test -p phase-engine --lib (18272 passed); combat/damage/trigger/delayed integration sweep (798 passed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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_ir/context.rs`:
- Around line 19-37: Correct the CR citations in the TriggerConditionScope
documentation and the related annotation around try_parse_whenever_this_turn:
replace the inaccurate CR 603.7c and CR 201.5 references with a verified
citation describing delayed-trigger creation, such as CR 603.7a. Keep the
existing scope and pronoun-resolution explanation unchanged.
🪄 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: d20e1169-079c-4b20-99ad-d40c8b60fd2a
📒 Files selected for processing (8)
crates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_ir/context.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/tests/integration/human_torch_combat_damage_redirect.rscrates/engine/tests/integration/love_on_the_battlefield_combat_counters.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/engine/tests/integration/love_on_the_battlefield_combat_counters.rs
- crates/engine/src/parser/oracle_effect/mod.rs
- crates/engine/src/parser/oracle_trigger.rs
- crates/engine/src/game/effects/delayed_trigger.rs
…n relocation (phase-rs#6884) Reconcile the parse blast radius the maintainer flagged (14 cards / 4 signatures) with permanent class-level regression coverage rather than one-off card tests. Signatures 1-2 (12 cards, "until your next turn, whenever ..." delayed triggers): the CreateDelayedTrigger duration field change is the intended Gap-C fix. "until your next turn" relocates from the (previously misattached) creator-ability duration to the WheneverEvent expiry (UntilControllersNextTurn). This is a correctness win: before, these triggers had no expiry, defaulted to EndOfTurn, and were purged at the creating turn's cleanup, so they never fired on opponents' turns (CR 603.7b). Sig 1 (plain inner effect) leaves the creator ability with no residual duration; Sig 2 (inner "... until end of turn" buff, e.g. Jace/Tamiyo) surfaces the residual UntilEndOfTurn on the creator ability. until_next_turn_delayed_trigger_relocates_duration_to_expiry_across_class pins both. None of these 12 cards is claimed as newly-supported; they remain coverage-red multi-ability cards (strict failure retained). Signatures 3-4 (Alluring Suitor + Love on the Battlefield): the "attack with exactly two creatures" recognition (AttackersDeclaredCount EQ 2 + you target) is a class fix. Love has full runtime coverage; added parse_shape_alluring_suitor_exactly_two_attack_constraint for the sibling so the class cannot regress independently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for the thorough review, @matthewevans. Both runtime over-fire paths, the test-evidence asks, and the CodeRabbit items are addressed in 🔴 Blocker 1 — double-fire (
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/kang_dynasty_until_next_turn_rider.rs`:
- Around line 103-167: Add production-path integration coverage for both
duration signatures in
until_next_turn_delayed_trigger_relocates_duration_to_expiry_across_class:
resolve each parsed ability through the engine, create and trigger it during an
opponent’s turn, and verify it fires before the controller’s next turn. Advance
to the controller’s next turn, then verify the delayed trigger is removed or no
longer fires after expiry, covering both the plain inner effect and the inner
“until end of turn” buff.
🪄 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: 80e37768-c0f0-48d8-9420-186631b8551f
📒 Files selected for processing (2)
crates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rscrates/engine/tests/integration/love_on_the_battlefield_combat_counters.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/love_on_the_battlefield_combat_counters.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head c7d5b6a removes the accidental scratch artifact, but the 12-card until-next-turn duration class still lacks production-path evidence.
🔴 Blocker
[HIGH] The duration-class regression is parse-only, and the only lifecycle test bypasses the parser-derived ability and never fires a trigger on the intervening turn. Evidence: crates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rs:103-167 asserts two parsed AST shapes; :215-249 directly constructs WheneverEventExpiry::UntilControllersNextTurn; :251-301 asserts only retention and later purge. Why it matters: a defect in the parser-to-resolution-to-trigger pipeline can leave all 12 duration-relocation cards unable to fire on the opponent’s turn while these tests remain green. Suggested fix: for one representative of each measured duration signature, parse the real Oracle text, resolve the parsed ability through the normal engine pipeline, cause the matching event during an intervening turn, assert the effect fires, then advance to the controller’s next turn and assert expiry.
🟡 Required follow-up
[MED] The current CodeRabbit citation finding is still present in the unchanged source. Evidence: crates/engine/src/parser/oracle_ir/context.rs:19-27,66-69 still labels parser-scope documentation with CR 603.7c + CR 201.5; the current review thread requests citation correction. Why it matters: engine-rule annotations are required to be source-verified. Suggested fix: verify the applicable rule text in docs/MagicCompRules.txt, then correct the comments or remove any citation that does not directly describe this parser-only scope.
🟡 Evidence status
The new head only removes a non-engine scratch JSON file (c7d5b6a versus 554aee7), but the parse-diff sticky currently names the parent head. CI must publish a current-head artifact before approval; no parser scope is being attributed from the stale artifact.
Recommendation: add the two production-path duration regressions, correct the source-verified CR annotations, and resubmit.
phase-rs#6884) The scope-evidence class-guard test's doc comment had a bulleted list immediately followed by a paragraph; add the blank /// separator so clippy::doc_lazy_continuation (workspace -D warnings) is satisfied. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head dd07cbafbd4fbc2a43e702171b1c66e692bcd06f only.
[MED] The duration regression test does not exercise the delayed trigger firing path. Evidence: crates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rs:216-250 constructs a SelfRef trigger on the Kang enchantment, and :258-301 only advances turns and checks delayed_triggers.len(); it never causes combat damage or observes the draw. Why it matters: this can pass while a surviving UntilControllersNextTurn trigger never matches/fires on the opponent turn—the behavioral claim the PR makes—and it does not cover Kang’s actual ParentTarget source binding. Suggested fix: resolve a ParentTarget rider with a real chosen creature, drive that creature through unblocked combat on the intervening turn, assert the controller draws, then advance to the controller’s next turn and prove it is expired.
[LOW] The new parser-scope CR annotations do not describe their code. Evidence: crates/engine/src/parser/oracle_ir/context.rs:19-27,66-69 cites CR 603.7c and CR 201.5; the verified rules respectively govern delayed abilities retaining reference to an expected-zone object and name-based self-reference, not parser scope or pronoun classification. Why it matters: project rule annotations are intended to be auditable and these create false provenance. Suggested fix: remove the parser-only citations or replace the delayed-trigger-creation portion with the verified CR 603.7a; retain CR 201.5 only immediately above code that implements a name-based self-reference.
…hase-rs#6884) [MED] The "until your next turn" duration test only proved persistence (delayed_triggers.len) and never exercised firing. Added until_next_turn_parent_target_rider_fires_on_intervening_turn_then_expires: installs a ParentTarget rider bound to a real chosen creature (Kang's "any of those creatures"), drives that creature through unblocked combat against the controller on the intervening (opponent's) turn via the production pipeline, asserts the controller drew and took the combat damage, then crosses into the controller's next turn and asserts the rider was purged at untap. A revert of the UntilControllersNextTurn expiry purges the rider at the creating turn's cleanup so it never fires (draw assertion fails); a revert of the untap purge leaks it (expiry assertion fails). This firing path also exercises the aggregate->per-source expansion + consumed-identity fix (blocker 1) at runtime. The existing survives-then-purges persistence test is retained. [LOW] Removed the false-provenance CR annotations (CR 603.7c / CR 201.5) from the parser-scope TriggerConditionScope enum and its ParseContext field. CR 603.7c governs a delayed ability's object reference across zone/characteristic changes and CR 201.5 governs name-based self-reference — neither describes parser scope or pronoun classification. Per the annotation convention this is parser scaffolding, not a rule implementation, so it carries no CR citation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks @matthewevans — both second-round points addressed in [MED] firing path. Added [LOW] false-provenance CR annotations. Removed CR 603.7c / CR 201.5 from the parser-scope Verified locally: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current head c82717b fixes the prior lifecycle assertion gap, but the duration-class runtime test still bypasses the parser-derived production ability chain.
[HIGH] The new intervening-turn test constructs a replacement AbilityDefinition instead of resolving Kang's parsed chapter. Evidence: crates/engine/tests/integration/kang_dynasty_until_next_turn_rider.rs:319-353 creates a fresh CreateDelayedTrigger/Draw definition and directly calls resolve_ability_chain; the actual Kang parse is inspected only as an AST at :49-85. Why it matters: the behavior claimed by this PR is the complete parser -> parent-target propagation -> delayed-trigger resolver pipeline for the 12-card duration class. The synthetic fixture can remain green if the parsed chain loses its UntilControllersNextTurn expiry, fails to carry the parent target through its preceding tap/goad clauses, or attaches the delayed sub-ability at the wrong point. This is the same production seam this PR changes, not a helper-level equivalent.
Suggested fix: derive the resolved ability from parse_effect_chain(KANG_CHAPTER, ...) rather than recreating its delayed trigger, supply a real selected creature through that parsed chain, then drive the existing intervening-turn combat and expiry assertions. Add the same production-path coverage for a representative of the second measured duration signature if its parser/resolution path differs.
…a synthetic def (phase-rs#6884) [HIGH] The intervening-turn firing test constructed a fresh CreateDelayedTrigger AbilityDefinition and resolved that, so it could stay green even if the parsed chain lost its UntilControllersNextTurn expiry, failed to carry the parent target through the preceding tap/goad clauses, or attached the delayed sub-ability at the wrong point. Replace it with kang_parsed_chapter_rider_fires_on_intervening_turn_ then_expires, which resolves Kang's ACTUAL parsed chapter via parse_effect_chain(KANG_CHAPTER) + build_resolved_from_def, supplying a real chosen creature as the tap target. This exercises the full production seam the PR changes: parse -> SetTapState/Goad clauses -> parent-target propagation of the chosen creature into the rider's ParentTarget source (asserted: valid_source == SpecificObject{chosen}, tapped == true) -> the delayed-trigger resolver's expiry stamping. It then drives that goaded creature through unblocked combat against the controller on the intervening turn (asserts the controller drew and took combat damage) and crosses into the controller's next turn (asserts the rider was purged). The synthetic install helper is removed. Second signature (Jace/Tamiyo class): its delayed-trigger install/expiry/purge seam is identical (pinned by the parse-shape class-guard test). A runtime firing drive was probed and intentionally NOT added: its inner "it gets -X/-0 until end of turn" parses "it" to Pump{target: SelfRef} (the source), a pre-existing inner-effect misparse on that unclaimed card that this PR neither introduces nor touches — so a Sig-2 firing assertion would test pre-existing behavior, not the Gap-C expiry seam. Tracked as a separate follow-up. Verification: cargo fmt --all; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings (clean); Kang test file passes (4/4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes for current head e3c91fe.
[MED] The added parser-scope/anaphora comments cite CR provisions that do not support the claims. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:1049-1051 and crates/engine/src/parser/oracle_trigger.rs:9015-9021, 9023-9031, 9047-9055 use CR 603.7c / 201.5 to justify delayed-parser scope, anaphoric parent-target binding, and gendered-pronoun source binding. CR 603.7c instead concerns delayed-object identity after a characteristic/zone change, while CR 201.5 concerns an object referring to itself by name. Neither describes these parser semantics.
Why it matters: CR annotations are assertions of rules authority; using unrelated rules makes the parser behavior look rules-verified when it is not.
Suggested fix: remove these commentary citations, or replace them only with direct, verified citations that actually support the narrowly stated behavior. Please do not substitute a nearby delayed-trigger rule unless it demonstrably supports the annotation.
…omments (phase-rs#6884) [MED] Follow-through on the earlier context.rs annotation fix: the same false-provenance CR citations remained on the sibling parser-scope/anaphora comments this PR added. Removed CR 603.7c / CR 201.5 (and the CR 109.4 + 608.2c lead-in) from the four PR-introduced comments: - oracle_effect/mod.rs: the `Delayed` scope set site. - oracle_trigger.rs: the anaphoric-subjects gate, the gendered-pronoun→SelfRef arm, and the plural-set→ParentTarget arm. CR 603.7c governs a delayed ability's object identity across characteristic/zone changes and CR 201.5 governs name-based self-reference; neither describes these Oracle-text parsing heuristics (mode selection, pronoun/anaphora recognition). Per the reviewer's instruction the citations are removed rather than swapped for a nearby delayed-trigger rule that does not demonstrably support the behavior; the explanatory prose is retained and each is labeled parser scaffolding. Scope confirmed via `git diff <merge-base>...HEAD`: these four are the only CR 603.7c/201.5 lines this PR added to the parser; all other such citations are pre-existing and left untouched. Comment-only change. Verified: cargo fmt --all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Done, @matthewevans — you're right that the earlier Removed CR 603.7c / CR 201.5 (and the CR 109.4 + 608.2c lead-in) from all four PR-introduced parser-scope/anaphora comments:
Per your instruction I removed the citations rather than substituting a nearby delayed-trigger rule that doesn't demonstrably support the behavior; the explanatory prose stays and each is now labeled parser scaffolding (mode selection / Oracle-text interpretation, not a rule implementation). I also confirmed scope with Comment-only change; |
|
Maintainer hold for current head |
…provenance type (phase-rs#6884) The lifecycle-port merge (phase-rs#6933) canonicalized DelayedTrigger.provenance from Option<..> to the DelayedInstallIdentity enum. The maintainer's port updated every production construction but not the building-block regression test added by this PR, so `provenance: None` in `multi_fire_combat_damage_consumes_raw_aggregate_not_synthetic` failed to compile (E0308), which cascaded to red Rust lint + both test shards. Set it to `DelayedInstallIdentity::LegacyDelayed` (a normal, non-command delayed trigger — the same value the production install path uses in effects/delayed_trigger.rs). Test-only change. Verified against the ported head: engine tests compile clean; the PR's added tests pass at runtime under the new canonical lifecycle (no behavioral reconciliation needed) — full lib suite 18323 passed, combat/trigger/delayed integration sweep 800 passed; cargo fmt + engine clippy (-D warnings) clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks for doing the lifecycle port, @matthewevans. The red CI on the ported head was a single leftover from #6933's Fixed it to Verified against your ported head before pushing: engine tests compile clean, and the PR's added tests pass at runtime under the new canonical lifecycle with no behavioral reconciliation needed — full lib suite (18323) and the combat/trigger/delayed integration sweep (800) both green; |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes for current head ec3b6bde657741a4bdd3e4ff99ce23bfcbbcec6f.
[MED] The provenance test fixture adds a CR citation that does not describe the code. Evidence: crates/engine/src/game/triggers.rs:17755 says CR 603.7c for DelayedInstallIdentity::LegacyDelayed and the absence of a command receipt. Why it matters: CR annotations are rule-authority claims; 603.7c governs a delayed ability referring to the expected object after it changes characteristics or zones, not this internal lifecycle/test provenance representation. Suggested fix: remove the citation (the fixture is engine scaffolding), rather than substituting a nearby delayed-trigger rule.
The required parse-diff sticky is still bound to prior head 2b622272cdeef43bda103208bdc74173839619fc; re-review after the correction has terminal CI and current-head artifact evidence.
…ase-rs#6884) Follow-up to the provenance test fix: the comment I added cited CR 603.7c on a test-only DelayedInstallIdentity::LegacyDelayed assignment. 603.7c governs a delayed ability's object reference across characteristic/zone changes, not this internal lifecycle/test provenance representation. Removed the citation (engine test scaffolding is not a rule implementation); kept the phase-rs#6933 context note. Comment-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed in |
|
Maintainer hold for current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head c44a4451e8555ab8bc27464023391ddb557a0cc8: current-head CI and parse-diff are green; the 14-card/four-signature parser delta is reconciled with the delayed-trigger expiry and exact-two-attacker scope, and the parsed Kang runtime path covers binding, firing, and expiry.
2 participants
Summary
Adds engine support for the Delayed/created "deals combat damage to a player/opponent" trigger (CreateDelayedTrigger + WheneverEvent) with tracked-set/self source binding mechanic, unlocking 3 card(s) in this deck.
Cards unlocked
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— clean (exit 0)./scripts/check-parser-combinators.sh (Gate A)— clean (PASS, all families A-G incl. cross-product detector D) after fixing a Git Bash python3 environment issue via a cygpath-translating shim over mingw64 Python 3.9.7cargo clippy-strict— incomplete - still running (cold build; zero errors/warnings through all deps and most workspace crates, engine crate compiling); Tilt down so fallback cargo path usedcargo test -p phase-engine— not run (blocked on clippy-strict completing to avoid cargo target-lock contention)./scripts/gen-card-data.sh— not runcargo coverage— not runcargo semantic-audit— not runScope Expansion
Beyond the plan: added a new per-source expansion for multi-fire delayed combat-damage triggers (aggregate CombatDamageDealtToPlayer → per-source synthetic DamageDealt) so TriggeringSource resolves; seeded inner_ctx.subject so "it"→TriggeringSource; extended effect_uses_parent_target for GenericEffect+ParentTarget statics (A0); gated TriggeringSource creation snapshot on one_shot (A2); gated the anaphora arms on a new in_delayed_trigger flag; added a swallow-audit discharge (any_whenever_event_expiry) for the new expiry.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
New Features
Bug Fixes