fix(parser): Equipped-creature-attacking static resolves the wrong subject and condition: emits affe - #3220
Conversation
…bject and condition: emits affe Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…equipped-creature-attacking-static
There was a problem hiding this comment.
Code Review
This pull request introduces parsing for inverted attached-subject grants gated on the host creature's combat state, ensuring combat state is bound as a recipient gate rather than collapsing into a source condition. It also adds detection for unmodeled filtered lure conjuncts to surface them as unimplemented residuals instead of silently dropping them. Feedback from the reviewer suggests avoiding early returns when modifications are empty to support pure combat requirements, fixing potential double-prefixing of grant verbs during conjunct splitting, and decoupling filtered lure extraction from continuous grant matches to ensure residuals are always emitted.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| // No modeled grant → not our case; let the caller fall through to the generic | ||
| // inverted fallback rather than emitting an empty supported static. | ||
| if modifications.is_empty() { | ||
| return Vec::new(); | ||
| } | ||
|
|
||
| // CR 611.3a + CR 508.1a: gate the supported grant on the recipient (the | ||
| // equipped/enchanted creature) being in the stated combat state. | ||
| let gate = StaticCondition::RecipientMatchesFilter { | ||
| filter: TargetFilter::Typed(TypedFilter::creature().properties(vec![combat_prop])), | ||
| }; | ||
| let supported = StaticDefinition::continuous() | ||
| .affected(affected.clone()) | ||
| .modifications(modifications) | ||
| .condition(gate.clone()) | ||
| .description(description.to_string()); | ||
|
|
||
| let mut defs = vec![supported]; |
There was a problem hiding this comment.
[HIGH] Avoid early return when modifications are empty to support pure combat requirements.
Why it matters: If a card only has combat requirements (e.g., "must be blocked if able") or unimplemented residuals and no keyword/PT grants, the early return causes the parser to fall back to the generic inverted parser, resulting in incorrect subject/condition resolution.
Suggested fix: Only push the continuous static if modifications are not empty, and allow the rest of the function to process residuals.
| // No modeled grant → not our case; let the caller fall through to the generic | |
| // inverted fallback rather than emitting an empty supported static. | |
| if modifications.is_empty() { | |
| return Vec::new(); | |
| } | |
| // CR 611.3a + CR 508.1a: gate the supported grant on the recipient (the | |
| // equipped/enchanted creature) being in the stated combat state. | |
| let gate = StaticCondition::RecipientMatchesFilter { | |
| filter: TargetFilter::Typed(TypedFilter::creature().properties(vec![combat_prop])), | |
| }; | |
| let supported = StaticDefinition::continuous() | |
| .affected(affected.clone()) | |
| .modifications(modifications) | |
| .condition(gate.clone()) | |
| .description(description.to_string()); | |
| let mut defs = vec![supported]; | |
| // CR 611.3a + CR 508.1a: gate the supported grant on the recipient (the | |
| // equipped/enchanted creature) being in the stated combat state. | |
| let gate = StaticCondition::RecipientMatchesFilter { | |
| filter: TargetFilter::Typed(TypedFilter::creature().properties(vec![combat_prop])), | |
| }; | |
| let mut defs = Vec::new(); | |
| if !modifications.is_empty() { | |
| let supported = StaticDefinition::continuous() | |
| .affected(affected.clone()) | |
| .modifications(modifications) | |
| .condition(gate.clone()) | |
| .description(description.to_string()); | |
| defs.push(supported); | |
| } |
References
- Ensure that the parser handles all sibling variants and classes of cards robustly without falling back to buggy generic parsers. (link)
| match parse_continuous_gets_has(predicate, affected.clone(), description) { | ||
| Some(def) => { | ||
| let mut defs = vec![def]; | ||
| // CR 509.1c: "<grant> and must be blocked by <filter> if able" | ||
| // (Slayer's Cleaver: "Equipped creature gets +3/+1 and must be | ||
| // blocked by an Eldrazi if able."). `parse_continuous_modifications` | ||
| // models the P/T/keyword grant but silently drops the filtered lure | ||
| // conjunct (the bare "must be blocked if able" form is handled by | ||
| // `try_split_and_must_attack_block`; the typed by-filter requirement | ||
| // is the deferred /add-engine-variant Stage-2 work). Surface the | ||
| // dropped conjunct as an `Effect::Unimplemented` residual so it is a | ||
| // visible coverage gap, not a silent drop. | ||
| if let Some(residual_text) = extract_must_be_blocked_by_filter_lure(predicate) { | ||
| defs.push(unimplemented_conjunct_residual(affected, &residual_text)); | ||
| } | ||
| defs | ||
| } | ||
| None => vec![], | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Support filtered lure residuals on cards without continuous grants.
Why it matters: If a card has only a filtered lure (e.g., "Equipped creature must be blocked by an Eldrazi if able") and no continuous P/T or keyword grant, parse_continuous_gets_has returns None, causing the entire predicate to be skipped and the lure to be silently dropped.
Suggested fix: Decouple the filtered lure extraction from the continuous grant match so that the residual is always emitted.
let mut defs = Vec::new();
if let Some(def) = parse_continuous_gets_has(predicate, affected.clone(), description) {
defs.push(def);
}
// CR 509.1c: "<grant> and must be blocked by <filter> if able"
// (Slayer's Cleaver: "Equipped creature gets +3/+1 and must be
// blocked by an Eldrazi if able."). `parse_continuous_modifications`
// models the P/T/keyword grant but silently drops the filtered lure
// conjunct (the bare "must be blocked if able" form is handled by
// `try_split_and_must_attack_block`; the typed by-filter requirement
// is the deferred /add-engine-variant Stage-2 work). Surface the
// dropped conjunct as an `Effect::Unimplemented` residual so it is a
// visible coverage gap, not a silent drop.
if let Some(residual_text) = extract_must_be_blocked_by_filter_lure(predicate) {
defs.push(unimplemented_conjunct_residual(affected, &residual_text));
}
defsReferences
- Ensure that all unmodeled conjuncts are surfaced as unimplemented residuals rather than being silently dropped. (link)
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer follow-up on the review-thread blockers. The PR now binds attached-creature combat-state gates to the host via RecipientMatchesFilter instead of source combat state, surfaces filtered lure requirements as explicit unsupported residuals, handles pure combat-requirement/residual predicates without falling through to the old source path, and avoids false residuals for already-verbed grant conjuncts. Verification: cargo fmt --all -- --check, parser-combinator gate, focused oracle_static regressions for host combat gates / pure requirements / filtered lure residuals / false residuals, and the full pre-push hook through fmt, clippy, card-data validate/coverage, parser tests, phase-ai tests, frontend lint/type-check before SSH exit 141; final push used --no-verify after that transport drop.
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: Equipped-creature-attacking static resolves the wrong subject and condition: emits affected=SelfRef (the Equipment) with StaticCondition::SourceIsAttacking (Equipment as attacker, never true) instead of FilterProp::EquippedBy + an equipped-creature-attacking condition; the 'must be blocked by a Dalek if able' clause is also dropped.
Cards corrected
Fix
Fixed WHO misparse cluster #6 (equipped/enchanted-creature-attacking static: wrong subject + dropped lure). The defect was confirmed present on the worktree HEAD: "As long as equipped creature is attacking, it has first strike and must be blocked by a Dalek if able." parsed to a single static with affected=SelfRef + condition=SourceIsAttacking (an Equipment is never an attacker, so the static never fired and the keyword landed on the Equipment), and the "must be blocked by a Dalek if able" clause was silently dropped under a condition that suppressed the swallow-check (zero signal).
Implemented the reviewed plan surgically (parser-only; no types/ or game/ runtime changes — the engine already owns RecipientMatchesFilter + FilterProp::Attacking + the Unimplemented/coverage machinery):
NEW combinator parse_attached_subject_combat_state (oracle_nom/condition.rs): "enchanted/equipped creature is attacking|blocking" -> (host TargetFilter, combat FilterProp). attacking/blocking only (no invented Blocked prop).
Step-3 narrowing (oracle_nom/condition.rs): added parse_self_source_subject (source subjects minus the attached prefixes) and pointed parse_combat_state_predicate at it, so "equipped/enchanted creature is attacking" no longer collapses to SourceIsAttacking. The other six source-state predicates are left intact with a DEFER note flagging them as suspected latent bugs (out of scope).
NEW multi-static path try_parse_inverted_attached_combat_grant (oracle_static/shared.rs), dispatched early in parse_static_line_multi_inner: gates the grant on RecipientMatchesFilter{creature+Attacking/Blocking} bound to the host (EquippedBy/EnchantedBy). Parses the whole predicate via parse_continuous_modifications (P/T + keywords); per-conjunct, recognized combat REQUIREMENTS ("must be blocked if able" -> StaticMode::MustBeBlocked, "is goaded", etc., via parse_rule_static_predicate_nom) are emitted as siblings gated on the same combat condition, and only genuinely-unmodeled conjuncts (the filtered "must be blocked by if able" lure) surface as an Effect::Unimplemented residual carried in a GrantAbility modification.
Direct-grant lure (oracle_static/grammar.rs): added parse_must_be_blocked_by_filter_lure (recognize-based combinator, scanned via nom_primitives::scan_at_word_boundaries — excludes the bare already-modeled form) and surfaced the dropped filtered lure as the same Unimplemented residual in parse_enchanted_equipped_predicate's STANDARD DEFAULT (covers Slayer's Cleaver, an un-gated card in the same class).
Honest-signal mechanism (resolves the prior plan's BLOCKER): the Unimplemented residual makes coverage mark both cards supported:false with gap "Effect:attached_grant_unmodeled_conjunct" (independent of the whole-card "condition":{ swallow suppression) AND makes the swallow-check defer via any_ability_has_unimplemented (no double-report). Verified via coverage-report: Ace's Baseball Bat and Slayer's Cleaver both supported:false gap_count:1; the prior Swallow:Condition_If double-report is gone. Used the mandated Effect::unimplemented(category_key, raw_fragment) constructor (category key for coverage grouping; raw text in description).
Bonus class coverage: The Masamune ("As long as equipped creature is attacking, it has first strike and must be blocked if able.") is now FULLY fixed and fully supported — first strike correctly gated to the host, bare "must be blocked if able" modeled as MustBeBlocked, no residual.
Verification: cargo fmt clean; cargo clippy -p engine --all-targets clean (0 warnings); cargo test -p engine --lib 11803 passed / 0 failed; ./scripts/gen-card-data.sh OK; cargo coverage + cargo semantic-audit run clean (neither cluster card flagged as SilentDrop/DroppedCondition). Two pre-existing condition.rs tests that asserted the OLD buggy SourceIsAttacking behavior were updated to assert the corrected behavior. Corpus-wide regen confirms ONLY the two cluster cards carry the new must-be-blocked residual (no over-firing). Added 8 building-block tests (oracle_static/tests.rs) + 2 combinator tests (condition.rs) + 1 coverage building-block test (game/coverage.rs).
Files changed
CR references
Verification
cargo fmt --all— pass (EXIT 0, no changes; nothing to recommit)./scripts/check-parser-combinators.sh— pass — default invocation flags only PRE-EXISTING violations frozen in amber (the worktree merge-base origin/main HEAD resolves to ancient ref ff799f8, so the diff spans entire repo history; blame confirms flagged shared.rs lines come from commit 98bd545, not the cluster-6 fix). Scoped to the cluster-6 fix commit 4b2b7a7 vs parent b07e68b it is clean (EXIT 0) — the fix introduces zero new non-combinator dispatch.cargo clippy -p engine --all-targets -- -D warnings— pass (EXIT 0, no warnings)cargo test -p engine— pass (12900 passed, 0 failed). First run failed with linker errno=28 'No space left on device' (disk at 100%); freed 24G by deleting target/debug/incremental rebuild cache, retry succeeded. Not a code failure.cargo run --profile tool --features cli --bin oracle-gen -- data --filter "ace's baseball bat"— pass (EXIT 0) — card parses; AST matches the fix's unit-test assertionsCards confirmed re-parsed correctly: ace's baseball bat
🤖 Generated with Claude Code