ship/skip followup#5730
Merged
Merged
Conversation
matthewevans
commented
Jul 13, 2026
matthewevans
commented
Member
- fix(parser): "and you skip …" is a clause start — recovers a swallowed skip AND a swallowed delayed return
- test(parser): pin that an "instead" override is always a branch or an honest failure — never an unconditional sequel
…d skip AND a swallowed delayed return
CR 614.1b: "Effects that use the word 'skip' are replacement effects." A skip is
its own instruction, never a continuation of the conjunct before it. The
bare-and splitter already carries a subject-prefixed clause-start group whose
comment states the rule exactly — "'you [verb]' is always a clause start" — and
lists "you gain ", "you lose ", "you draw " … but not "you skip ".
Oracle (verified at source, MTGJSON AtomicCards.json; both cards identical, and
note there is NO comma before "and"):
Ivory Gargoyle / Molten Firebird — "When this creature dies, return it to the
battlefield under its owner's control at the beginning of the next end step
and you skip your next draw step."
Waterspout Elemental — "… return all other creatures to their owners' hands
and you skip your next turn."
Because the splitter left that whole sentence as ONE clause, TWO independent
clauses died together on one root cause:
1. CR 614.1b — the skip was swallowed by the leading `return …` imperative.
No `Effect::SkipNextStep` was ever produced; the replacement vanished.
2. CR 603.7a — the temporal phrase stopped being clause-FINAL.
`strip_temporal_suffix` (lower.rs:4446) only matches a temporal phrase at
the END of the clause, so with the conjoined tail behind it the phrase was
INFIX and never stripped: the delayed return collapsed into an IMMEDIATE
one and the Gargoyle came back the moment it died.
Splitting at the conjunction restores the temporal phrase to clause-final
position, so this single arm recovers both behaviors — the skip lowers, and the
return becomes the `AtNextPhase{End}` delayed trigger it always should have been.
Nothing new is needed downstream: "You skip your next draw step." already parses
to `Effect::SkipNextStep{ Controller, Step(Draw), 1, NextOccurrence }`.
RED-FIRST: the two witnesses are deliberately INDEPENDENT and were each watched
red on their own, so a fix recovering only one half could not hide behind the
other. Before: the Gargoyle is on the BATTLEFIELD the instant the dies-trigger
resolves (`left: Some(Battlefield), right: Some(Graveyard)`), and
`steps_to_skip[controller][Draw]` is 0. After: it stays in the graveyard, returns
at the next end step, and exactly one Draw-step skip is registered.
POPULATION (measured on the full-pool base export, 35,396 faces — not estimated):
the shape "temporal phrase followed by a conjoined clause" is 2 faces; the
" and you skip " conjunction is 3 (Waterspout Elemental is the third
beneficiary, and its skip is a whole TURN). The other 137 faces carrying a
temporal phrase with trailing text are the PREFIX form ("At the beginning of the
next end step, sacrifice it"), owned by `strip_temporal_prefix` — untouched.
LEDGER: full-pool delta vs the preceding commit = exactly 3 faces changed —
ivory gargoyle, molten firebird, waterspout elemental — and nothing else. The
change is confined to the acceptance set; zero collateral.
… honest failure — never an unconditional sequel
CR 608.2c + CR 614.6. Two facts must hold together for every "instead" override,
and they are asserted as an INVARIANT rather than as one expected shape:
1. the sentence BEFORE the override always survives as the chain root — the
honest-failure path is CLAUSE-scoped, and a line-level blob that swallowed
the preceding effect would silently cost a working one;
2. the override is EITHER a real `ConditionInstead` branch OR an honest
`Effect::Unimplemented` — but NEVER a bare effect with no condition, which
is the CR 614.6 double-execution defect the branch guard exists to prevent.
Piece It Together is the live witness ("Draw a card. If Piece It Together's
intensity is 4, instead take an extra turn after this one."). Its Draw{1} must
survive either way.
Deliberately NOT pinned as "the override must be Unimplemented": that is a
TRANSIENT state. The intensity bind has since landed on main, so this card now
lowers `ConditionInstead { QuantityCheck { Intensity{Source} EQ 4 } }` and takes
the BRANCH arm — the upgrade from honest-red to typed-green that the guard was
built to allow. An assertion pinned to the failure mode would have gone red on
that improvement and pressured the next reader to weaken the guard. The invariant
is true on both sides of that landing, and of every future condition-grammar one.
Exercised across all three shapes: the intensity card (branch), an unlowerable
condition (honest failure), and a plainly lowerable one (branch).
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request fixes a parsing issue where temporal suffixes followed by a conjoined "and you skip..." clause were swallowed, causing delayed-return effects to resolve immediately and skip effects to be dropped. This is resolved by adding a clause-splitting arm for "you skip " in starts_bare_and_clause_lower, supported by integration tests for Ivory Gargoyle. The review feedback points out a sibling coverage gap where other subjects (e.g., "they skip", "each opponent skips") are not handled, and suggests refactoring the parser arm using modular nom combinators to cover these variants robustly.
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.
| // clause, never fired and the delayed return collapsed into an immediate | ||
| // one. Splitting here restores the temporal phrase to clause-final | ||
| // position, so ONE arm recovers both behaviors. | ||
| value((), tag("you skip ")), |
Contributor
There was a problem hiding this comment.
[HIGH] Sibling coverage gap for "skip" clause-splitter.
While "you skip" is handled, other valid MTG subjects like "they skip", "each opponent skips", "each player skips", or "he/she/it skips" will fail to split on bare "and", causing those clauses to be swallowed.
To prevent combinatorial explosion and maintain a robust parser, avoid adding flat, redundant verbatim tags for every subject-verb combination. Instead, decompose the phrase into modular, reusable parsers for the constituent parts (subjects and verbs) and compose them using idiomatic combinators.
value(
(),
tuple((
alt((
tag("you"),
tag("they"),
tag("each opponent"),
tag("each player"),
tag("he"),
tag("she"),
tag("it"),
)),
space1,
alt((tag("skip"), tag("skips"))),
space1,
))
),References
- L2. Sibling coverage: If a parser arm or string was extended (singular, one keyword, one CR section), are plural / possessive / negated / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' variants covered? List the variants checked. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
Parse changes introduced by this PR · 3 card(s), 4 signature(s) (baseline: main
|
matthewevans
added a commit
to jaytbarimbao-collab/phase
that referenced
this pull request
Jul 13, 2026
… parity rows it exposes (CR 603.3b + CR 603.4) (phase-rs#5732) `integration_cards.json` is a cached subset of the card-data export, but it was last FULLY regenerated at b9685cc (phase-rs#5695). Nineteen parser/types PRs merged since then; every fixture touch in between was surgical (phase-rs#5672 +1, phase-rs#5679 +2, phase-rs#5727 2 entries), so the parse values silently drifted. Regenerated from the export at 8c35dc5 (oracle-gen, MTGJSON 5.3.0+20260629). Population (json deep-equality, not line counts — the file is one line): committed 2658 entries -> 2726. 70 added, 2 removed, 44 changed values. Attribution of the 44 changed (causal: exports built at phase-rs#5720 / phase-rs#5717 / phase-rs#5730 and compared, NOT shape-guessing): phase-rs#5723 (P02-U3b shared condition grammar) ...... 3 archive trap, temple of civilization, thaumaton torpedo (all gained the comparator/lhs/rhs/qty/scope condition shape) phase-rs#5721 + phase-rs#5719 (where-X quantity channel + ..... 21 restriction grammar; both merged BEFORE phase-rs#5717 — merge order != PR-number order) bellowsbreath ogre, cryptex, deadly rollick, deflecting swat, desert, dread wanderer, esquire of the king, flesh, fraying sanity, gloomlake verge, great desert hellion, gutterbones, officious interrogation, once upon a time, potioner's trove, ribald shanty, rock jockey, second little pig, shifting woodland, snuff out, starport security phase-rs#5695..phase-rs#5720 no-regen window (bloc) ........... 20 Stale already at phase-rs#5720, so attributable to the 19-PR window above the b9685cc anchor, not to any single PR: alrund god of the cosmos, animal friend, approach of the second sun, cavernous maw, fblthp the lost, from father to son, hour of revelation, increasing vengeance, jodah the unifier, mana reflection, misty salon, puca's eye, ram through, reidane god of the worthy, secrets of the key, sevinne's reclamation, temple of the dead, the dining car, unleash the flux, valgavoth terror eater The 70 added keys are new test-source card references the generator collects; phase-rs#5729 (tests-only) contributed zero parse delta, as expected. Corrected premise: 44 entries are truly stale, not 7. Six of the seven originally reported reproduce; `osteomancer adept` is NOT stale (committed == fresh). The regen turns `ordering_parity_sweep` red, so the gate's evidence rows ship ATOMICALLY with it. Both rows are population entries, not ordering regressions: the sweep skips Unimplemented-bearing triggers, so a card only enters it once its parse binds. great desert hellion -> BATCH_GENUINE_ROWS. Its LTB Draw was Unimplemented until phase-rs#5721/phase-rs#5719 bound Intensity{Source}. Each co-departing Hellion draws off its OWN intensity but discards the SHARED hand, so the second trigger discards the cards the first just drew: with intensities a != b the final hand, graveyard and library differ by order. The members are not identical functions, so commutation genuinely fails and the new prompt is the CR 603.3b choice the legacy serde walk wrongly auto-ordered (CR 603.5: each "may" is chosen on resolution). planar collapse -> DOCUMENTED_OVER_PROMPT (L8-held family). New fixture key. Upkeep ObjectCount(Creature) >= 4 intervening-if x DestroyAll + self-Sacrifice: the first copy's sweep drives the census to 0, so the sibling's CR 603.4 re-check is false and it does nothing. Monotone and self-limiting — identical siblings commute up to relabeling, so the prompt is conservative, fail-closed and rules-correct. Neither row weakens the gate: both are direction-gated over-prompts (an under-prompt is never suppressible), and both are consumed by the ledger's exact-set asserts (over_prompt_hit 18->19, batch_genuine_hit 1->2), so a misclassification still trips the STRICT PROOF-GATE. Verification: engine lib 16481/16481 pass (was 16480 + 1 red); integration 2929/2929 pass. 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
1 participant
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.