Fix greasefang (delayed triggers that trigger at the beginning of "your next end step") - #80
Closed
scherven wants to merge 12 commits into
Closed
Fix greasefang (delayed triggers that trigger at the beginning of "your next end step")#80scherven wants to merge 12 commits into
scherven wants to merge 12 commits into
Conversation
…to fix-greasefang
Member
|
Hey @scherven - apologize for the delay here. I was actually working on Greasefang last week and thought this might end up redundant. If we can resolve the conflicts I can look to see if there's anything else applicable here, such as the target referencing you mentioned. |
Member
|
Thanks so much for this fix, @scherven — and sorry it sat open for a while. 🙏 The underlying issue has since been addressed on
Verified against the current parsed card data for Greasefang, Okiba Boss: the combat trigger returns the target Vehicle from the graveyard, grants it haste, and the delayed Since the behavior you were fixing is now covered, I'm closing this as already-addressed. Genuinely appreciate you taking the time to fix it and write the integration test — please keep the contributions coming! 🎉 |
matthewevans
added a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 14, 2026
* fix(parser): a cross-LINE "instead" override must BIND or FAIL — never publish as a sibling
CR 614.15 says a self-replacement effect replaces "part or all of that spell or
ability's own effect(s)", and that its text "can be a separate ability,
particularly when preceded by an ability word". CR 614.6 says a replaced event
never happens. So an ability-word override line ("Corrupted — Counter that spell
instead if …") must be BOUND into the ability it replaces. Publishing it as a
second, independent top-level ability makes the engine perform the base effect
AND the replacement — the exact CR 614.6 violation phase-rs#44 killed intra-chain.
The cross-line binder already existed and built the right shape. Two defects fed
it a condition-less override, and it silently fell through to a sibling emission:
1. VOCABULARY ASYMMETRY. The cross-line path lowered its condition through the
nom `StaticCondition` grammar alone, which cannot express a target-relative
predicate. The intra-chain path (`build_instead_def`) ran a three-parser
ladder that can. A condition the chain lowers fine — "its controller has three
or more poison counters" — was therefore dropped at the line level. Both paths
now lower through ONE authority, `conditions::lower_instead_condition`, so the
two vocabularies cannot drift apart again.
2. The poison predicate was not parameterized. CR 122.1f DEFINES "poisoned" as
one or more poison counters, so "its controller is poisoned" (Corrupted
Resolve) and "its controller has three or more poison counters" (the Corrupted
ability word) are the same predicate at two thresholds — same subject, same
`QuantityRef::TargetControllerCounter`, same `GE`. They are now one combinator
parameterized on its threshold, not two.
And the floor: when a cross-line override cannot be conditioned at all, it is now
reported as `Effect::unimplemented` rather than published unconditioned. An
honest red, never a silent double-execution (mirrors the intra-chain
`InsteadLowering::ConditionUnlowerable` floor).
Runtime witness (Anoint with Affliction), watched RED before this commit:
"Exile target creature if it has mana value 3 or less. / Corrupted — Exile that
creature instead if its controller has three or more poison counters." Its
override published a second, condition-less `ChangeZone -> Exile { ParentTarget }`.
Targeting a mana-value-5 creature whose controller has ZERO poison counters —
both printed gates false — the engine EXILED it anyway (left: Exile, right:
Battlefield). It now stays on the battlefield, and the SHAPE test pins that the
override is bound as a `ConditionInstead` branch on a single top-level ability
rather than merely neutered into an inert sibling.
8122 parser tests pass; parser combinator gate A PASS.
* fix(parser): bind the CR 614.15 PARTIAL cross-line override ("… instead of <N>")
CR 614.15: a self-replacement effect replaces "part or all" of the ability's own
effect, and the printed grammar says WHICH. A bare trailing "instead" replaces the
whole clause. A trailing "instead of <N>" replaces only a PART — the antecedent's
quantity — and names the OLD value it displaces ("put up to two creature cards …
into your hand instead of one").
The cross-line gate recognized only the whole-clause printings, so every partial
override fell through it and was published as an INDEPENDENT top-level ability —
CR 614.6 double-execution. Two things were needed:
1. The trailing "of <N>" is a MARKER, not an operand. It is redundant with the
antecedent, which still holds the old value, so it carries nothing the branch
needs. `is_replacement_marker_tail` now accepts a bare "instead" and an
"instead of <N>" as the same replacement marker, via one combinator. Anything
else after "instead" ("instead of putting it into your hand") names a
NON-quantity part and is deliberately NOT accepted: treating it as a
whole-clause marker would silently drop the part it names.
2. The override's body cannot be lowered in isolation. Gather the Pack's
"put up to two creature cards from among the revealed cards into your hand"
parses alone to a bare `ChangeZone` — losing the reveal, the library source and
the rest-to-graveyard rider the printed Dig carries. Binding THAT as the
replacement would trade the double-execution for an effect LOSS.
`try_parse_dig_instead_alternative` is the existing antecedent-parameterized
handler for exactly this shape (it already serves Follow the Lumarets
intra-chain): it rebuilds the alternative as a full `Dig` that reuses the
preceding Dig's source and reveal-mode and swaps only what the override
changes. It is now also reached across a line boundary, handed the previous
LINE's def as its antecedent — the same relationship, one line further out.
The rebuilt alternative carries its own condition, so it flows into the existing
ability-word merge and the existing cross-line binder untouched: the binder wraps
it in `ConditionInstead` and parks the printed Dig as the `else_ability` fallback.
No new binding mechanism.
Runtime witness (Gather the Pack), watched RED before this commit — TWO top-level
abilities (left: 2, right: 1). The stray second ability was
`ChangeZone { destination: Hand, target: Creature, up_to, multi_target: 0..2 }`:
with spell mastery on, the card both dug AND offered to BOUNCE up to two creatures
off the battlefield, an effect it does not have. It is now one ability whose Dig
carries a ConditionInstead alternative Dig — keep_count 1 -> 2, with the top-five
source, the reveal and the rest-to-graveyard rider all preserved (asserted).
8122 parser tests pass; parser combinator gate A PASS.
* fix(parser): the residual cross-line overrides fail honestly — no face double-executes
Closes the class. The two commits before this one BIND the cross-line
self-replacement printings that can be bound faithfully. This one makes the
remainder fail honestly, so that after this commit NO face in the class publishes
a self-replacement override as an independent ability (CR 614.6).
The residuals are the CR 614.15 PARTIAL printings whose antecedent is not a Dig:
Nissa's Pilgrimage "search your library for up to three basic Forest cards
instead of two" -- replaces the FIRST clause's count
Talent of the Telepath "... instead of one" -- replaces a NON-FIRST clause
See the Unwritten "put two creature cards onto the battlefield instead of
one" -- Dig antecedent, but the body omits
"from among them", so the alternative
cannot be rebuilt from it
Caravan Vigil "put that card onto the battlefield instead of putting it
into your hand" -- replaces a NON-FIRST clause's
destination
These all carry a perfectly good condition, so it is tempting to hand them to the
binder. That would be WRONG, and silently so. The binder binds the FIRST emitted
clause and parks the base's tail in `else_ability`, which the runtime walks ONLY
when the swap does NOT fire: Nissa's Pilgrimage would search for three basic
Forests and then never reveal them, never put one onto the battlefield, never
shuffle. That trades a double-execution for an effect LOSS — a different silent
wrong, and the one this unit was explicitly chartered not to introduce.
A faithful bind needs clause-level antecedent selection (the override replaces the
base clause it PARALLELS, not necessarily the first) plus a tail that survives in
BOTH branches. That is a distinct mechanism; it is filed, not guessed at. Until it
exists these fail honestly: the base ability stands exactly as printed, and the
override is reported `Effect::unimplemented`.
The "would" exclusion is CR 614.1 — a replacement effect watches for an event that
WOULD happen. A "would" clause names an EVENT (CR 614.1a) and is owned by the
replacement machinery, so claiming it here would replace a WORKING rider encoding
with an honest red (this is the boundary phase-rs#44 established; 68 faces regressed when
it was missing).
Full-pool ledger (35,396 faces, base origin/main efe7409 vs after, ONE export
each, zero noise floor post-phase-rs#5745) -- MEASURED:
8 faces changed, 4 reds gained, 0 reds lost.
3 BOUND (2 -> 1 top-level abilities): Anoint with Affliction, Bring the Ending,
Gather the Pack.
4 silent double-execution -> HONEST RED: Caravan Vigil, Nissa's Pilgrimage,
See the Unwritten, Talent of the Telepath.
1 shape change, behaviour verified unchanged and pinned: Precognitive Perception.
Nothing else in the pool moved: the residual predicate did not over-fire.
Tomb of Horrors Adventurer (the phase-rs#80 regression face) is NOT in the changed set —
its "instead" is INTRA-line inside a trigger and never reaches this path.
Precognitive Perception is the face the ledger surfaced that this unit did not
predict. Widening the condition vocabulary made its Addendum line take the
strip-the-body path, moving the override's second clause ("then draw three cards")
out from under the condition the chain had been stamping on it. The engine runs an
unswapped `ConditionInstead` sub's tail when it has no `else_ability`, so an
unguarded tail would have drawn three MORE cards outside your main phase — six off
a card that draws three. Driven through the real cast pipeline in the upkeep it
draws exactly three; the assertion is pinned, and proven able to fail (asserting 6
reports left: 3, right: 6).
8122 parser tests pass; parser combinator gate A PASS.
* fix(lint): blank line before a doc-list continuation paragraph (clippy::doc_lazy_continuation)
---------
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Previously, only "the next end step" was handled. Also, allow delayed triggered abilities to correctly reference the previous target when they're linked to another triggered ability.