chore: update coverage stats and badges - #41
Merged
Conversation
matthewevans
enabled auto-merge (squash)
April 8, 2026 22:13
matthewevans
added a commit
to minion1227/phase
that referenced
this pull request
Jul 12, 2026
* refactor(engine): make draw delivery a frame-addressed sequence (Plan 03 / 2b) Replaces the single `GameState::pending_multi_draw` slot with `draw_sequences: DrawSequenceStack` — an ID-addressed stack of in-flight draw instructions — and collapses the draw driver's callback chain into one post-pause authority, `effects::draw::resume_draw_sequence`. Why a stack and not a slot. CR 121.2 makes a `Draw N` an instruction that performs N individual draws; CR 616.1g allows a replacement applied to one of those individual draws to itself contain another event. The old `Option<...>` slot could not represent that nesting: a substituted inner draw overwrote the outer instruction's state and the outer's remaining units were silently lost. Frames are addressed by `DrawSequenceFrameId`, never by position, so a resume proves it is driving the instruction it parked rather than whatever happens to be on top now. Behaviour-preserving. No parser code is touched, and the full-pool card-data export is byte-identical (sha 95a979814e2e4e1f07ec0f2eb0fe435bf3ef8cdd457ac6e1 e292cdab6cd45d16, unchanged). The 3 pre-rewrite pins in `draw_from_general_post_replacement.rs` and all 4 BUG-PIN assertions (phase-rs#5652, phase-rs#5658) stay green, as do 16219 engine lib tests. The draw-replacement producer census (7 rows) and zone-authority Gate B (82 hits / 62 rows) are untouched. Two correctness details that are easy to get wrong here: * CR 104.4b loop equality. `GameState`'s hand-curated `PartialEq` is the loop-detection comparator, and it already excludes identity-bearing state (`transient_continuous_effects`, `resolution_source_relatch`). Comparing the new stack structurally would fold its monotonic `next_frame_id` allocator into loop equality, so two identical board positions that had merely allocated different numbers of draw frames earlier in the game would never compare equal and loop detection would silently stop firing. `DrawSequenceStack::loop_equal` therefore compares position (who draws, units owed, units delivered) and not history — exactly what the predecessor `Option<PendingMultiDraw>` compared. * CR 608.2c, not CR 609.3. The running per-instruction total that a chained "discard that many" reads was annotated CR 609.3 ("if an effect attempts to do something impossible, it does only as much as possible"), which is a different rule. The correct citation is CR 608.2c ("later text on the card may modify the meaning of earlier text"). CR 609.3 remains correctly cited for the partial-draw case in the same file, so this is a precision fix, not the broader 609.3 sweep. Legacy saves keep working: `pending_multi_draw` still deserializes into a read-only legacy field and is migrated to a one-frame stack by `migrate_pending_multi_draw`, called from `finalize_rules_state` alongside the existing `migrate_post_replacement_continuation`. * docs(engine): cite CR 608.2c, not CR 609.3, for the multi-draw running total The two multi-draw test assertions cited CR 609.3 for the per-instruction running total that a chained "discard that many" reads. Grep-verified against docs/MagicCompRules.txt, CR 609.3 is "If an effect attempts to do something impossible, it does only as much as possible" — a different rule. The governing rule is CR 608.2c: "later text on the card may modify the meaning of earlier text." Part of the same miscitation cluster as task phase-rs#32, but scoped here to the two assertions inside the multi-draw tests this branch rewrote. Deliberately NOT a blanket sweep: CR 609.3 is *correctly* cited elsewhere in the draw path for the partial-draw case (a library with fewer cards than requested really is "do only as much as possible"), so phase-rs#32 must not be a find/replace. * refactor(engine): own the post-replacement continuation in a drain stack (Plan 03 / 2c-1) Replaces the five ownerless parallel `GameState` fields (`post_replacement_continuation` / `_source` / `_applied` / `_event_source` / `_event_target`) with one owned record, `PostReplacementDrain`, held on a `PostReplacementDrainStack`. Why. Those five values describe one thing, but they were written by three different install paths, none of which set all five, and every teardown had to remember to null each one by hand. That design has already caused a real bug: elimination.rs still carries the comment "this field was added after the teardown block below was written and was missed until this regression." "A continuation is pending" is now one fact rather than an invariant maintained across ~40 sites. The three install paths disagree about what to do when a continuation is already resident, and that disagreement is load-bearing, so it is now named rather than emergent from where the assignment happens to sit: * `stash_post_replacement_continuation` -> KeepResident (discards the incoming) * optional accept/decline -> Replace (overwrites the resident) * combat prevention riders -> Replace (overwrites the resident) Both policies are lossy, in opposite directions; CR 616.1g explicitly contemplates a replacement applying to an event contained within another, so both can discard real work. `ResidentDrainPolicy` preserves each path's behaviour exactly and makes the divergence reviewable. Turning the stack loose to actually nest is a separate, characterized commit — it changes behaviour. The subtle part: a drain and its continuation do not die together. `apply_pending_post_replacement_effect` used to take the continuation out of the slot EARLY but clear the event-context fields LATE, and that interleaving is required. The continuation must already be gone so a nested "is a continuation pending?" check taken during the dispatch does not re-drain it; but the event context must still be readable, because that is how `TargetFilter::PostReplacementSourceController` resolves "the source's controller draws cards" (CR 615.5) for Swans of Bryn Argoll. No type enforced that ordering — every caller simply had to respect it. `DrainStatus` makes it a state transition instead: `Ready(work)` -> `Dispatching` -> popped. `begin_dispatch` takes the work and marks the drain `Dispatching` WITHOUT removing it; `has_post_replacement_drain()` reports only `Ready` drains, matching what the old `.is_some()` check meant; `finish_dispatch` retires it, taking its event context with it. This is not theoretical. Sabotaging `begin_dispatch` to pop the drain instead of marking it `Dispatching` — the naive refactor — makes Swans of Bryn Argoll go red (verified: the pin at draw_from_general_post_replacement.rs:184 fails, P1 draws 0 instead of 3). The pins written before this rewrite are what caught it. Behaviour-preserving. Full-pool card-data export byte-identical (95a979814e2e4e1f07ec0f2eb0fe435bf3ef8cdd457ac6e1e292cdab6cd45d16, unchanged); 16219 engine lib tests and 2731 integration tests green, including the 3 pins and all 4 BUG-PIN assertions (phase-rs#5652, phase-rs#5658); producer census (7 rows) and zone-authority Gate B (82 hits / 62 rows) untouched. Legacy saves keep working: the five flat fields still deserialize into read-only legacy slots and are folded into a drain by `migrate_post_replacement_continuation` (which already handled the older pre-2026-05-09 split-slot shape). * feat(engine): declare a DrawReplacementScope on every Draw replacement (Plan 03 / 2a) CR 121.2 makes "draw N cards" two distinct things: the instruction, and the N individual card draws it performs. CR 121.2a says an instruction "can be modified by replacement effects that refer to the number of cards drawn. This modification occurs before considering any of the individual card draws. See rule 616.1g." A replacement definition watches exactly one of those. Until now the engine could not tell them apart. `DrawReplacementScope::{InstructionCount, IndividualDraw}` is a restriction on the DEFINITION — a sibling of `combat_scope`, `destination_zone` and `damage_target_filter`, evaluated in the same matcher — not a property of the event. It is `Some` exactly when `event` is `Draw` and `None` otherwise (`validate_draw_scope`), declared at construction and never inferred later: inferring it from the execute shape at match time is precisely the CR 121.2a conflation this axis exists to prevent. The scope is INERT in this commit. Nothing consults it yet — matching is unchanged, and the three-stage machine that will gate on it is a later, explicitly behaviour-changing commit. This is deliberate, not an oversight: the field is carried, validated and serialized first so that the producers can be audited before any behaviour depends on them. All 6 engine producers assign it explicitly: * parser: the antecedent's grammatical number IS the scope, so the existing `alt()` now CAPTURES which branch matched instead of discarding it with `value((), ...)` — "would draw a card" -> IndividualDraw, "would draw one or more cards" -> InstructionCount. * parse_conditional_draw_replacement -> InstructionCount (Quantum Riddler). * Dredge (CR 702.52a) and the runtime shields -> IndividualDraw. * the Forge `R:` loader -> IndividualDraw, stated explicitly rather than left to default so `validate_draw_scope` cannot pass on an unset scope. The gate is a cross-check, not a presence check. `draw_replacement_census.py` now requires the producer-declared `draw_scope` to EQUAL the scope it derives independently from the definition's shape. Checking only that the field is present would be vacuous — a producer that defaulted every Draw to IndividualDraw would sail through. Verified non-vacuous: giving Dredge the wrong scope makes the gate fail on Dakmor Salvage with the exact mismatch. mtgish-import waiver (granted): adding a field to `ReplacementDefinition` is E0063 at that crate's 9 struct literals, and there is no zero-touch option. The edit is strictly mechanical — 8x inert `draw_scope: None`, 1 real scope, 1 import line, no logic, no cleanup, no formatting. NOT fixed with a `Default` derive: the E0063 is the compile-time audit working, and a Default derive would silently defuse it at ~91 literals for every future field. One premise correction: the mtgish Draw scope is NOT a constant. Forge's `ReplacableEventWouldDraw` carries both a singular antecedent (`APlayerWouldDrawACard`) and count-form ones (`APlayerWouldDrawOneOrMoreCards`, `APlayerWouldDrawTwoOrMoreCards`), so the scope follows the event variant — the same rule the Oracle parser applies. Export delta, stated as separate ledgers: 35396 cards before and after, 0 added, 0 removed; exactly 51 changed, and all 51 are explained ENTIRELY by the added `draw_scope` key; 0 changed for any other reason. The intended schema bump, scoped to the Draw corpus, with no collateral parser drift. 16219 engine lib tests, 2731 integration tests, 3 pins and 4 BUG-PINs green. The producer census (7 rows) is byte-identical and needs no refresh: its population is "sites that mint a Draw definition", and this commit added scope INSIDE the existing 7 rather than adding or removing any. * docs(engine): record why `applied` is drain-owned, and that KeepResident is right by accident Two annotations on the post-replacement drain, so the shipped tree tells the truth about what it does and does not know. **`PostReplacementDrain::applied` is co-owned, not merely co-located.** It lives inside the drain on the strength of a census of every use of the field before the bundling (`git grep post_replacement_applied d1f7d05`, 7 sites): exactly ONE read — `apply_pending_post_replacement_effect`'s `std::mem::take`, which IS the drain — and every write and clear sits with a continuation install. `combat_damage.rs`'s clear is the line immediately above the continuation it is zeroing the set FOR. So the set never lives without a drain and is never read except at drain time. The reading that it has an independent lifecycle comes from looking at the *instant* of that clear rather than its *purpose*. **`ResidentDrainPolicy::KeepResident` is currently correct BY ACCIDENT**, and the obvious fix is a two-card regression. Discarding the incoming continuation looks like a plain bug; letting the drain stack nest looks like the fix. It is not. Instrumenting every collision across the full suite finds exactly two live occupants, and in both the discarded continuation is BYTE-IDENTICAL to the resident one: * Wolverine, Fierce Fighter — RemoveAllDamage{SelfRef}, stashed once per damage instance in a combat batch; * Krark's Thumb — FlipCoins{Multiply{2, EventContextAmount}}. The discard is therefore *accidentally de-duplicating*: the same replacement's continuation is stashed twice, the second is dropped, and the effect runs once — which is right. Naive nesting runs both: Wolverine heals twice, Krark's Thumb doubles twice. No pin covers it. The slot conflates two rules. CR 614.5 — a replacement "gets only one opportunity to affect an event or any modified events that may replace that event" — says the SAME definition arriving twice must be suppressed. CR 616.1g — "one replacement or prevention effect may apply to an event, and another may apply to an event contained within the first event" — says a DIFFERENT definition on a contained event must nest. A blind occupancy test satisfies the first by accident and destroys the second. The fix gates on the identity of the replacement, not the occupancy of the slot: suppress when the incoming ReplacementId is already in the event's `applied` set, nest otherwise. Those two cards are the regression tests. Tracked in issue phase-rs#5676; successor brief in .planning/architecture-remediation/P03-2C2-HANDOFF.md. No behaviour change: comments only. * fix(engine): install() must collide on has_ready(), not on residency (Plan 03 review) ## Blocker: nested mandatory post-effects were being stranded `PostReplacementDrainStack::install(KeepResident)` collided on `!drains.is_empty()`. That is the wrong predicate, and it is a regression this unit introduced. A drain stays RESIDENT while it dispatches — its event context must remain readable (CR 615.5) — but its continuation has already been taken, so it is no longer PENDING WORK. The predecessor slot expressed exactly this, for the wrong reason: it moved the continuation out of the slot before dispatching, so the slot read empty and a re-entrant stash landed. CR 616.1g: that re-entrant stash is real work. A running continuation draws; the draw is replaced; the replacement carries a mandatory post-effect (Jace, Wielder of Mysteries' win; Abundance's reveal-until). Colliding on mere residency dropped it, and `draw_through_replacement` — which gates its drain on `has_post_replacement_drain()`, i.e. on Ready drains only — then never ran it. `KeepResident` now collides on `has_ready()`, which is the predicate that method's own doc comment already described. Regression tests (`types::game_state::drain_stack_reentrancy_tests`), written before the fix and confirmed RED against the pre-fix tree: * ..._does_not_drop_a_stash_arriving_while_the_outer_drain_dispatches — the fix * ..._still_drops_a_stash_arriving_while_a_ready_drain_is_pending — the guard rail The guard rail matters: the accidental CR 614.5 dedup that Wolverine, Fierce Fighter and Krark's Thumb depend on is on a READY resident and survives untouched. This restores the old semantics exactly and does not prejudge the identity gate (phase-rs#5676). LIFO bracketing was checked rather than assumed. `finish_dispatch` pops only a last()-Dispatching drain, so an undispatched nested Ready drain would wedge its parent. Instrumented across the full suite: 0 wedges. (Probe validity: ~151 of 153 installs sat at resident=0, so drains are being popped between installs — the probe was live.) ## CR 614.12a was pasted onto plumbing 614.12a is ETB-entry timing ("a replacement effect that modifies how a permanent enters the battlefield"). It does not govern a drain lifecycle. The 11 tags this unit added are re-cited to the rules that actually govern — CR 614.6 (a replaced event's modified actions), CR 615.5 (a prevention's additional effect, which is why the event context must outlive the continuation), CR 616.1g (nesting) — or dropped where the member is plumbing, which CLAUDE.md forbids annotating. The ~52 pre-existing base tags are a separate sweep. ## Validators that claimed enforcement they did not have `DrawSequenceStack::validate` had zero call sites while its doc said "enforced by". It is now a debug_assert at `push()`, so the claim is true. `ReplacementDefinition::validate_draw_scope` had zero call sites while three comments said "checked by". Wiring it at `replacement_definition_for_id` — the single point where the engine resolves a definition it is about to consult — immediately caught 12 real violations: 11 unscoped test constructors (now fixed; Teferi's Ageless Insight gets IndividualDraw, matching its SINGULAR antecedent and the value the real card carries in the corpus, despite being a "doubler" colloquially), plus a finding that is the reason the assert is NOT wired: the committed fixture `crates/engine/tests/fixtures/integration_cards.json` predates the field and carries 7 Draw replacements with no scope (Abundance, Blood Scrivener, Jace, Laboratory Maniac, Living Conundrum, Quantum Riddler, Teferi's). Enforcing at the consult seam fails all 7 until that fixture is regenerated — a separate deliberate act, since a regen sweeps in whatever else has changed in the pool. The claim is downgraded to "checkable by" and names the real authority: the corpus census, which cross-checks every declared scope against an independently derived one. Production is unaffected — card-data.json is regenerated from the parser by CI and all 6 producers set the scope. `validate_draw_scope` now treats `DrawCards` as a draw event, agreeing with `draw_replacement_census.py`'s `event in ("Draw","DrawCards")` scan instead of contradicting it. ## Nit `begin_dispatch`'s Dispatching arm re-assigned a status `mem::replace` had already written, behind a comment asserting a constraint that does not exist. Both deleted. Export-silent: the full-pool export is byte-identical across this commit (sha 31966d51…, 35396 cards, 0 changed). 16221 lib + 2731 integration green; 3 pins and 4 BUG-PINs green; producers 7, corpus 51 + scope cross-check, Gate B 82/62. * test(engine): end-to-end witness for the has_ready() install guard The `drain_stack_reentrancy_tests` added with the fix model the bug at the `PostReplacementDrainStack` API level. The brief asked for the full path, and it was right to: an API-level test cannot show that the seam is reachable from real cards. This drives it through the real pipeline: combat damage -> Swans of Bryn Argoll prevention -> apply_pending_post_replacement_effect -> begin_dispatch (Swans' drain is now resident AND Dispatching) -> Swans' CR 615.5 rider DRAWS for the damage source's controller (P1) -> draw_through_replacement -> replace_event -> Jace, Wielder of Mysteries matches (P1's library is empty) -> apply_single_replacement stashes its MANDATORY post-effect (WinTheGame) -> install(KeepResident) <-- the seam Jace's Draw replacement is `mode: Mandatory`, `execute: WinTheGame` — read from the card data, not recalled. It is exactly the nested-mandatory-post-effect class the fix exists for. Verified RED against the pre-fix guard: reverting `has_ready()` to `!drains.is_empty()` makes the stash get dropped and P1 never wins — P1 must win: ... Dropping the re-entrant stash strands that post-effect and P1 never wins. got Priority { player: PlayerId(0) } and green with the guard restored. CR 616.1g + CR 615.5. * docs(engine): re-cite the last net-new CR 614.12a tags on continuation plumbing CR 614.12a is ETB-entry timing — "a replacement effect that modifies how a permanent enters the battlefield". It does not govern installing or dispatching a post-replacement continuation. Same class as the 11 removed from game_state.rs; I swept that file and failed to re-run the sweep across the rest of my diff. * replacement.rs `stash_post_replacement_continuation` -> CR 614.6. The continuation IS the replacement's own actions: "If an event is replaced, it never happens. A modified event occurs instead, which may in turn trigger…" The collision policy is a CR 616.1g question and is already named in the body. * replacement.rs optional accept/decline install -> CR 614.6, same reason. * engine_replacement.rs resident-top dispatch -> CR 615.5 + CR 616.1g. This one was net-neutral in the tag COUNT (20 -> 20) only because my rewrite replaced a base tag with my own — the line is mine and carried the same wrong rule, so leaving it because a counter happened to net out would have been the wrong call. Net-new CR 614.12a tags across every file this unit touched: zero. Per-file counts are now at or below base (replacement.rs 20->20, engine_replacement.rs 20->19, game_state.rs 12->12). The ~52 pre-existing base tags remain task phase-rs#41. All CR numbers grep-verified against docs/MagicCompRules.txt. Comments only; no behaviour change, export-silent. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
matthewevans
added a commit
that referenced
this pull request
Jul 12, 2026
* docs(engine): sweep CR 609.3 citations — per-site reclassification (#32) CR 609.3 is "If an effect attempts to do something impossible, it does only as much as possible." Of 158 citations, 97 genuinely implement that rule (impossible/no-op/empty-pool/do-as-much-as-possible) and are left untouched. 61 were misattributed and are reclassified per-site: - optionality ("you may" / accept-decline) -> CR 608.2d, the rule for choices an effect offers during resolution; its worked example is literally "You may sacrifice a creature". NOT CR 603.5, which is scoped to triggered abilities only. The sibling field `optional_for` already carried CR 608.2d. - "choose any number" -> CR 107.1c ("any number" = any positive number or zero). - repeat_for loop-count and iteration driving -> CR 608.2c (instructions are followed in the order written). NOT CR 107.1, which only says the game uses integers. - preceding-effect / tracked-set back-references ("the number of cards drawn this way") -> CR 608.2c. Every replacement number was grep-verified against docs/MagicCompRules.txt. Comment-only; no behavior change. * docs(engine): sweep CR 701.16a citations — per-site reclassification (#31) CR 701.16a is Investigate ("Create a Clue token"). Of 30 citations, 15 genuinely implement Investigate and are left untouched. 15 were misattributed: - sacrifice contexts (6) -> CR 701.21a ("To sacrifice a permanent, its controller moves it from the battlefield directly to its owner's graveyard"): the Forge sacrifice effect, the Sacrifice AST count field, the sacrifice AST builder's ObjectCount filter lift, the "target opponent sacrifices" controller override, Pox Plague's chain test, and Krark-Clan Ironworks' sacrifice-as-cost assert. - private "look at" contexts (9) -> CR 701.20e ("Some effects instruct a player to look at one or more cards ... shown only to the specified player"): the Dig look-step parsers, Gonti's look-then-exile-face-down idiom, and both reveal-vs-look contrast pairs, whose public side (CR 701.20a, Reveal) was already correct. Every replacement number was grep-verified against docs/MagicCompRules.txt. Comment-only; no behavior change. * docs(engine): sweep CR 614.12a citations — per-site reclassification (#41) CR 614.12a is "If a replacement effect that modifies how a permanent enters the battlefield requires a choice, that choice is made before the permanent enters." 133 of 145 citations genuinely implement it (as-enters choices, Devour co-entry, Karoo / Mox Diamond MayCost, enters-with-your-choice-of-counter, enter-as-copy, deferred-entry replay) and are left untouched. 12 were misattributed onto the post-replacement continuation machinery, which is event-type agnostic and fires for damage/life replacements where no permanent enters the battlefield: - cross-event-type substitution (Lich-class "if you would gain life, draw that many cards instead") and the generic continuation lifecycle (9) -> CR 614.6, "If an event is replaced, it never happens. A modified event occurs instead." NOT CR 615.5, which is prevention-specific (it speaks of the amount of damage that was prevented). - Swans of Bryn Argoll's prevented-damage-source stash (1) -> drop 614.12a; that site's CR 615.5 + CR 609.7 citations were already correct and sufficient. - two serde backward-compat migration sites -> annotation dropped entirely, per CLAUDE.md "do not annotate boilerplate, serialization, or plumbing." HOLD-OUT: parser/swallow_check.rs is under concurrent edit and is excluded; its one 614.12a site is CORRECT as written (MayCost is the Karoo/Mox as-enters cost). Every replacement number was grep-verified against docs/MagicCompRules.txt. Comment-only; no behavior change. --------- 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.
Automated update of README coverage badges from latest card data.