fix(parser): keep both targets of the two-target fight class (Tail Swipe) - #5583
Conversation
…ipe) "Choose target creature you control and target creature you don't control. [buff the you-control creature]. Then those creatures fight each other." (phase-rs#4751 — Tail Swipe, Joust, Blizzard Brawl, plus the already-supported counter siblings Malamet Battle Glyph, Longstalk Brawl, Duel for Dominance) silently dropped its SECOND target when the buff was a pump ("gets +N/+M"). The card "half worked": only the you-control creature was ever targeted. Root cause (CR 601.2c): `starts_target_continuous_clause_lower` recognises a "target <noun> gets/gains/has/loses ..." continuous clause by scanning with `take_until(" gets ")` — but that scan ran ACROSS the sentence period. For these cards the "gets +N/+M" verb lives in a LATER sentence ("... you don't control. The creature you control gets +1/+1"), so the scan reached past the period and mis-classified the two-target DECLARATION's second slot as a continuous clause. The clause splitter then bisected "target A and target B" at its "and", and the orphaned "target creature you don't control" chunk fell through to `Effect::unimplemented("target", ...)`. Counter cards (Malamet: "put a +1/+1 counter on it") have no such verb, so they were unaffected — which is why the gap was Pump-specific and previously deferred. Fix, in three parts: - Bound the continuous-verb scan to the current sentence (stop at the first ". "), so a "gets/gains/has/loses" in a later sentence no longer pulls the two-target declaration apart. Skulduggery ("target creature you control gets +1/+1 and target creature an opponent controls gets -1/-1") — a genuine same-sentence two-target-continuous — still splits correctly. - Extend the existing two-target buff rekey (previously `PutCounter`-only) to the two remaining buff shapes the class uses: `Pump{Any|ParentTarget}` (Tail Swipe / Joust) and a SelfRef-affected `GenericEffect{target:None}` (Blizzard's "gets +N/+M and gains indestructible"). Both bind slot 0 — the first-declared you-control creature — instead of an unscoped whole-board target. The rekey is gated on the chain declaring >= 2 TargetOnly slots, so single-target pumps and global anthems are untouched. CR 611.2c: the reciprocal "those creatures fight each other" object stays `ParentTarget` by design (`parse_fight_target`), so the fight itself needs no rekey. Verified: all six class cards parse with no residual Unimplemented and the buff bound to slot 0 (checked against the real card-data.json entries); the full parser test suite (7919) is green; Skulduggery and a global anthem are unregressed. New test: `two_target_fight_pump_keeps_both_slots_and_buffs_slot_zero`. Fixes phase-rs#4751.
There was a problem hiding this comment.
Code Review
This pull request generalizes the two-target fight buff rekeying logic to support Pump and GenericEffect shapes alongside PutCounter, and bounds the continuous clause verb scan to the current sentence to prevent mis-parsing two-target declarations. The feedback suggests renaming the generalized functions to reflect that they now handle all buffs, and correcting the nom parser in starts_target_continuous_clause_lower to return the properly advanced remainder instead of the unadvanced input.
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.
| let segment = match take_until::<_, _, OracleError<'_>>(". ").parse(rest) { | ||
| Ok((_, seg)) => seg, | ||
| Err(_) => rest, | ||
| }; | ||
| let _ = alt(( | ||
| value((), (take_until(" gets "), tag(" gets "))), | ||
| value((), (take_until(" gains "), tag(" gains "))), | ||
| value((), (take_until(" has "), tag(" has "))), | ||
| value((), (take_until(" loses "), tag(" loses "))), | ||
| )) | ||
| .parse(rest) | ||
| .parse(segment)?; | ||
| Ok((rest, ())) |
There was a problem hiding this comment.
[HIGH] Ensure standard nom parser discipline by returning the correct remaining input. Evidence: crates/engine/src/parser/oracle_effect/sequence.rs:2243-2254.
Why it matters: Returning the unadvanced rest violates standard nom parser expectations and could cause silent parsing bugs or infinite loops if future callers rely on the consumed remainder. Suggested fix: Calculate the consumed length from the segment and slice rest accordingly.
let segment = match take_until::<_, _, OracleError<'_>>(". ").parse(rest) {
Ok((_, seg)) => seg,
Err(_) => rest,
};
let (remaining_in_segment, _) = alt((
value((), (take_until(" gets "), tag(" gets "))),
value((), (take_until(" gains "), tag(" gains "))),
value((), (take_until(" has "), tag(" has "))),
value((), (take_until(" loses "), tag(" loses "))),
))
.parse(segment)?;
let consumed_len = segment.len() - remaining_in_segment.len();
Ok((&rest[consumed_len..], ()))References
- Avoid verbatim string equality for parsing Oracle phrases; instead, decompose compound phrases into modular, reusable parsers and compose them using idiomatic combinator aggregates like nested alt and tag sequences.
| pub(super) fn rewrite_two_target_counter_chain(def: &mut AbilityDefinition) { | ||
| if count_typed_target_only_slots(def) >= 2 { | ||
| rekey_counter_slot_in_chain(def); |
There was a problem hiding this comment.
[MED] Rename functions to reflect generalized buff rekeying. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:1667-1669.
Why it matters: The functions rewrite_two_target_counter_chain and rekey_counter_slot_in_chain now handle Pump and GenericEffect in addition to counters, making the "counter" naming misleading and harder to maintain. Suggested fix: Rename them to rewrite_two_target_buff_chain and rekey_buff_slot_in_chain.
| pub(super) fn rewrite_two_target_counter_chain(def: &mut AbilityDefinition) { | |
| if count_typed_target_only_slots(def) >= 2 { | |
| rekey_counter_slot_in_chain(def); | |
| pub(super) fn rewrite_two_target_buff_chain(def: &mut AbilityDefinition) { | |
| if count_typed_target_only_slots(def) >= 2 { | |
| rekey_buff_slot_in_chain(def); |
| @@ -1684,15 +1688,46 @@ fn count_typed_target_only_slots(def: &AbilityDefinition) -> usize { | |||
| } | |||
|
|
|||
| fn rekey_counter_slot_in_chain(def: &mut AbilityDefinition) { | |||
There was a problem hiding this comment.
[MED] Rename helper function to reflect generalized buff rekeying. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:1690.
Why it matters: The helper function now handles Pump and GenericEffect in addition to counters, so renaming it to rekey_buff_slot_in_chain improves code readability and maintainability. Suggested fix: Rename the function to rekey_buff_slot_in_chain.
| fn rekey_counter_slot_in_chain(def: &mut AbilityDefinition) { | |
| fn rekey_buff_slot_in_chain(def: &mut AbilityDefinition) { |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the sentence-bounding fix (part 1) is right and I want it; the buff rekey (part 2) widens a rewrite that runs over the whole card pool behind a guard that never checks for a fight.
Thanks for this — the root-cause analysis is genuinely good, and it's a strong first contribution. I verified your claims rather than taking them on faith, and most of them hold up.
🔴 Blocker — the rekey's guard doesn't encode the class it documents
rekey_counter_slot_in_chain (crates/engine/src/parser/oracle_effect/lower.rs:1686-1713) recurses into sub_ability and else_ability, rewriting every matching node. It runs from the generic assembly pipeline (crates/engine/src/parser/oracle_effect/assembly.rs:1860), so it is applied to every ability chain in the pool. Its only guard is count_typed_target_only_slots(def) >= 2 (lower.rs:1664).
That guard never checks for a fight. The function is named rewrite_two_target_counter_chain and its doc comment describes "the two-target fight class" — but what it actually tests is "this chain has ≥2 typed target slots." Those are not the same set. While the matcher was PutCounter{ParentTarget}-only, the gap was masked (measured: exactly 3 cards). Widening it to Pump{Any|ParentTarget} unmasks it.
Consequence: in any ≥2-target chain, every Pump{Any|ParentTarget} node in the chain is rewritten to ParentTargetSlot { index: 0 } — including chains with more than one pump, which then all collapse onto the same slot.
The full pool (data/card-data.json, 35,396 faces) has 45 faces with ≥2 target and ≥2 gets +/-. The ones structurally at risk:
| Card | Oracle text | If the pumps lower to Any/ParentTarget |
|---|---|---|
| Consume Strength | "Target creature gets +2/+2… Another target creature gets -2/-2…" | both the buff and the debuff land on slot 0 — same creature, net zero, opponent untouched |
| Leeching Bite | "+1/+1 … Another target creature gets -1/-1" | same collapse |
| Drooling Groodion | "+2/+2 … Another target creature gets -2/-2" | same collapse |
| Gloom Ripper | "target creature you control gets +X/+0 … up to one target creature an opponent controls gets -0/-X" | the debuff lands on your own creature |
| Gurmag Rakshasa | "target creature an opponent controls gets -2/-2 … and target creature you control gets…" | same, inverted |
| Arm the Cathars | "+3/+3, up to one other target creature gets +2/+2, and up to one other … +1/+1" | all three buffs collapse onto one creature |
| Bounty of Might | three × "Target creature gets +3/+3" | all three onto one creature |
To be precise about what I have and have not established. Confirmed by reading the source: the rewrite is pool-wide, it recurses over the whole chain, it rewrites every matching node to index 0, and the guard is ≥2-slots-only. Not confirmed: whether these specific cards' pumps actually lower to Any/ParentTarget rather than an explicit Typed filter — if they all lower to Typed, the guard never fires and they're safe. That is exactly the claim your comment makes ("Only the two-target fight chain routes a Pump through here… an unscoped Any/ParentTarget target is always that back-reference"), and it is an empirical claim about 35,396 faces that has been checked against 6.
I'm blocking on the evidence standard rather than a proven break, because this failure mode is invisible to every gate we have: a pump bound to the wrong slot emits no Effect::Unimplemented, produces no coverage delta, and — I checked — no test in the repo covers Consume Strength, Leeching Bite, Drooling Groodion, Gloom Ripper, or Arm the Cathars. It would land silently and we'd find out from a player.
Fix — make the guard mean what the doc comment says. Gate the rewrite on the class's actual signature: the presence of an Effect::Fight node (crates/engine/src/types/ability.rs:9737) in the chain, not on "has ≥2 target slots." That is the real categorical boundary, it excludes every card in the table above by construction, and it makes the widened matcher safe without an empirical argument. Since the class has exactly one buff, additionally asserting a single buff node would foreclose the multi-pump collapse entirely.
If you'd rather keep the slot-count guard, then the ask is a full-pool dual-run (all 35,396 faces in data/card-data.json, not the ~2,410-face fixture) enumerating every face that reaches the Pump{Any|ParentTarget} + ≥2-slot path, with confirmation that each is genuinely the fight-class back-reference. Note the fixture corpus is ~7% of the pool, so a green fixture run is not evidence here.
🟡 Non-blocking
- CI on this head proves nothing yet. As a first-time contributor your workflows were gated: only
Apply AI-CONTRIBUTOR triage labels,Contributor trust(skipping), andSuperagent Security Scanran. Clippy, test-engine, card-data, coverage, and the parse-diff sticky never executed, so the "green" is an artifact of "nothing that ran, failed." I've approved the runs — real CI will now execute, and the<!-- coverage-parse-diff -->sticky it produces is required review evidence for a parser change like this. Please confront that card-level diff against your claimed scope when it appears. - The PR description is missing the AI-CONTRIBUTOR template sections (
summary,files changed,track,llm). Please seedocs/AI-CONTRIBUTOR.mdand fill them in — they're the gates we review against. CR 611.2cis correctly cited for the pump (a resolution-generated continuous effect's affected set is fixed when it begins), but the sentence in your description attaches it to "those creatures fight each other," which is CR 701.14 (Fight), not a continuous effect. The in-code annotations are fine; it's just the prose.
✅ Clean — verified, not assumed
- The sentence-bounding fix is correct and I want it. Bounding the verb scan to before the first
". "so a later sentence'sgets/gains/has/losescan't be pulled back onto a two-target declaration is the right seam, and the root-cause write-up is accurate. - Your "the boolean caller ignores the remainder" claim checks out. I traced it: the sole call site is
sequence.rs:2709, inside a chain terminating in.parse(s).is_ok()atsequence.rs:2763-2764. The remainder is genuinely discarded, so returningrestrather than the post-verb remainder is safe. Thank you for flagging it explicitly instead of leaving it for a reviewer to discover. - Both CR citations grep-verified against
docs/MagicCompRules.txt. CR 601.2c really is the multi-instance-of-"target" rule (line 2461) and CR 611.2c really is the continuous-effect affected-set rule (line 2909). Correctly chosen, not decorative — that's rarer than it should be. - Composed from existing combinators (
take_until/alt/value), no new enum variants, no verbatim Oracle-text matching.
Recommendation: re-key the guard on Effect::Fight (or bring the full-pool measurement), push, and let the now-unblocked CI run. Part 1 lands as-is. I'll re-review on the new head.
Parse changes introduced by this PR · 7 card(s), 6 signature(s) (baseline: main
|
|
Correction — I withdraw the main blocker. The parse-diff I asked for arrived, and it refutes my claim. What's left is much smaller, but one item is real and I'd like it resolved before merge. ⬜ Withdrawn: the pump-collapse collateralI claimed that widening the matcher to I flagged that as an evidence-standard block, not a proven break, and said explicitly that I hadn't confirmed whether those pumps lower to 🔴 Blocker (the real one, and it's narrow)Two cards moved in opposite directions on the same construction. From the parse-diff:
These are the same grammatical shape:
Both are The direction that worries me is Life at Stake losing Neither card is in your claimed scope, so these are collateral from part 1 (bounding the verb scan to the current sentence), not from the rekey. What I need: account for both. If Life at Stake losing 🟡 Non-blocking, but I'd still take the one-line fix
Gating on ✅ Credit where it's duePart 1 — bounding the verb scan to the current sentence — is the right seam, and your claim that the caller ignores the remainder checks out: the sole call site ( Recommendation: resolve the Life at Stake / Mages' Contest divergence and I'll approve. The fight-gate is a request, not a gate. |
…e at Stake Review follow-up (phase-rs#4751, matthewevans): - Gate rewrite_two_target_counter_chain on the class's real signature — >= 2 typed target slots AND an Effect::Fight in the chain (chain_contains_fight), CR 701.14a. The slot count alone said nothing about a fight; requiring the Fight node makes the widened Pump/GenericEffect rekey safe by construction rather than by the pool happening not to trip it. All six class cards (Malamet, Longstalk, Duel, Tail Swipe, Joust, Blizzard Brawl) still rekey to slot 0. - Pin Life at Stake: the review's collateral 'you removed' is a stray Unimplemented("you") head dropping, not a lost chooser. The functional Choose{NumberRange} + exile + lose-life chain is byte-identical to main; the card is unimplemented on both sides. Test asserts the chain heads at the NumberRange Choose, keeps the exile/lose-life tail, and no longer carries a stray Unimplemented("you").
|
Thanks — and thanks for the correction round; the parse-diff resolving the collapse claim was exactly the right call. On the remaining item, I dug into both cards and I think the divergence is benign. Evidence, then a pinning test. Life at Stake is not regressed — the
|
|
Blocker withdrawn — I verified your refutation independently, and you're right. Approving on settled green; nothing further needed from you. ⬜ Withdrawn: the Life at Stake regressionI claimed Life at Stake was losing a chooser, and that the card would simply be broken. It isn't, and I checked rather than taking your word for it. From "abilities": [{
"kind": "Spell",
"effect": { "type": "Unimplemented", "name": "you", "description": "You" },
"sub_ability": {
"effect": { "type": "Choose", "choice_type": { "NumberRange": { "min": 0, "max": 20 } } },
...
}
}]The head node on So the divergence you explained is exactly what you said it is: both cards are unimplemented on both sides, and the sign of the That's the second claim of mine this PR has refuted with evidence. Both times you brought the AST instead of an argument, which is the right way to move a reviewer — and I've retracted the corresponding signal from your record. ✅ The fight-gate — taken, and it's the structural versionif count_typed_target_only_slots(def) >= 2 && chain_contains_fight(def) {with ✅ Tests drive the real entry pointBoth new tests call Next stepNothing on you. CI is still in flight on |
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — CI is terminal red on a pre-existing class guard that this PR overturns without reconciling it. The parser work itself still looks right to me, and my earlier blocker on Life at Stake stays retracted. The problem is narrower and load-bearing: #5482 pinned an invariant about this exact card, and this PR breaks it silently.
🔴 Blocker — the PR reverses a pinned exclusion from #5482, and the guard is now red
crates/engine/tests/integration/s07_malamet_castpath.rs:320 — tail_swipe_class_guards_do_not_touch_it_and_stays_red FAILED on this head (Rust tests (shard 1/2), run 29183253831):
Tail Swipe has <2 Typed TargetOnly slots (guard excludes it)
That test is not yours and it is not new. It landed on main in #5482 (9d82a1d0cc, merged 2026-07-10T07:47Z) — a day before this PR was opened (2026-07-11T15:34Z) and two days before the current head (2026-07-12T06:49Z). So this is not main drift and not a rebase artifact: the guard was already on main when you branched.
Its doc comment states the invariant in as many words:
/// Tail Swipe stays RED — the class-shape guards (c)/(d) must NOT touch it. Its
/// non-counterPumpnode is not rekeyed toParentTargetSlot, gets no
///subject_slot, and it keeps anUnimplementednode (its unparsed main-phase
/// pump gate keeps gap>0).
and it asserts the exclusion witness directly:
// Exclusion witness: only 1 TargetOnly slot (+ an Unimplemented for the second
// target), so the >=2-TargetOnly rewrite guard never fires.
assert!(target_only < 2, "Tail Swipe has <2 Typed TargetOnly slots (guard excludes it)");Your change makes Tail Swipe parse both targets as typed TargetOnly slots. Combined with chain_contains_fight — unambiguously true for Tail Swipe ("Then those creatures fight each other.") — the card now satisfies count_typed_target_only_slots(def) >= 2 && chain_contains_fight(def) and enters the very rewrite class #5482 wrote a witness to keep it out of. #5482 also deliberately scoped the rekey to PutCounter only (the guard's own words: "Pump must NOT be rekeyed to ParentTargetSlot (PutCounter-only guard)"); this PR widens it to Pump/GenericEffect.
To be clear: you may well be right. #5482's exclusion reads like a "we cannot handle this yet" placeholder, not a permanent invariant — and bringing Tail Swipe into the class is exactly what this PR is for. But overturning a two-day-old pinned invariant is a design change that has to be argued, not landed silently through a red test.
🔴 Blocker — unresolved wrong-game-result risk: does the main-phase gate survive?
Tail Swipe, verbatim from Scryfall (verified, not paraphrased):
Choose target creature you control and target creature you don't control. If you cast this spell during your main phase, the creature you control gets +1/+1 until end of turn. Then those creatures fight each other.
#5482's exclusion rested on that cast-timing clause staying Unimplemented — "its unparsed main-phase pump gate keeps gap>0" — which is what kept the card red and therefore unplayable. If your change makes the two targets parse while that condition is still dropped, Tail Swipe becomes more supported with an unconditional pump: it would get +1/+1 even when cast outside your main phase. That is a wrong game result, and it is precisely the failure #5482's witness was standing in front of.
The guard panicked on its first assertion, so CI never reached the Pump-rekey and subject_slot assertions — which means neither of us can currently see whether the condition survives.
Please show, with a test: does Effect::Pump on Tail Swipe still carry the main-phase cast condition after the rekey, or is it now unconditional? If the condition is preserved and the card is genuinely correct, say so and update #5482's witness with that reasoning. If it isn't, the gate needs to keep Tail Swipe out until the cast-timing clause parses.
✅ Clean — credited, and unchanged from my last pass
- You refuted my earlier blocker with evidence and you were right. I claimed Life at Stake would lose a chooser. I verified against
main's generatedcard-data.json: the head node there is a strayUnimplementedstub with the realChooseunderneath, so there was never a parsed chooser to lose. My finding is withdrawn; nothing about it is on you. chain_contains_fightis the right instinct — you saw that widening the rekey toPumpalone would be too broad and bounded it deliberately. That reasoning is visible in the comments and it is correct as far as it goes; it just doesn't yet account for Tail Swipe itself.- CR 701.14a (fight) and CR 611.2c grep-verify and govern.
- Both new tests drive
parse_oracle_text— the entrydatabase::synthesisactually uses — rather thanparse_effect_chainwith a fresh context. That is the path the card really walks, and it is the right choice.
Recommendation
Request changes. Two sides deliberately modified the same class-shape seam: #5482 contributes "Tail Swipe stays excluded and red", this PR contributes "the two-target fight class rekeys the buff." Don't resolve that by deleting the guard — reconcile it, and land a discriminating test from each side green on the same head:
- Decide and state whether Tail Swipe belongs in the class. If yes, update
tail_swipe_class_guards_do_not_touch_it_and_stays_redwith the justification (it is fine to change a witness; it is not fine to leave it red). - Prove the main-phase cast condition is preserved on the rekeyed
Pump— or keep Tail Swipe excluded until it parses. - Keep Malamet's existing behavior green.
The parse-diff sticky is still baseline_pending, so I have no card-level blast radius yet either; once CI is green it should render and I'll confront it against your claimed scope. Ping me when the head moves — no defect signal recorded for this round.
…een)
main's s07_malamet_castpath::tail_swipe_class_guards_do_not_touch_it_and_stays_red
pinned Tail Swipe as a known gap (< 2 target slots, Pump un-rekeyed, keeps an
Unimplemented node). This PR closes that gap, so the pin failed on the merged
head. Flip it to tail_swipe_class_keeps_both_targets_and_buffs_slot_zero: both
target slots survive, the Pump is rekeyed to ParentTargetSlot { index: 0 }, and
no Unimplemented node remains. Verified on the merged code alongside the other
five class cards.
|
Merged current
Re-verified on the merged code: all six class cards keep both target slots and rekey the buff to slot 0 ( |
matthewevans
left a comment
There was a problem hiding this comment.
Approved. Both blockers resolved, and one of them was mine — the "unconditional pump" risk I raised does not exist, and I verified that rather than taking your word for it.
✅ Blocker 2 (the cast-timing condition) — refuted, by construction
I was worried that once the second target parsed, Tail Swipe's Unimplemented node would vanish and take the main-phase gate with it, leaving an unconditional +1/+1. That premise was wrong, and main's own generated card-data.json shows why:
- TargetOnly (creature you control)
- Unimplemented name='target' ← this is the SECOND TARGET, not the cast clause
- Pump cond={"type":"CastDuringPhase","phases":["PreCombatMain","PostCombatMain"]}
- Fight
CastDuringPhase already parses on main and lives on the Pump node independently of the Unimplemented one. The two were never coupled.
And the condition survives your rewrite mechanically, via two independent guarantees:
rekey_counter_slot_in_chainassigns only*target(lower.rs) — it mutates in place and never reconstructs the node, soconditionis untouched.- The condition tail fires only for
AbilityCondition::TargetMatchesFilter { subject_slot }. Tail Swipe's isCastDuringPhase, so theif letdoesn't match and it's left alone.
The pump stays conditional. Blocker cleared.
✅ Blocker 1 (the pre-existing guard) — legitimately closed, not weakened
You inverted tail_swipe_class_guards_do_not_touch_it_and_stays_red into tail_swipe_class_keeps_both_targets_and_buffs_slot_zero. Rewriting a guard that just caught you is exactly the move that deserves scrutiny, so I checked it — and it's correct. #5482 pinned Tail Swipe as RED to record a known gap, not to assert a safety invariant. This PR closes that gap, so inverting the witness is the honest response, and the new assertions are strictly stronger.
✅ The fight gate — and a card you fixed without knowing it
Adding chain_contains_fight(def) to the gate is the fix I asked for, and it did more than bound future collateral. The parse-diff shows:
1 card · ability/PutCounter · target: `parent target slot 0` → `parent target`
Examples: Strength of the Tajuru
That reads like a regression. It isn't — it's a repair. Strength of the Tajuru is "Choose target creature, then choose another target creature for each time this spell was kicked. Put X +1/+1 counters on each of them." It has no fight, and it was only being rekeyed because the old gate matched on slot-count alone. And:
// filter.rs:3354
TargetFilter::ParentTarget => ability.targets.iter().collect(), // ALL targets
TargetFilter::ParentTargetSlot{i} => ability.targets.get(*i).into_iter()... // ONE targetSo on main, a multikicked Strength of the Tajuru put counters on only the first creature. Evicting it from a class it never belonged to restores ParentTarget = all targets — which is what "each of them" means. Narrowing the guard undid collateral that had already shipped.
Blast radius — bounded by hand
The >= 2 typed slots AND Fight gate holds a closed population. On main exactly 3 cards qualify (Duel for Dominance, Longstalk Brawl, Malamet Battle Glyph — all already rekeyed). Your sentence-bounding fix admits 3 more (Tail Swipe, Joust, Blizzard Brawl) by making their second target parse, for the 6 class cards you name. Nothing else can enter without declaring two typed targets and containing an Effect::Fight.
Parse-diff: 7 cards / 6 signatures against a fresh baseline (66dbe0c9ae), every line accounted for. All 12 checks green, including Card data (10m21s).
Merging. No quality label — this took several heads — but the pushback on my blocker was evidence-backed and right, for the second time this week. That is the correct way to handle a review you disagree with.
Fixes #4751.
Tier: Frontier
Model: claude-opus-4-8
Summary
The two-target fight class — "Choose target creature you control and target creature you don't control. [buff the you-control creature]. Then those creatures fight each other." — silently dropped its second target whenever the buff was a pump (
gets +N/+M). Tail Swipe "half worked" (only the you-control creature was targeted). Affects Tail Swipe, Joust, Blizzard Brawl (the counter siblings Malamet Battle Glyph / Longstalk Brawl / Duel for Dominance already worked). This bounds the continuous-verb scan to the current sentence and rekeys the buff to the first-declared (you-control) slot.Implementation method (required)
/engine-implementerpipeline/engine-implementerRoot cause (CR 601.2c)
starts_target_continuous_clause_lowerrecognises atarget <noun> gets/gains/has/loses …continuous clause viatake_until(" gets ")— but the scan ran across the sentence period. For these cards thegets +N/+Mverb lives in a later sentence, so the scan reached past the.and mis-classified the two-target declaration's second slot as a continuous clause. The clause splitter then bisectedtarget A and target Bat itsand, and the orphanedtarget creature you don't controlchunk fell through toEffect::unimplemented("target", …). Counter cards have no such verb, so the gap was Pump-specific.Fix
". "), so agets/gains/has/losesin a later sentence no longer pulls the two-target declaration apart (sequence.rs). Skulduggery (a genuine same-sentence two-target-continuous) still splits correctly.PutCounter-only) to the other buff shapes the class uses:Pump{Any|ParentTarget}(Tail Swipe / Joust) and a SelfRef-affectedGenericEffect{target:None}(Blizzard's "gets +N/+M and gains indestructible"). Both bind slot 0 (the first-declared you-control creature) instead of an unscoped whole-board target (lower.rs).TargetOnlyslots and contains anEffect::Fightnode (CR 701.14a) — so the widened matcher is safe by construction, not by the current pool happening not to trip it.The reciprocal "those creatures fight each other" object stays
ParentTargetby design (parse_fight_target, CR 701.14), so the fight itself needs no rekey.CR references
CR 601.2c (multiple instances of "target"), CR 611.2c (continuous-effect affected set fixed at creation), CR 701.14a (Fight).
Verification
Unimplementedand the buff bound to slot 0 — checked against the realcard-data.jsonentries (Malamet, Longstalk, Duel, Tail Swipe, Joust, Blizzard Brawl).mainand this branch; the only delta is a strayUnimplemented("you")head re-chunking — no functional chooser is created or destroyed. Pinned bylife_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub.two_target_fight_pump_keeps_both_slots_and_buffs_slot_zero,life_at_stake_keeps_choose_mechanic_and_drops_only_the_stray_you_stub. Full parser suite green; parser-combinator gate + clippy clean.