fix(parser): preserve "who <look-back> this turn" target clause as non-duration (Admiral Beckett Brass #4735)

[shin-core]

Summary

Fixes #4735. Admiral Beckett Brass's end-step steal — "gain control of target nonland permanent controlled by a player who was dealt combat damage by three or more Pirates this turn" — wrongly parsed with duration: UntilEndOfTurn, so the control change expired at end of turn (making the steal useless). A control-change with no stated duration is permanent (CR 611.2a).

Root cause

strip_trailing_duration mis-stripped the trailing "this turn" from the target's look-back relative clause and stamped it as the effect's duration.

The existing target_relative_clause_owns_suffix guard already prevents this for that-introduced object-property clauses — but not who-introduced player look-back clauses. Beckett uses "a player who was dealt combat damage … this turn".

Fix

Add player_lookback_relative_clause_owns_suffix — the who-sibling of the existing guard. A self-standing structural recognizer for "… who <look-back verb> … this turn" (CR 608.2i). It deliberately does not delegate to parse_that_clause_suffix (whose vocabulary is object-property clauses like "that's enchanted", not the "controlled by a player who <verb>" player shape). It fires only when the relative clause consumes through the trailing "this turn" to end-of-input, so a genuine outer duration after the clause still strips.

Scope note (deliberate coverage decision)

The full target restriction ("controlled by a player dealt combat damage by 3+ Pirates") stays an over-broad coverage gap. The object→player controller-predicate filter and the ≥N-source count axis it would require each serve only this one card (verified via Scryfall — Beckett is the sole match for both). Per the build-for-the-class rule, they are deliberately not added. The clause still parses to a valid GainControl (no Unimplemented); only the phantom duration — the actually-broken behavior — is fixed, and that fix generalizes to the whole "who <look-back> this turn" clause class.

Tests

• End-to-end: Beckett's end-step trigger lowers to GainControl with duration: None.
• Guard: strip_trailing_duration preserves the who-look-back clause (yields no duration).
• Hostile fixture (review requirement): a who-look-back clause plus a genuine trailing "until end of turn" → the outer duration still strips; the guard does not over-suppress.
• Siblings intact: the five existing quantity/relative-clause guard tests stay green.

Verification

• Full lib suite: 15,978 passed, 0 failed
• parser::oracle_effect (2,564), clippy clean.

CR 611.2a (no-duration continuous effect lasts until end of game) + CR 608.2i (look-back effects).

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request introduces a new helper function player_lookback_relative_clause_owns_suffix to prevent the parser from incorrectly stripping "this turn" as an effect duration when it belongs to a player look-back relative clause (such as in Admiral Beckett Brass). Tests are added to verify this behavior and ensure genuine outer durations are still correctly parsed. The review feedback highlights a critical issue in the alt combinator ordering where a shorter prefix is matched before a longer one, which would prevent the longer phrase from ever being parsed.

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.

[gemini-code-assist]

[HIGH] Incorrect alt combinator ordering can lead to parsing ambiguity. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:5289.

nom::branch::alt tries parsers in order and returns the first success. Placing a shorter prefix like "was dealt " before a longer match like "was dealt combat damage " can lead to incorrect parsing and is brittle. This goes against the style guide's emphasis on idiomatic Rust (line 13).

To ensure correct and robust parsing, more specific matches should always precede their prefixes in an alt block.

Suggested change

	let lookback_verb = alt((
	tag::<_, _, OracleError<'_>>("was dealt "),
	tag("were dealt "),
	tag("was dealt combat damage "),
	tag("lost life "),
	tag("gained life "),
	tag("has lost life "),
	tag("has gained life "),
	tag("controls "),
	));
	let lookback_verb = alt((
	tag::<_, _, OracleError<'_>>("was dealt combat damage "),
	tag("was dealt "),
	tag("were dealt "),
	tag("lost life "),
	tag("gained life "),
	tag("has lost life "),
	tag("has gained life "),
	tag("controls "),
	));

References

1. The code should use library idioms to their fullest. In this case, nom::branch::alt requires longer matches to be placed before shorter prefixes to avoid ambiguity. ^(link)
2. 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) to prevent combinatorial explosion and improve maintainability.

[matthewevans]

Reviewed — the architecture is sound and I'll approve+enqueue once CI is green (the branch is currently BEHIND, so it'll need an update-branch first).

What's right here:

• Right seam. strip_trailing_duration was amputating the target's look-back " this turn" and stamping a phantom Duration::UntilEndOfTurn on a control-change that is permanent. Adding player_lookback_relative_clause_owns_suffix as the "who"-introduced sibling of target_relative_clause_owns_suffix ("that"-introduced) extends the established building block rather than special-casing Beckett Brass.
• Positional discipline. The guard only fires when the relative clause consumes through " this turn" to eof, so a genuine outer duration leaves a non-empty remainder and still strips — and you proved exactly that with the hostile who_lookback_plus_genuine_outer_duration fixture. That's the discriminating test that makes this safe.
• CR citations verified. CR 611.2a (no stated duration ⇒ lasts until end of game) and CR 608.2i (look-back-in-time effects) both check out against the Comprehensive Rules and are applied correctly.
• Combinator-based, and the lone rfind carries the sanctioned allow-noncombinator word-boundary annotation (Pattern 5).

One non-blocking tidy-up (optional): in the lookback_verb alt, tag("was dealt ") precedes tag("was dealt combat damage "), so the longer arm is unreachable — the shorter prefix matches first and take_until(" this turn") swallows the remainder. Behavior is correct; the combat damage arm is just dead. Safe to drop it, but not required.

Holding only on CI.

[matthewevans]

Thanks for the drift-clear — I re-checked on the now-current head (branch is 0 commits behind main), and the coverage gate is still red. That's the discriminating result: the earlier failure was a real regression, not stale-baseline drift. Precise picture:

The duration guard itself is correct. player_lookback_relative_clause_owns_suffix rightly stops strip_trailing_duration from amputating " this turn" and stamping a phantom Duration::UntilEndOfTurn on a steal that's permanent (CR 611.2a — no stated duration). The hostile fixture (a genuine outer "until end of turn" still strips) proves the positional discipline. No objection to the guard.

But it surfaces an unmodeled target-filter clause on its own target card. The card-data coverage gate reports:

REGRESSED (coverage honesty) — 1 card:
  Admiral Beckett Brass  [Swallow:Duration_ThisTurn]

Before this PR, strip_trailing_duration swallowed " this turn" (as the wrong duration), so the parse completed and the card counted as supported-but-misparsed. Now that the guard correctly leaves " this turn" in place, nothing downstream consumes the "controlled by a player who was dealt combat damage by three or more Pirates this turn" look-back — the target parser has no filter for that player condition — so " this turn" is swallowed and the card drops supported → unsupported. The gate hard-blocks on that (coverage-regression-check.sh --fail-on-engine, exit 1).

Net: the PR improves correctness (no phantom duration) but regresses coverage, and the gate treats supported → unsupported as a block even when the prior "support" was a misparse.

To land it, the look-back target clause needs to be consumed so the card stays supported — i.e. model "player who was dealt combat damage by three or more Pirates this turn" as a target filter. The engine already tracks combat-damage-this-turn state in GameState (e.g. creature_types_dealt_combat_damage_this_turn), but there's no target-filter parser path for the "player who was dealt combat damage by ≥N of a subtype this turn" shape yet. Closing that keeps your duration fix and the card's coverage.

Happy to help scope the target-filter piece. The guard is good work — it just can't ship alone without regressing the very card it targets.

[shin-core]

Pushed a follow-up commit fixing the card-data CI failure (DIAGNOSTIC REGRESSION: swallowed-clause increased from 858 to 859).

Root cause: this PR's core fix removed the phantom Duration::UntilEndOfTurn that strip_trailing_duration was mis-injecting from the trailing "this turn" of Beckett's unparsed look-back clause (the control-change is permanent — CR 611.2a). With no Duration node left in the AST, the coverage-honesty Duration_ThisTurn swallow detector newly flagged Admiral Beckett Brass: it sees "this turn" in the Oracle text but no duration in the parse, so it reported a swallowed clause (+1 over the 858 baseline). That's the detector honestly surfacing the deferred look-back clause — but "this turn" there is a turn-history predicate on the target's controller (CR 608.2i look-back), not a swallowed effect duration.

Fix: mirror the parser-side guard inside the detector. player_lookback_relative_clause_owns_suffix (the same recognizer strip_trailing_duration already uses to decline amputating the suffix) is promoted to pub(crate) and reused as the single authority for recognizing this clause structure. The exemption is occurrence-balanced — it only exempts when the sole "this turn" is the one the look-back clause owns — so a card with a genuine earlier duration and a trailing look-back clause still fires. New regression test duration_this_turn_accepts_player_lookback_relative_clause; the existing duration_this_turn_still_fires_outside_exemption_scope guard stays green (no over-suppression).

[matthewevans]

Thanks for the follow-up — commit 3dfb5ccc is the right local fix for what it targets: mirroring player_lookback_relative_clause_owns_suffix into detect_duration_this_turn so the swallow detector stops false-flagging the deferred "this turn" as a phantom duration. It's occurrence-balanced (total_this_turn == 1), CR-annotated (608.2i + 611.2a), and the test discriminates. Good.

The open blocker is one level up, though, and this doesn't close it. The card's target is target nonland permanent controlled by a player who was dealt combat damage by three or more Pirates this turn — the controlled by a player who <look-back> clause is a controller predicate on the target, and the target parser has no bridge for it. PlayerFilter::OpponentDealtDamage (the look-back filter this needs) is only ever produced in oracle_quantity.rs (counts) and oracle_nom/condition.rs (conditions); oracle_target.rs has no controlled by a player who … path (only anaphoric controlled by that player / the chosen player).

So with just the detector exemption, the risk is a false-green: if the coverage gate now marks Admiral Beckett Brass "supported," the effect would gain control of any nonland permanent, ignoring the Pirate-combat-damage restriction on legal targets — a card that's "supported" but mis-targets is worse than one left deferred. To land this, the target needs to carry the controller look-back predicate as a TargetFilter (wiring the existing OpponentDealtDamage filter into the target-controller path), with a runtime test showing the parsed target restricts to controllers who took ≥3 Pirate combat damage this turn. Happy to review that as the next push.

[github-actions]

Parse changes introduced by this PR · 14 card(s), 15 signature(s) (baseline: main 1f875c96ecbb)

6 card(s) · ability/conjure · removed: conjure

Examples: Blood Age Muster, Dazzling Flameweaver, Goblin Influx Array (+3 more)

5 card(s) · ability/DraftFromSpellbook · added: DraftFromSpellbook

Examples: Blood Age Muster, Dazzling Flameweaver, Goblin Influx Array (+2 more)

1 card(s) · ability/Conjure · added: Conjure

Examples: Smog Smasher

1 card(s) · static/Continuous · added: Continuous (affects=combat related creature, mods=grant Lifelink)

Examples: Alms Beast

1 card(s) · static/DoubleTriggers(BattlefieldTransition(enter=true,leave=true,[Legendary,Artifact]… · added: DoubleTriggers(BattlefieldTransition(enter=true,leave=true,[Legendary,Artifact]))

Examples: Gandalf the White

1 card(s) · static/DoubleTriggers(EntersBattlefield([Artifact])) · removed: DoubleTriggers(EntersBattlefield([Artifact]))

Examples: Gandalf the White

1 card(s) · ability/DraftFromSpellbook · added: DraftFromSpellbook (kind=activated)

Examples: Tome of the Infinite

1 card(s) · ability/GainControl · field duration: until end of turn → ∅

Examples: Admiral Beckett Brass

1 card(s) · ability/GainControl · field target: permanent non-land → controlled by each opponent who was dealt combat damage this turn permanent non-land

Examples: Admiral Beckett Brass

1 card(s) · ability/PumpAll · added: PumpAll (duration=until end of turn, filter=another same name as parent target creature + not parent target, p/t=+2/+2)

Examples: Echoing Courage

1 card(s) · ability/PumpAll · added: PumpAll (duration=until end of turn, filter=another same name as parent target creature + not parent target, p/t=-2/-2)

Examples: Echoing Decay

1 card(s) · ability/PumpAll · added: PumpAll (duration=until end of turn, filter=another same name as parent target creature + not parent target, p/t=-3/-3)

Examples: Bile Blight

1 card(s) · ability/PutCounter · field counter: PlayerCount { filter: OpponentDealtDamage { kind: Any, source: None } } P1P1 → PlayerCount { filter: OpponentDealtDamage { kind: Any, source: None, min_sources: 1 } } P1P1

Examples: Furious Spinesplitter

1 card(s) · ability/conjure · removed: conjure (kind=activated)

Examples: Tome of the Infinite

1 card(s) · ability/static_structure · removed: static_structure

Examples: Alms Beast

[shin-core]

Pushed 1cbbfb35 — this builds the target controller look-back predicate you asked for, so Beckett no longer mis-targets.

The bridge (addresses the false-green)

New reusable object→player filter, the object-axis generalization of the whole PlayerFilter enum:

FilterProp::ControllerMatches { player: Box<PlayerFilter> }

This is exactly the missing object-side analogue you identified — FilterProp::ControllerChoseLabel was the single hard-coded instance of this shape; ControllerMatches generalizes it to any PlayerFilter. Runtime eval delegates to the single-authority matches_player_scope against obj.controller (CR 109.4 + CR 608.2c), so it unlocks the whole class of "controlled by a player who «predicate»" cards — not just Beckett. The parser arm in parse_ownership_or_controller_suffix recognizes "controlled by a player who was dealt combat damage by «subtype» this turn" and wires it to the existing PlayerFilter::OpponentDealtDamage { kind: CombatOnly, source: Some(Pirate) } — the look-back filter you pointed to. I also added the "lost life this turn" leaf so the sibling class is covered by the same combinator.

Beckett now lowers to GainControl with target Typed(nonland permanent) carrying ControllerMatches{OpponentDealtDamage{CombatOnly, Some(Pirate)}}, zero Unimplemented — asserted by a parse-shape test with a positive reach-guard, backed by runtime filter-eval tests (positive Pirate-combat-damage; negatives for non-Pirate source, noncombat damage, and undamaged controller).

One documented gap: the numeric "three or more"

Per a scope decision, the count threshold is consumed but not enforced — the bridge carries ≥1 combat-damage-by-a-Pirate semantics. This is a strict, correct tightening over the old "any nonland permanent" (the true ≥3 set ⊆ ≥1 set), never over-broad, and it's annotated as a known // count deferred gap rather than silently dropped. I kept the object→player bridge general (class-building) and deliberately did not add a one-card min_sources count field. If you'd prefer the count enforced before merge, it's a clean parameterization on OpponentDealtDamage (distinct-source count in quantity.rs) — happy to add it as a follow-up push.

add-engine-variant gate

ControllerMatches: Stage 1 DOES_NOT_EXIST (only ControllerChoseLabel/ControllerRef exist, neither expresses "controller satisfies an arbitrary PlayerFilter"), Stage 2 EXTEND_OK (generalization, not a 3rd sibling), Stage 3 WITHIN_SECTION (single PlayerFilter domain, single resolver). All exhaustive-match sites (filter classification ×4, ability_rw profile, coverage formatting, ai_support) got explicit arms — no wildcard fallbacks.

Gemini's alt-ordering flag

Fixed in the same push — "was dealt combat damage " now precedes its "was dealt " prefix in the duration guard's alt, so the longer branch is reachable.

Full lib + parser + coverage + swallow suites green locally.

[matthewevans]

Thanks for the substantial rework — this is a big step up from the prior head, and the core design is right.

What's strong (please keep):

• FilterProp::ControllerMatches { player: Box<PlayerFilter> } is exactly the correct seam: a general object-axis bridge into the whole PlayerFilter enum rather than a one-off DealtCombatDamageBy… variant. Any controller look-back predicate ("lost life this turn", "was dealt combat damage", …) now composes through it, and evaluation delegates to the single-authority matches_player_scope against obj.controller. That's parameterize-don't-proliferate done properly.
• The inline tests are genuinely discriminating on the axes the filter enforces: Pirate-vs-Goblin source, combat-vs-noncombat damage, damaged-vs-undamaged controller, plus serde round-trip. Good coverage.

Blocker — the "three or more" threshold is dropped, and that's the card's defining restriction.

Oracle text: "controlled by a player who was dealt combat damage by three or more Pirates this turn." The parser consumes the three or more quantifier but does not thread it into the filter, so the engine enforces "≥1 Pirate." That isn't a rare edge — a player taking damage from one or two Pirates is the modal board state, and "three or more" is the whole build-around point of the card. With the clause now fully parsing, Admiral Beckett Brass is marked supported while it can illegally steal from a player hit by a single Pirate — a false-green, one layer deeper than the unrestricted-target version this replaces. A deferral that lives only in a // count deferred comment isn't a real boundary; the type still says "any Pirate."

Path to land (either works):

1. Enforce the threshold — give OpponentDealtDamage an optional count/comparator and count distinct combat-damaging Pirate sources in matches_player_scope; or
2. If the count is genuinely out of scope for this PR, don't silently consume it — let the unhandled quantifier keep the clause from being claimed as fully supported, so the coverage report reflects what the engine actually enforces.

Everything else here is ready. (Also: CI is still pending and the branch is behind main by 9 — a rebase will want a fresh green regardless.)

[matthewevans]

@shin-core if you can land threading the quantity through on this PR I'll add both the enhancement + quality label for multipliers.

give OpponentDealtDamage an optional count/comparator and count distinct combat-damaging Pirate sources in matches_player_scope

[shin-core]

Pushed a7c9b5be — took path #1 (enforce the threshold). The "three or more" count is now threaded end-to-end, so Beckett is genuinely correct with no false-green.

What changed

• PlayerFilter::OpponentDealtDamage gains min_sources: u32 (serde default 1, skip_serializing_if = 1). A parameterization on the already-consolidated single damage variant (CR 120.9), not a new sibling — default 1 preserves every existing card's "≥1 matching source" semantics and keeps serialized card-data byte-identical (legacy roundtrip test stays green).
• opponent_dealt_damage_matches now counts DISTINCT damaging sources by DamageRecord::source_id when min_sources > 1 and requires ≥ min_sources (a single Pirate across two combat steps is ONE source, CR 120.9). The min_sources <= 1 path stays the allocation-free .any(), so no perf change for the ~all existing cards.
• Parser reads the <N> or more quantifier via the shared nom_primitives::parse_number ("three" → 3) into min_sources — so Beckett lowers to min_sources: 3. The // count deferred comment is gone; the type now says exactly what it enforces.

The discriminating tests you asked for

• controller_matches_pirate_combat_damage_positive — 3 distinct Pirates → matches.
• controller_matches_two_distinct_pirates_below_threshold_negative — 2 distinct Pirates → does NOT match (the exact modal case you flagged).
• controller_matches_same_pirate_twice_is_one_source_negative — same Pirate twice = 1 distinct source → does NOT match (distinct-source counting is correct, not per-event).
• Parser test asserts min_sources: 3 on Beckett; serde legacy roundtrip (absent → 1) green.

All destructure/construction sites (effects, ability_scan, ability_rw, the count-quantity paths) thread the field; ability_scan/ability_rw classify it as axis-free (the source read is already classified, the count is just a threshold on it).

Also: the branch is now 0 behind main, and the Gemini alt-ordering nit was fixed in the prior push (1cbbfb35). Full lib suite green locally — pushing for fresh CI.

[matthewevans]

Approving — the second-layer false-green is fully resolved. The prior head parsed the clause but dropped the "three or more Pirates" count, so the filter enforced ≥1. This head threads the count as a typed min_sources field on PlayerFilter::OpponentDealtDamage, parsed from the Oracle text with a combinator (opt(terminated(parse_number, tag(" or more ")))) rather than hardcoded — so "three or more" → min_sources = 3 and the fix generalizes to any "N or more " clause, not just this card.

Enforcement is rules-correct: min_sources <= 1 keeps the allocation-free ≥1 path for every existing card; min_sources > 1 counts DISTINCT damaging sources by source_id (CR 120.9 + CR 608.2i — one Pirate dealing combat damage across two combat steps is one source, not two) and requires ≥ min_sources. All enumeration seams (matches_player_scope, both deal_damage paths, speed_effects, ability_rw/ability_scan) delegate to the single opponent_dealt_damage_matches authority. Tests are discriminating: a parse-path test asserts Beckett emits min_sources: 3, and runtime negatives (2 distinct Pirates; the same Pirate counted twice) fail closed below the threshold. Both CR citations grep-verified. CI green, merge CLEAN. Thanks for threading the count through cleanly.