chore: update coverage stats and badges by matthewevans · Pull Request #44 · phase-rs/phase

matthewevans · 2026-04-08T22:48:59Z

Automated update of README coverage badges from latest card data.

…ion condition (CR 309.7) Fixes the CI red on PR #5722. Tomb of Horrors Adventurer's "If you've completed a dungeon, copy that spell twice instead" lost its branch to `Unimplemented(instead_condition)`, turning t60's CR 707.10c plural-retarget pin red. The CR 614 branch guard was RIGHT to refuse: it will not emit an override body as an unconditional sequel, and the condition genuinely did not lower. What it exposed is a vocabulary asymmetry underneath. `parse_inner_condition` has spelled "you've completed a dungeon" all along, producing `StaticCondition::CompletedADungeon` — but the StaticCondition -> AbilityCondition bridge listed that variant among the "no effect-resolution equivalent -> None" arms, alongside genuinely source-bound / layer-bound / cost-bound predicates. It does not belong there: CR 309.7 ("A player completes a dungeon as that dungeon card is removed from the game") is a plain CONTROLLER-state predicate, evaluable at resolution exactly like `IsMonarch` (CR 725), `IsInitiative` (CR 726) and `HasCityBlessing` (CR 702.131c) — all of which already have `AbilityCondition` spellings. The dungeon one simply never got written, so the bridge told the truth about the enum while lying about the rule. While a failed condition merely fell through silently, the asymmetry was invisible. The honest-failure guard made it a visible regression. The fix is therefore the condition lowering, NOT a weaker guard: - `AbilityCondition::CompletedDungeon { specific }` — new sibling in the controller-designation family, shaped identically to the existing `TriggerCondition::CompletedDungeon`. Kept a sibling rather than folded into the others per the categorical-boundary rule: each designation is its own CR rule section. - `game::dungeon::has_completed_dungeon` — the truth function, extracted from triggers.rs and now the SINGLE AUTHORITY. Both readings of the same printed clause (the intervening-if and the resolution-time one) delegate to it, so they cannot drift on what "completed" means. - the bridge maps `CompletedADungeon` in both directions (only the "any dungeon" reading round-trips; StaticCondition has no specific-dungeon spelling). RESULT (full-pool export, 35,396 faces): Tomb of Horrors Adventurer lowers to `CopySpell -> sub_ability CopySpell { condition: ConditionInstead { inner: CompletedDungeon } }` — a real typed BRANCH, not Unimplemented and not an unconditional sibling. 4 faces now carry the new resolution-time condition (tomb of horrors adventurer, undermountain adventurer, white plume adventurer, safana calimport cutthroat); the pre-existing TriggerCondition users are untouched. Whole engine lib green: 16462 tests, 0 failed. The #44 boundary pins all still hold (Sea Gate Stormcaller's AdditionalCostPaidInstead, Porcelain Zealot's TargetHasKeywordInstead, Torrential Gearhulk's "would"-event exile rider), and the Anax double-execution witness still passes. Note for the ledger: my "every changed face was already RED in base" claim for #44 was true on base 0eba503 and FALSE on current main — t60's work landed after my base and made this face green. Stale-base blindness; the acceptance set must be re-derived against the merge target, not the branch point.

* fix(parser): CR 614 "instead" override must be a BRANCH, never an unconditional sibling An "instead" clause is a replacement effect (CR 614.1a), and a replaced event never happens (CR 614.6): the engine does the override OR the printed effect, never both. The parser had a correct branch lowering, but `build_instead_def` returned a bare `None` when the override's CONDITION could not be lowered — so the caller fell through and re-emitted the override BODY as an ordinary, unconditional chain clause. The engine then ran both branches with the condition dropped entirely. Runtime witness (Anax, Hardened in the Forge): "Whenever Anax or another nontoken creature you control dies, create a 1/1 red Satyr … If the creature had power 4 or greater, create two of those tokens instead." lowered to `Token{count:1} -> sub_ability Token{count:2}` with `condition: null` on BOTH defs, so a 2/2 dying created THREE Satyrs. The new integration test drives the real trigger pipeline and pinned that red (left: 3, right: 1) before the fix. `try_parse_generic_instead_clause` now returns a three-way `InsteadLowering` instead of an `Option`, so "I could not lower this condition" is distinguishable from "I do not own this clause". Two boundaries keep the honest-failure path from eating working lowerings: - CR 614.1 splits the grammar by the modal "would" ("effects … watch for a particular event that WOULD happen"). A "would" condition names an EVENT and is owned by the replacement machinery — the ReplacementDefinition, the line-level replacement parser, or the structural cast-then-exile rider chain that Torrential Gearhulk / Goblin Dark-Dwellers / Mission Briefing use. Those defer (`NotOwned`) and fall through untouched. Only CR 608.2c STATE conditions are `ConditionUnlowerable`. Without this split, 68 faces regressed. - The honest failure is enforced at the LAST resort — immediately before the generic emission at the tail of the chunk loop — because handlers further down the same ladder still own "instead" clauses and recover a typed condition for them (`strip_target_keyword_instead` for Porcelain Zealot, `strip_additional_cost_conditional` for Sea Gate Stormcaller). Enforcing it early destroyed those branches. Full-pool ledger (35,396 faces, base vs after): 45 faces changed, ZERO coverage loss — every changed face was already RED. 26 were flagged by the Replacement_Instead swallow detector: the silent double-execution is replaced by an honest `Unimplemented(instead_condition)`. 19 were already Unimplemented but under a garbage marker mined from the clause text ("deal", and "wandering" — a word from the card's own NAME); they now carry the correct name and the full clause. 0 previously-clean faces became dirty; 8036 parser tests pass. Residual (filed, not claimed): of the 95 flagged faces, 30 are the CR 614.1a EVENT class (out of scope here) and 40 reach their "instead" clause by other paths — chiefly cross-LINE overrides whose condition DOES lower but which are emitted as an independent conditional ability rather than an `else_ability` alternative (Gather the Pack, See the Unwritten). Those remain a CR 614.6 double-execution and need the cross-line antecedent binding. * fix(parser): the Replacement detector must accept AddPendingETBCounters (CR 614.1c) `detect_replacement` reported 5 FALSE POSITIVES pool-wide — chocobo camp, kumano faces kakkazan, osteomancer adept, summon: fenrir, and yuna, grand summoner all DO lower their CR 614.1c "enters with ..." replacement, and the detector called it swallowed anyway. The carrier they use is `Effect::AddPendingETBCounters`, the DELAYED form of the enters-with replacement: the rider binds to a LATER cast rather than to the source permanent. Yuna's parse is fully correct — `CreateDelayedTrigger{ WhenNextEvent{SpellCast, creature} -> AddPendingETBCounters{P1P1, 2} }` — and Osteomancer Adept's graveyard-cast rider lowers to a sequential `AddPendingETBCounters` consumed as `CastFromZone` permission metadata. Neither produces a `ReplacementDefinition`, and neither was accepted. It was missed because the detector's own completeness grep (`rg '^ \w*Enter\w*'`, quoted in the comment above the carrier list) keys on variants NAMED Enter*, and this one is named Add*. It is the direct sibling of `Effect::AddPendingEntersModifications`, which the same predicate already accepts, and both are produced by the very same two parser functions (`try_parse_cast_this_way_enters_rider`, `try_parse_enters_with_additional_counters`). The carrier is probed via the tree-global typed evidence, NOT via the structural `effect_is_replacement_carrier` walk: that walk descends sub_ability / else_ability / mode_abilities only, so it cannot see a carrier nested inside an EFFECT — and Yuna's lives inside `Effect::CreateDelayedTrigger`'s inner definition. Putting it in the structural helper leaves Yuna still reported. Full-pool ledger (35,396 faces): Replacement warnings 19 -> 14. Cleared exactly the 5 predicted false positives; ZERO newly flagged. Parses byte-identical (0 faces changed) — this is an audit-side correction only. Replacement_Instead unchanged at 69; no other detector moved. 160 swallow tests pass. The regression test pins both producing grammars and carries a POSITIVE CONTROL (Undead Sprinter, a measured true positive of this detector) so the negative assertions cannot go vacuous if the detector ever dies. * fix(engine): lower "if you've completed a dungeon" to a typed resolution condition (CR 309.7) Fixes the CI red on PR phase-rs#5722. Tomb of Horrors Adventurer's "If you've completed a dungeon, copy that spell twice instead" lost its branch to `Unimplemented(instead_condition)`, turning t60's CR 707.10c plural-retarget pin red. The CR 614 branch guard was RIGHT to refuse: it will not emit an override body as an unconditional sequel, and the condition genuinely did not lower. What it exposed is a vocabulary asymmetry underneath. `parse_inner_condition` has spelled "you've completed a dungeon" all along, producing `StaticCondition::CompletedADungeon` — but the StaticCondition -> AbilityCondition bridge listed that variant among the "no effect-resolution equivalent -> None" arms, alongside genuinely source-bound / layer-bound / cost-bound predicates. It does not belong there: CR 309.7 ("A player completes a dungeon as that dungeon card is removed from the game") is a plain CONTROLLER-state predicate, evaluable at resolution exactly like `IsMonarch` (CR 725), `IsInitiative` (CR 726) and `HasCityBlessing` (CR 702.131c) — all of which already have `AbilityCondition` spellings. The dungeon one simply never got written, so the bridge told the truth about the enum while lying about the rule. While a failed condition merely fell through silently, the asymmetry was invisible. The honest-failure guard made it a visible regression. The fix is therefore the condition lowering, NOT a weaker guard: - `AbilityCondition::CompletedDungeon { specific }` — new sibling in the controller-designation family, shaped identically to the existing `TriggerCondition::CompletedDungeon`. Kept a sibling rather than folded into the others per the categorical-boundary rule: each designation is its own CR rule section. - `game::dungeon::has_completed_dungeon` — the truth function, extracted from triggers.rs and now the SINGLE AUTHORITY. Both readings of the same printed clause (the intervening-if and the resolution-time one) delegate to it, so they cannot drift on what "completed" means. - the bridge maps `CompletedADungeon` in both directions (only the "any dungeon" reading round-trips; StaticCondition has no specific-dungeon spelling). RESULT (full-pool export, 35,396 faces): Tomb of Horrors Adventurer lowers to `CopySpell -> sub_ability CopySpell { condition: ConditionInstead { inner: CompletedDungeon } }` — a real typed BRANCH, not Unimplemented and not an unconditional sibling. 4 faces now carry the new resolution-time condition (tomb of horrors adventurer, undermountain adventurer, white plume adventurer, safana calimport cutthroat); the pre-existing TriggerCondition users are untouched. Whole engine lib green: 16462 tests, 0 failed. The phase-rs#44 boundary pins all still hold (Sea Gate Stormcaller's AdditionalCostPaidInstead, Porcelain Zealot's TargetHasKeywordInstead, Torrential Gearhulk's "would"-event exile rider), and the Anax double-execution witness still passes. Note for the ledger: my "every changed face was already RED in base" claim for phase-rs#44 was true on base 0eba503 and FALSE on current main — t60's work landed after my base and made this face green. Stale-base blindness; the acceptance set must be re-derived against the merge target, not the branch point. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>

* 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>

…gn invariants (phase-rs#5816) The add-* skills lagged five invariants that have landed on main, and one of them (add-static-ability) stated the opposite of what the engine now does. - add-static-ability: continuous effects apply to OBJECTS (CR 613.1), not only battlefield permanents. New "Zone Boundary" section: the layer pass materializes keywords ONLY into the zones it also resets ({Battlefield, Hand, Stack} — layer_pass_materializes_keywords); library / graveyard / exile are owned by off_zone_characteristics and computed on demand. A grant into an unowned zone is a silent no-op that still reports as supported (the PR phase-rs#5803 defect class). - add-keyword: document the strict keyword router (PR phase-rs#5813 / task phase-rs#123). Router slots and routing classifiers must parse through parse_router_keyword_line / _list / _fragment (all-consuming); the permissive parse_granted_keyword_fragment / extract_granted_keyword_list are for EMBEDDED GRANT contexts only. A line with an unconsumed semantic tail must DECLINE, not commit. Gate G in scripts/check-parser-combinators.sh is the whole-file invariant. New router-registry checklist item (is_keyword_cost_line + ROUTER_KEYWORD_CASES set-equality test). - add-trigger: new "Purged Source" section (CR 608.2h). LKI look-back via subject_filter_matches_with_lki / source_controller_or_lki; attachments do NOT survive on the live object (CR 704.5m/n sever them — only LKISnapshot::attachments holds the pre-sever set) while attack/block history DOES (durable id-keyed ledgers on GameState). Names the non-vacuity vector: a ceased-to-exist token (CR 111.7), not a printed creature. - casting-stack-conditions + add-engine-effect: the three X carriers (GameObject::cost_x_paid = CR 107.3m cast-X, GameState::announced_source_x = CR 107.3a/d announce-X, ResolvedAbility::chosen_x = the published channel, CR 107.3i) and publish_announced_x as the single publish authority. Re-measuring X at resolution is rules-wrong (CR 107.3c), not merely late. - add-replacement-effect: cross-line "instead" lowers to a BRANCH (ClauseDisposition::ReplaceMeaning → else_ability), never two independent effects — CR 614.1a + CR 614.6, the phase-rs#44/phase-rs#79 double-execution class. - project-reference: card-export measurement hazards — export-cards rewrites the tracked parser input oracle-subtypes.json (non-idempotent), gen-card-data dirties known-tokens.toml, and the export is nondeterministic on ~20 faces (the noise floor of any whole-pool ledger). - card-test: gen-test-fixture.py --check verifies COVERAGE only (a key-set comparison), never the stored parse VALUES — a parser change can leave the integration fixture stale while the gate stays green. Also fixes stale anchors found while verifying the self-check blocks (all five now pass; two were red before this change): - add-static-ability / add-keyword cited crates/engine/src/parser/oracle_static.rs, which is a directory now (oracle_static/mod.rs, oracle_static/grammar.rs, oracle_static/keyword_grant.rs). - casting-stack-conditions cited pay_and_push and pay_ability_cost in casting.rs; they live in casting_costs.rs and costs.rs. - add-replacement-effect cited "MTG Rule 614.16" for as-enters choices; 614.16 is the token/counter-creation replacement rule. The correct citations are CR 614.1c + CR 614.12a. Every symbol grep-verified against the tree at 5f4fbcd; every CR number grep-verified against docs/MagicCompRules.txt. scripts/check-skill-doc.sh and Gate G both pass. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>

chore: update coverage stats and badges

28b3771

matthewevans enabled auto-merge (squash) April 8, 2026 22:49

matthewevans merged commit a40f2de into main Apr 8, 2026
2 checks passed

matthewevans deleted the chore/update-coverage-badges branch April 8, 2026 22:53

matthewevans mentioned this pull request Jul 13, 2026

Conditional "instead" clauses lower to unconditional chains — engine executes BOTH halves (94-face class) #5683

Closed

matthewevans mentioned this pull request Jul 14, 2026

docs(skills): bring contributor-facing skills up to the merged campaign invariants #5816

Merged

Navigation Menu

Uh oh!

Uh oh!

chore: update coverage stats and badges - #44

chore: update coverage stats and badges#44
matthewevans merged 1 commit into
mainfrom
chore/update-coverage-badges

matthewevans commented Apr 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Uh oh!

Uh oh!

Conversation

matthewevans commented Apr 8, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant