fix(parser): enforce the "if it didn't have decayed" intervening-if on dies triggers (Wilhelt, the Rotcleaver)

[michiot05]

Tier: Frontier
Model: claude-fable-5
Thinking: high

Closes #5937.

Summary

Wilhelt, the Rotcleaver — "Whenever another Zombie you control dies, if it didn't have decayed, create a 2/2 black Zombie creature token with decayed." Discord report: Wilhelt mints tokens for the deaths of Zombies that did have decayed — including his own decayed tokens, creating a value loop the card is explicitly designed to prevent.

Root cause: the past-tense keyword-possession intervening-if has no parser production. extract_if_condition_with_card_name recognizes was /wasn't zone-change predicates and counter forms, but "if it didn't have decayed" falls through to parse_clause_ast, which strips the conditional head and returns ClauseAst::Conditional { condition: None } — the guard is discarded and the token effect parses unconditionally. The swallow detector correctly flags it (Condition_If, so coverage stays red), but the runtime AST plays without the CR 603.4 gate.

Class-level fix, not a Wilhelt special case: one nom production, parse_zone_change_object_keyword_possession, covering the full tense × polarity × keyword-registry cross-product — if it (had | didn't have | did not have) <keyword> — lowering to the existing Not(ZoneChangeObjectMatchesFilter { Battlefield → Graveyard, creature + HasKeywordKind }) shape, wired as a new first arm of parse_zone_change_object_filter_predicate beside the existing was/wasn't and P/T-comparison arms. Zero engine changes: the runtime path pre-exists end-to-end (fire-time gate + CR 603.4 resolution re-check both read the immutable ZoneChangeRecord snapshot, which captures layer-computed keywords per CR 603.10a look-back — so continuous grants like "gains decayed until end of turn" count, verified by a hostile test). Guard rejects KeywordKind::Unknown (the shared-kind alt-cost families) and KeywordKind::Landwalk (the land-type parameter folds to a single kind; a kind-level gate would over-match across land types per CR 702.14a) — those forms stay honestly swallowed with the Condition_If diagnostic intact.

Implementation method (required)

Method: /engine-implementer — plan → /review-engine-plan (1 round; 3 findings integrated: dies-shape fail-open caveat, KeywordKind-level guard, overlap note) → implement → /review-impl (2 rounds: round 1 returned one MED finding — the landwalk kind-fold over-match — fixed with a guard extension + honest-swallow test; round 2 clean) → commit, each step in a fresh agent context. Final read-only /review-impl against the committed head: PASS head=4f8c6171e50f04c0a885af90248343a57b3a2f5b (seam + idiom checks explicitly confirmed).

Gate A

./scripts/check-parser-combinators.sh → Gate A PASS head=4f8c6171e50f04c0a885af90248343a57b3a2f5b base=094d4e5630de0659ee3d62b959cb851b12c3e632 (Gate G PASS as well). The new production is pure nom (alt/value/tag + the shared parse_keyword_name primitive); no string probes anywhere in the diff.

Anchored on

• crates/engine/src/parser/oracle_trigger.rs:5782 — parse_dying_object_pt_comparison_condition (Massacre Girl, Known Killer): the identical dies-trigger intervening-if shape (ZoneChangeObjectMatchesFilter, Battlefield → Graveyard, LKI record evaluation) this arm mirrors, including the Not negation idiom of the adjacent was/wasn't axis.
• crates/engine/src/parser/oracle_trigger.rs:5619 — parse_event_object_has_alt_cost_keyword_intervening_if ("if it has madness" → FilterProp::HasKeywordKind): the present-tense keyword-possession sibling; the new arm is its past-tense/LKI counterpart.
• crates/engine/src/game/filter.rs:5069 — the pre-existing HasKeywordKind arm of zone_change_record_matches_property: the runtime consumer, unchanged.

Verification

• cargo fmt --all clean; cargo clippy -p engine --tests 0 warnings.
• New integration suite (crates/engine/tests/integration/wilhelt_decayed_intervening_if_5937.rs, registered in main.rs) — 11 passed: parse-fidelity rows (typed condition present; Condition_If swallow cleared; token effect intact) + runtime rows through the real cast → death → trigger pipeline with Wilhelt's verbatim Oracle text: decayed death → no token (the reported bug), non-decayed death → exactly one token, the reported token-loop scenario, SBA deaths both ways (CR 704.5f), the hostile LKI row (decayed granted until end of turn, then dies → no token — proves the snapshot reads layer-computed keywords), and scope siblings (opponent's Zombie, Wilhelt itself).
• New parser unit rows — 4 passed: un-negated positive form ("if it had flying"), HadCounters precedence lock, and two coverage-honesty rows (non-keyword and islandwalk both stay swallowed).
• Stay-green guards: l02_bb4_intervening_if suite (23 passed), Massacre Girl suite (4 passed), full oracle_trigger lib subset (1023 passed, 0 failed).
• Fails before, passes after (tests written first, run against the unfixed parser): FAILED. 5 passed; 6 failed — wilhelt_condition_is_not_had_decayed panicked ("must carry an intervening-if condition"), wilhelt_decayed_death_creates_no_token / _token_death_does_not_loop / _sba_death_decayed / _granted_decayed_death all failed left: 1, right: 0 (the live bug). After the fix: 11 passed, 0 failed.
• CR annotations: every number grep-verified against docs/MagicCompRules.txt before writing — CR 603.4 (:2592), 603.10a (:2638), 603.6c (:2604), 700.4 (:3234), 400.7 (:1950), 608.2h (:2806), 702.147a (:5090), 702.14a (:4015), 613.1f (:2970), 704.5f (:5502). One self-caught correction during the gate: 704.5g (lethal damage) → 704.5f (toughness ≤ 0) for the SBA fixture comment.

Claimed parse impact

Regenerated card-data.json and diffed against the pre-change baseline (same base commit): exactly 1 entry changed — wilhelt, the rotcleaver. Its Condition_If swallow warning is removed and the dies trigger's condition: null becomes the typed Not(ZoneChangeObjectMatchesFilter{...}). Corpus sweep confirms Wilhelt is the only current printing with this grammar (every other "if it had …" form is the counter/mana-value family, structurally rejected by the closed keyword table); the production nonetheless covers the whole tense × polarity × keyword class for future printings.

Scope Expansion

None.

🤖 Generated with Claude Code

Summary by CodeRabbit

• Bug Fixes
  • Improved parsing of intervening-if past-tense possessive clauses (“had/didn’t have ”) for zone-change conditions, including correct negation and safer handling of unsupported keyword types.
  • Refined dies-trigger detection/conditioning for Wilhelt so “Decayed” replacement triggers match only the intended dying Zombie, avoids replacement loops, and respects correct snapshot timing and scope.
• Tests
  • Added integration coverage for the updated parsing behavior (including swallowed-clause diagnostics) and expanded Wilhelt Issue #5937 runtime and snapshot/scoping scenarios.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 6984d12f-60b6-4d03-abf0-c8c0056e7df7

📥 Commits

Reviewing files that changed from the base of the PR and between eab3fda and 112cb48.

📒 Files selected for processing (2)

• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/tests/integration/main.rs

🚧 Files skipped from review as they are similar to previous changes (2)

• crates/engine/tests/integration/main.rs
• crates/engine/src/parser/oracle_trigger_tests.rs

---

📝 Walkthrough

Walkthrough

The parser recognizes past-tense keyword possession in dies-trigger zone-change filters, rejects unsupported keyword kinds, and centralizes dies look-back construction. Parser and Wilhelt integration tests cover intervening-if parsing, decayed-token replacement, state-based deaths, layer-derived keywords, and trigger scope.

Changes

Oracle trigger and Wilhelt regression

Layer / File(s)	Summary
Dies trigger recognition and wiring crates/engine/src/parser/oracle_trigger.rs	Tightens dies-head recognition, threads the zone-change gate through condition parsing, and adds a dedicated dies verb-phrase parser.
Keyword possession parser crates/engine/src/parser/oracle_trigger.rs	Parses “had” and negated possession clauses, validates keyword kinds, and shares battlefield-to-graveyard look-back construction with PT comparisons.
Parser regression coverage crates/engine/src/parser/oracle_trigger_tests.rs	Covers keyword-kind matching, counter-condition precedence, rejected keywords and landwalk forms, non-dies lookbacks, and swallowed-clause diagnostics.
Wilhelt parsing and runtime coverage crates/engine/tests/integration/main.rs, crates/engine/tests/integration/wilhelt_decayed_intervening_if_5937.rs	Adds the integration module and verifies Wilhelt’s intervening-if condition, decayed replacement behavior, SBA deaths, layer-computed keywords, and trigger scope.

Estimated code review effort: 4 (Complex) | ~45 minutes

Suggested labels: bug

Suggested reviewers: matthewevans

Sequence Diagram(s)

sequenceDiagram
  participant OracleText
  participant DiesTriggerParser
  participant KeywordPossessionParser
  participant TriggerCondition
  OracleText->>DiesTriggerParser: provide dies trigger with had or did not have clause
  DiesTriggerParser->>KeywordPossessionParser: parse keyword possession under battlefield-to-graveyard gate
  KeywordPossessionParser->>TriggerCondition: construct positive or negated dies look-back condition
  TriggerCondition-->>DiesTriggerParser: return intervening-if condition

Loading

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main parser change for Wilhelt’s dies-trigger intervening-if condition.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

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_trigger.rs`:
- Around line 6002-6021: Extract the repeated dies-lookback construction into a
shared helper near the existing trigger-condition parsing logic, including the
Battlefield-to-Graveyard Typed creature filter and the negated/non-negated
wrapping. Update this block and the was/wasn’t axis (and the documented
P/T-comparison arm) to call the helper, preserving their current parsing results
and leaving future non-dies handling able to derive zones from trigger context.

🪄 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: 75c88035-a019-4fdf-b63d-a7ed3bb5de76

📥 Commits

Reviewing files that changed from the base of the PR and between d568002 and 4f8c617.

📒 Files selected for processing (4)

• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/parser/oracle_trigger_tests.rs
• crates/engine/tests/integration/main.rs
• crates/engine/tests/integration/wilhelt_decayed_intervening_if_5937.rs

[github-actions]

Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main 7195a8e31e14)

🟡 Modified fields (1 signature)

• 1 card · 🔄 trigger/ChangesZone · changed field condition: ∅ → not (object entering graveyard is with decayed creature)
  • Affected (first 3): Wilhelt, the Rotcleaver

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Blocked — the generalized keyword-lookback grammar is unsound outside dies triggers.

🔴 Blocker

crates/engine/src/parser/oracle_trigger.rs:5378-5381 routes every trigger through the new had/didn't have <keyword> extraction. That production builds dies_lookback_condition at :5911-6011, which pins Battlefield → Graveyard at :5952-5965. Runtime evaluation compares those zones against the actual event in crates/engine/src/game/triggers.rs:8380-8393; for a non-dies event, the inner predicate is false and the negation in “didn't have” becomes true, making the gate fail open.

Thread or derive the trigger's actual zone-change context before creating the filter, or restrict this grammar to proven dies shapes. Add an entering/non-dies negative regression so unsupported forms remain honestly swallowed until generalized.

✅ Clean

The prior helper-extraction feedback is addressed by the current dies_lookback_condition reuse; the current parse diff is limited to Wilhelt.

Recommendation: request changes for correct zone-context binding and a non-dies regression.

[michiot05]

Thanks for the review — the blocker is fixed on head eab3fda7a by your sanctioned option 2 (restrict to proven dies shapes), implemented so option 1 stays a natural extension:

The gate. parse_trigger_line_with_index_ir now proves the trigger head's zone-change shape before intervening-if extraction: trigger_head_dies_zone_change() parses the head's final verb phrase through a shared parse_dies_verb_phrase combinator (extracted from parse_zone_change_clause, so the prover and the event constructor recognize the identical verb set — dies / "is put into a graveyard from the battlefield", CR 700.4) and threads Option<(Zone, Zone)> down through extract_if_condition_with_card_name → parse_zone_change_object_filter_predicate. The keyword-possession arm binds only under a proven (Battlefield, Graveyard) pair. I threaded the zone pair rather than a boolean so a future LTB/exile generalization derives zones and relaxes the match without re-plumbing — your option 1's vocabulary, gated to option 2's scope today. Qualified heads ("…dies during your turn") fail conservatively to the swallow diagnostic, never open.

The demanded regressions. Two new rows mirror the existing swallow tests: the same clause on an ETB head (extract_didnt_have_keyword_on_etb_trigger_stays_swallowed) and on an LTB head (..._on_ltb_trigger_stays_swallowed) — condition None and the Condition_If swallow still firing. Both were red against the previous head with exactly the fail-open shape your review predicted bound as the condition (Some(Not(ZoneChangeObjectMatchesFilter{ Battlefield→Graveyard, HasKeywordKind(Decayed) }))); green after the gate.

Scope note. Only the keyword-possession arm is gated. The sibling was/wasn't and P/T-comparison arms share the same hardcoded zone pair (pre-existing, fail-closed for their positive forms) and can adopt the threaded context in a follow-up — noted in the arm's doc comment, left untouched here to keep the parse diff Wilhelt-only.

Verification on eab3fda7a: Gate A + Gate G PASS; cargo clippy -p engine --tests clean; targeted parser rows 10/10 (both new regressions + positive dies-head arm + HadCounters precedence + landwalk/non-keyword swallows); Wilhelt integration 11/11; full parser::oracle_trigger module 1026/1026. Parse output unchanged vs the prior head (the gate keeps Wilhelt bound and only declines heads where nothing bound before), so the parse-diff artifact should continue to show Wilhelt only.

Model: claude-fable-5

[matthewevans]

Approved — the zone-context blocker is resolved on 112cb486b1b2020d642068c9b58280f778140cfb.

trigger_head_dies_zone_change now proves the battlefield-to-graveyard head before the keyword-lookback arm can construct its negated condition; the ETB and LTB regressions keep unsupported non-dies forms honestly swallowed. The current parse-diff artifact is Wilhelt-only, matching the claimed scope, and the runtime integration path is discriminating.