fix(parser): bind the where-X QUANTITY channel or fail honestly (CR 107.3c)#5721
Merged
Conversation
…07.3c) Sibling of #5706, which fixed the P/T channel of the same defect. A "where X is <expr>" clause DEFINES the value of X in an ability's text (CR 107.3c) — the controller does not choose it. When the parser could not represent that definition it fabricated a well-typed placeholder holding the raw Oracle text and carried on: QuantityRef::Variable { name: "<raw oracle text>" } -> game/quantity.rs:1841, the non-"X" Variable arm -> state.last_named_choice -> .unwrap_or(0) Every such node renders as a supported dynamic quantity in the coverage report while resolving to 0 — or, worse, to an unrelated number left behind by some earlier "choose a number". Porcuparrot dealt 0 damage. Abby, Merciless Soldier made 0 tokens. Arek, False Goldwarden drained for nothing. All reported as supported. #5706 fixed the P/T half of two of these functions and left the quantity half fabricating in the same functions; it also missed the CopyTokenOf where-X path 20 lines above the one it fixed. This removes the fabrication at all five sites (oracle_effect/lower.rs x2, mana.rs, token.rs x2). The where-X rewriter family is now Option-returning and routes failure through the existing unbound_where_x -> Effect::unimplemented("where_x_binding", ...) seam — no new machinery. The explicit `return None` for "the number of times ..." now surfaces as an honest red instead of being re-fabricated by the caller. Binds four expression classes whose typed home AND live resolver both already existed; zero new enum variants: "~'s intensity" / "this spell's intensity" -> Intensity { scope: Source } "the chosen number" -> ChosenNumber "the amount of mana spent to cast it/her" -> ManaSpentToCast { Total } "the amount of {S} spent to cast ..." -> ManaSpentToCast { FromSource } CR 107.4h: {S} refers to mana of any type produced by a snow source spent to pay a cost. FromSource filters payment-time source snapshots, so the snow-ness of the producing source is read at payment time. Two sibling-cluster duplicates retired in the process: - oracle_static/anthem.rs carried a private where-X intensity combinator that was never wired into the shared interpreter. It now delegates; the grammar lives once, in the shared parse_quantity_ref. - parse_mana_spent_to_cast_amount re-listed the self-anaphora pronouns instead of using parse_mana_spent_self_subject. That duplicate list is exactly why Toph, Greatest Earthbender ("...spent to cast her") fell through. Full-pool measured on 35,396 faces (base c281a86). Probe anchored on the `name` key: the export is internally tagged and PtValue::Variable collides on the variant name ({"type":"Variable","value":"*"}). Non-vacuity control: name:"X" = 647 faces. Census re-derived as 93 faces / 74 expressions (supersedes the 94/75 measured on an older base — drift, not contradiction). DEFECT LEDGER (93 faces; gains and reds separate, never netted): 13 gained binding (a real typed quantity) 80 gained honest-red (an Effect::unimplemented gap — harvest yield) 0 still fabricating Invariant: zero raw-text QuantityRef::Variable{name} survive pool-wide. Aliases preserved: "X" (647 faces), "guessed" (The Toymaker's Trap). The leaked "count" sentinel (Abby) was a real fabrication leak, not a legitimate channel instance — it is cleared by the ManaSpentToCast bind. COLLATERAL LEDGER (10 faces outside the defect set; the binds went into the shared combinator, so they also reached inline "equal to its intensity" and "for each {S} spent" contexts): 9 gains — and three were SILENT WRONG BEHAVIOR, not mere gaps: Expel the Interlopers destroyed EVERY creature regardless of power (the chosen-number filter bound was empty) Search for Glory always gained exactly 1 life (Fixed{1}) Jarsyl, Dark Age Scion had a swallowed mana-value constraint 1 regression — Piece It Together. Its Draw{1} genuinely worked before and is now absorbed into a gap. This is a GENUINE FUNCTIONAL REGRESSION, not a lying-green conversion, and it is named as such. Cause attributed by toggle probe, not inference: disabling only the intensity arm brings the Draw back. Once "~'s intensity" parses, the sentence becomes a parseable conditional-"instead" whose effect is unsupported, and the line merges into the preceding gap. The bind did not create that defect — it made one more card reach the CR 614 "instead" defect already under active repair. Absorbed on unit economics. No pinning test is added: that would cement the wrong output. It is recorded as a recovery witness on the CR 614 work instead — when branch lowering lands, the Draw must return. Tests: 6 parser witnesses + 3 runtime witnesses, each WATCHED FAILING before the fix (runtime reds produced by disabling the bind arms, then restored). The first draft of the convert witness was VACUOUS — green pre-fix, because its Draw phrasing never reached the fabricating path; it was replaced with Porcuparrot's real DealDamage shape, whose pre-fix failure prints the fabrication verbatim. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Parse changes introduced by this PR · 101 card(s), 127 signature(s) (baseline: main
|
matthewevans
added a commit
that referenced
this pull request
Jul 13, 2026
…channel field (CR 120.6) Rebase fallout: main gained 23 bare-form QuantityRef::PreviousEffectAmount sites after this unit's base (mostly #5721-era where-X test pins), all E0533 against the new struct variant. Every site is Total-channel semantics (drain gains, reveal-draw counts, for-each counter tails, repeat_for), so expression sites get channel: DamageChannel::Total explicitly and pattern sites match { .. } per the unit's own idiom. Census: CI's full E0533 list (21 engine sites) plus 2 phase-ai sites CI could not reach because the engine crate failed first; grep census cross-checked against both.
matthewevans
added a commit
to real-venus/phase
that referenced
this pull request
Jul 13, 2026
* fix(parser): bind the chain-set anaphor across its participle axis (CR 608.2c)
`QuantityRef::TrackedSetAggregate` and its live resolver both already existed,
but the parser's anaphor list recognized only the EXILE forms ("those exiled
cards", "the card exiled this way"). Every other participle fell through to an
honest red and the whole surrounding clause was dropped:
Ill-Timed Explosion "the greatest mana value among cards discarded this way"
Sword of the Ages "the total power of the creatures sacrificed this way"
Reign of the Pit (same expression)
Ruinous Intrusion "the mana value of the permanent exiled this way"
Astarion's Thirst "the power of the creature exiled this way"
Verdant Rejuvenation "the HIGHEST mana value among creatures you control"
Three axes were enumerated as phrase tables where they are independent choices,
so each new card needed a new full-string tag. They are now composed:
- extremum adjective (greatest | highest | largest) -> AggregateFunction::Max
- object property (power | toughness | mana value)
- chain-set anaphor ([the ] <noun> <participle> this way)
`parse_tracked_set_anaphor` is now the single authority for chain-set anaphora,
composed on noun x participle rather than listed as permutations. Verdant
Rejuvenation needed nothing but the word "highest" — the aggregate axis had been
a table of six full-phrase tags.
A SINGULAR "this way" referent ("the <property> of the <noun> <participle> this
way") carries no aggregate adjective, so it cannot ride the extremum/total
prefix. It gets its own gated arm, reading the same chain set — `Sum` over a
one-member set is that member's value, following the precedent already set for
the singular "the card exiled this way".
PARTICIPLE LIST IS DELIBERATELY RESTRICTED to the causes the engine actually
stamps (`ThisWayCause::{Exiled, Discarded, Sacrificed, Milled}`, via
`publish_tracked_set_with_causes`). "goaded" is EXCLUDED: `effects/goad.rs`
publishes no tracked set, so binding Havoc Eater's "the total power of creatures
goaded this way" would aggregate over an empty set and silently resolve to 0 —
a well-typed lie of exactly the class phase-rs#57 exists to kill. It stays an honest red
and is pinned by a negative-control test.
The bare PRE-nominal form ("the exiled card") stays reachable only behind an
aggregate prefix, where it is unambiguous. Offering it as a bare referent
regressed two other referents that claim the same words — the linked-exile pool
("the mana value of the exiled card") and the craft-material pool ("... used to
craft it"). Caught by the existing suite; the explicit "this way" shape is what
makes the bare-referent arm safe.
FULL-POOL LEDGER (35,396 faces, base 4e6e5aa; whole-face structural diff,
gains and reds separate, never netted):
8 faces changed
8 gained binding
0 gained honest red
0 re-shaped
Target faces: Verdant Rejuvenation, Ill-Timed Explosion, Ruinous Intrusion,
Astarion's Thirst, Reign of the Pit, Sword of the Ages (partial - its "those
creature cards" exile clause is a separate, still-open red).
COLLATERAL GAINS (2), both verified correct, neither in the unit's brief:
Fateful Tempest "the total mana value of cards milled this way"
Shadowgrange Archfiend "the greatest power among creatures sacrificed this way"
Tests: 4 parser witnesses + 4 runtime witnesses (3 positive + 1 negative
control). The runtime witnesses hand the parsed QuantityExpr to the live
resolver against a populated chain set, and were WATCHED FAILING pre-fix via a
toggle probe (participle arms disabled -> both aggregate witnesses read 0;
restored). The discarded-mana-value witness was VACUOUS in its first draft:
`add_creature` leaves the mana cost empty, so every member's mana value was 0
and the assertion would have passed for the wrong reason (0 == 0). It now
establishes the costs explicitly via `with_mana_cost`.
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(engine): give the previous-effect quantity its EXCESS channel (CR 120.10)
`DamageChannel` existed. `GameState::last_effect_excess_amount` existed, was
stamped by the damage effects, and was cleared at depth-0. The CONDITION peer
`AbilityCondition::PreviousEffectAmount { channel }` already read both channels.
Only the QUANTITY side was missing its channel:
AbilityCondition::PreviousEffectAmount -> channel: DamageChannel -> reads both
QuantityRef::PreviousEffectAmount -> BARE, no channel -> total only
`ability.rs` even documented `QuantityRef::PreviousEffectAmount` as the
total-channel twin. The excess twin was simply never built, so there was no way
to TYPE "the amount of excess damage dealt to that creature this way" — and the
whole surrounding instruction was dropped:
Goblin Negotiation "Create ... 1/1 red Goblin creature tokens equal to the
amount of excess damage dealt to that creature this way."
Hell to Pay (same shape, tapped Treasures)
Lacerate Flesh (same shape, Blood tokens)
Adds `channel: DamageChannel` to the variant (serde-elided `Total`) plus the
resolver arm. This is a parameterization, not a new leaf: a sibling
`PreviousEffectExcessAmount` would be the textbook sibling-cluster smell, and
the axis lies wholly inside CR 120 (120.6 total / 120.10 excess), so it does not
cross a rule-section boundary. The mtgish FIELD gate does not apply — it is
scoped to the `Effect` enum, and mtgish-import never names this variant
(confirmed by a clean `cargo check --workspace`, not by grep).
The compiler was the census: 20 sites across 11 files, including one in
`phase-ai` that a `crates/engine` grep would have missed. No wildcard arms were
used to absorb the new field.
"THAT EXCESS DAMAGE" IS DELIBERATELY NOT BOUND, and that is a CORRECTNESS
constraint. The bare demonstrative's antecedent is fixed by its sibling clause,
and the two readings resolve from DIFFERENT state:
Contest of Claws "If excess damage was dealt THIS WAY, ... where X is that
excess damage." -> same resolution, field is live -> Excess is right.
Fall of Cair Andros "Whenever a creature ... is dealt excess noncombat damage,
amass Orcs X, where X is that excess damage."
-> the antecedent is the TRIGGERING EVENT. The triggered
ability resolves as its own top-level chain and the depth-0
prelude CLEARS last_effect_excess_amount, so the bind reads
None -> 0.
I shipped that bind into a local export and the whole-face ledger caught it: Fall
of Cair Andros moved from `Amass { count: Variable{name:"X"} }` (the phase-rs#57 residual
fabrication, resolving to 0) to `Amass { count: PreviousEffectAmount{Excess} }` —
the SAME wrong answer wearing a more convincing type. Strictly worse. A
channel-scoped ledger would have missed it: it shows up only as "red count
unchanged, tree changed".
A context-free leaf combinator cannot separate those two readings; the
disambiguator lives one layer up. So only the shape carrying an explicit "this
way" IN THE PHRASE is bound. The bare demonstrative stays an honest red, pinned
by a negative control, and the clause-layer rebind is filed as follow-up work
(along with the Fall of Cair Andros phase-rs#57 residual leak, which hides inside the
legitimate name:"X" alias population and is therefore invisible to t57's probe).
FULL-POOL LEDGER (35,396 faces, base 4e6e5aa; whole-face structural diff,
cumulative with the chain-set anaphor commit; gains and reds separate, never
netted):
11 faces changed
11 gained binding
0 gained honest red
0 re-shaped
This commit's share: Goblin Negotiation, Hell to Pay, Lacerate Flesh.
BYTE-IDENTITY: the diff is empty outside the bound-face census, so the `Total`
serde elision leaves every pre-existing PreviousEffectAmount face unchanged.
NOT CLAIMED, and filed instead of hidden:
- Archaic's Agony is a LYING GREEN — "Exile cards ... equal to the excess damage
dealt to that creature this way" parses to ChangeZone with count: null and ZERO
Unimplemented. This bind does not reach it: the top-of-library exile mover never
consults the quantity parser for its count. Filed as a mover-path defect.
- Mephit's Enthusiasm, Molten Impact (`note`), Brazen Boarding (`conjure`),
Overclocked Electromancer (energy `get`, and turn-scoped -> wants
DamageDealtThisTurn{Excess}, a third channel) are OUTPUT-INERT: their
surrounding effect is still unimplemented, so no gain is claimed for them.
Tests: 4 runtime witnesses (2 positive, 2 negative controls). WATCHED FAILING
pre-fix via a toggle probe: with the excess arm disabled, every positive witness
fails with `Unimplemented { name: "effect_structure" }` — proving the clause was
dropped whole, not merely mis-valued. The Total-channel control passes in both
states, so a resolver that always read the excess field would not survive it.
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(engine): migrate 23 main-drift PreviousEffectAmount sites to the channel field (CR 120.6)
Rebase fallout: main gained 23 bare-form QuantityRef::PreviousEffectAmount
sites after this unit's base (mostly phase-rs#5721-era where-X test pins), all E0533
against the new struct variant. Every site is Total-channel semantics
(drain gains, reveal-draw counts, for-each counter tails, repeat_for), so
expression sites get channel: DamageChannel::Total explicitly and pattern
sites match { .. } per the unit's own idiom. Census: CI's full E0533 list
(21 engine sites) plus 2 phase-ai sites CI could not reach because the
engine crate failed first; grep census cross-checked against both.
---------
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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.
Sibling of #5706, which fixed the P/T channel of the same defect. A "where X
is " clause DEFINES the value of X in an ability's text (CR 107.3c) — the
controller does not choose it. When the parser could not represent that
definition it fabricated a well-typed placeholder holding the raw Oracle text
and carried on:
QuantityRef::Variable { name: "" }
-> game/quantity.rs:1841, the non-"X" Variable arm
-> state.last_named_choice -> .unwrap_or(0)
Every such node renders as a supported dynamic quantity in the coverage report
while resolving to 0 — or, worse, to an unrelated number left behind by some
earlier "choose a number". Porcuparrot dealt 0 damage. Abby, Merciless Soldier
made 0 tokens. Arek, False Goldwarden drained for nothing. All reported as
supported.
#5706 fixed the P/T half of two of these functions and left the quantity half
fabricating in the same functions; it also missed the CopyTokenOf where-X path
20 lines above the one it fixed. This removes the fabrication at all five sites
(oracle_effect/lower.rs x2, mana.rs, token.rs x2). The where-X rewriter family
is now Option-returning and routes failure through the existing unbound_where_x
-> Effect::unimplemented("where_x_binding", ...) seam — no new machinery. The
explicit
return Nonefor "the number of times ..." now surfaces as an honestred instead of being re-fabricated by the caller.
Binds four expression classes whose typed home AND live resolver both already
existed; zero new enum variants:
"~'s intensity" / "this spell's intensity" -> Intensity { scope: Source }
"the chosen number" -> ChosenNumber
"the amount of mana spent to cast it/her" -> ManaSpentToCast { Total }
"the amount of {S} spent to cast ..." -> ManaSpentToCast { FromSource }
CR 107.4h: {S} refers to mana of any type produced by a snow source spent
to pay a cost. FromSource filters payment-time source snapshots, so the
snow-ness of the producing source is read at payment time.
Two sibling-cluster duplicates retired in the process:
was never wired into the shared interpreter. It now delegates; the grammar
lives once, in the shared parse_quantity_ref.
of using parse_mana_spent_self_subject. That duplicate list is exactly why
Toph, Greatest Earthbender ("...spent to cast her") fell through.
Full-pool measured on 35,396 faces (base c281a86). Probe anchored on the
namekey: the export is internally tagged and PtValue::Variable collides onthe variant name ({"type":"Variable","value":"*"}). Non-vacuity control:
name:"X" = 647 faces. Census re-derived as 93 faces / 74 expressions
(supersedes the 94/75 measured on an older base — drift, not contradiction).
DEFECT LEDGER (93 faces; gains and reds separate, never netted):
13 gained binding (a real typed quantity)
80 gained honest-red (an Effect::unimplemented gap — harvest yield)
0 still fabricating
Invariant: zero raw-text QuantityRef::Variable{name} survive pool-wide.
Aliases preserved: "X" (647 faces), "guessed" (The Toymaker's Trap).
The leaked "count" sentinel (Abby) was a real fabrication leak, not a
legitimate channel instance — it is cleared by the ManaSpentToCast bind.
COLLATERAL LEDGER (10 faces outside the defect set; the binds went into the
shared combinator, so they also reached inline "equal to its intensity" and
"for each {S} spent" contexts):
9 gains — and three were SILENT WRONG BEHAVIOR, not mere gaps:
Expel the Interlopers destroyed EVERY creature regardless of power
(the chosen-number filter bound was empty)
Search for Glory always gained exactly 1 life (Fixed{1})
Jarsyl, Dark Age Scion had a swallowed mana-value constraint
1 regression — Piece It Together. Its Draw{1} genuinely worked before and
is now absorbed into a gap. This is a GENUINE FUNCTIONAL REGRESSION, not
a lying-green conversion, and it is named as such. Cause attributed by
toggle probe, not inference: disabling only the intensity arm brings the
Draw back. Once "~'s intensity" parses, the sentence becomes a parseable
conditional-"instead" whose effect is unsupported, and the line merges
into the preceding gap. The bind did not create that defect — it made one
more card reach the CR 614 "instead" defect already under active repair.
Absorbed on unit economics. No pinning test is added: that would cement
the wrong output. It is recorded as a recovery witness on the CR 614 work
instead — when branch lowering lands, the Draw must return.
Tests: 6 parser witnesses + 3 runtime witnesses, each WATCHED FAILING before
the fix (runtime reds produced by disabling the bind arms, then restored). The
first draft of the convert witness was VACUOUS — green pre-fix, because its
Draw phrasing never reached the fabricating path; it was replaced with
Porcuparrot's real DealDamage shape, whose pre-fix failure prints the
fabrication verbatim.
Co-authored-by: matthewevans matthewevans@users.noreply.github.com