chore: update coverage stats and badges - #1
Merged
Conversation
matthewevans
enabled auto-merge (squash)
April 7, 2026 18:08
matthewevans
added a commit
to AlexD-Richardson/phase
that referenced
this pull request
Apr 27, 2026
…handle synchronous-continuation drain Two interlocking bugs in the `repeat_for` resume path surfaced during PR phase-rs#124 review of overloaded Winds of Abandon: 1. Iteration-resume path dropped sub_ability for iterations 2+. The stash site cleared `resume_ability.sub_ability = None` and the drain called `resolve_effect` directly, bypassing the chain-level wiring at line 1461 that stashes the SearchChoice continuation. For iteration 0, the put-onto-battlefield + shuffle continuation runs correctly; for iterations 1+, it never ran at all — opponents 2+ would be prompted to pick a card but their chosen card stayed in their library. Fix: keep `sub_ability` on the resumed copy and clear `repeat_for` instead (so the resume call doesn't re-enter the outer iteration loop). The drain now calls `resolve_ability_chain` (depth=1, to preserve chain-local tracked-set state) so each resumed iteration goes through the same line-1660 SearchChoice continuation wiring as iteration 0. Per-iteration parent-target rebinding still propagates correctly because `rebind_first_object_target` updates `iter_ability.targets`, which the line-1651 sub_ability propagation copies onto the continuation chain. 2. Synchronous-continuation case was unhandled. If an iteration set `pending_continuation` without changing `waiting_for`, the inner loop would increment and run the next iteration — clobbering the continuation — or, on the trailing iteration, fall through and break without re-stashing remaining iterations. Fix: detect a None→Some `pending_continuation` transition and re-stash with `next_iteration = iteration + 1` before breaking, so the outer drain runs the continuation and then re-enters this drain for the next iteration. Tests: - `repeat_for_resumed_iteration_runs_full_sub_ability_chain`: end-to-end across two distinct opponents, asserts both chosen lands land on the battlefield AND both Shuffle resolutions emit EffectResolved events. Would have caught finding phase-rs#1 directly. - `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`: exercises the synchronous-continuation case with a multi-iteration resume that completes without entering any choice state. PR phase-rs#124 review feedback (findings phase-rs#1, phase-rs#2, phase-rs#5 first/second tests).
matthewevans
added a commit
that referenced
this pull request
Apr 27, 2026
* Add Winds of Abandon * fix(overload): spell out ChangeZone field drops explicitly (no `..` rest) The overload `Effect::ChangeZone → ChangeZoneAll` arm previously used `..` to absorb every field other than `origin`/`destination`/`target`. Today's dropped fields are semantically inert for hidden-zone exile, but `..` would silently absorb any newly-added `ChangeZone` field too, meaning a future field addition could go missing in the overloaded form without compiler help. Bind every field by name and annotate the rationale for each drop. Adding a new `ChangeZone` field will now fail to compile here, forcing a deliberate decision about overload semantics. PR #124 review feedback (finding #3). * fix(engine): preserve sub_ability on resumed repeat_for iterations + handle synchronous-continuation drain Two interlocking bugs in the `repeat_for` resume path surfaced during PR #124 review of overloaded Winds of Abandon: 1. Iteration-resume path dropped sub_ability for iterations 2+. The stash site cleared `resume_ability.sub_ability = None` and the drain called `resolve_effect` directly, bypassing the chain-level wiring at line 1461 that stashes the SearchChoice continuation. For iteration 0, the put-onto-battlefield + shuffle continuation runs correctly; for iterations 1+, it never ran at all — opponents 2+ would be prompted to pick a card but their chosen card stayed in their library. Fix: keep `sub_ability` on the resumed copy and clear `repeat_for` instead (so the resume call doesn't re-enter the outer iteration loop). The drain now calls `resolve_ability_chain` (depth=1, to preserve chain-local tracked-set state) so each resumed iteration goes through the same line-1660 SearchChoice continuation wiring as iteration 0. Per-iteration parent-target rebinding still propagates correctly because `rebind_first_object_target` updates `iter_ability.targets`, which the line-1651 sub_ability propagation copies onto the continuation chain. 2. Synchronous-continuation case was unhandled. If an iteration set `pending_continuation` without changing `waiting_for`, the inner loop would increment and run the next iteration — clobbering the continuation — or, on the trailing iteration, fall through and break without re-stashing remaining iterations. Fix: detect a None→Some `pending_continuation` transition and re-stash with `next_iteration = iteration + 1` before breaking, so the outer drain runs the continuation and then re-enters this drain for the next iteration. Tests: - `repeat_for_resumed_iteration_runs_full_sub_ability_chain`: end-to-end across two distinct opponents, asserts both chosen lands land on the battlefield AND both Shuffle resolutions emit EffectResolved events. Would have caught finding #1 directly. - `drain_pending_repeat_iteration_restashes_on_synchronous_continuation`: exercises the synchronous-continuation case with a multi-iteration resume that completes without entering any choice state. PR #124 review feedback (findings #1, #2, #5 first/second tests). * fix(parser): narrow normalize_verb_token's -es rule to known predicate verbs The previous orthographic rule stripped `-es` whenever the resulting stem ended in `ch`/`sh`/`ss`/`x`/`z`. This was correct for `searches → search` but over-applied to the `-eze`/`-eeze` family, producing invented stems like `freezes → freez`, `breezes → breez`, `sneezes → sneez` — none of which match any downstream lookup, but they violate the project's "parser must not swallow" rule by silently fabricating non-existent words for unknown inputs. Narrow the rule to only strip `-es` when the resulting stem is a registered `PREDICATE_VERBS` member. Unknown verbs now pass through unchanged, and the only verbs that take the strip are ones the parser explicitly knows about. Adds a regression test that asserts neither `freezes` nor `breezes` nor `sneezes` produces an invented stem, while the registered `searches → search` case still works. PR #124 review feedback (findings #4, #5 third test). * test(engine): genuinely exercise synchronous-continuation re-stash predicate The previous version of `drain_pending_repeat_iteration_restashes_on_synchronous_continuation` constructed a Draw + sub-Draw setup that completed each iteration cleanly without ever installing a `pending_continuation` synchronously — meaning the `installed_continuation` predicate was never evaluated true. The test passed regardless of whether the predicate existed. Rewrite the test to use `ConditionInstead` with an `else_ability` and a non-Priority pre-set `waiting_for`, which exercises the line-1486 path that synchronously stashes the else branch into `pending_continuation` without changing `waiting_for`. Verified by temporarily disabling the `installed_continuation` predicate: the test now FAILS with the exact "pending_continuation overwritten before consumption" debug_assert that finding #2 describes. Asserts after one resumed iteration: - `pending_continuation` is Some (else_ability was stashed synchronously) - `pending_repeat_iteration` is re-stashed with `next_iteration = 2` - only iteration 1's parent Draw fired (1 card), proving the drain broke immediately on the synchronous-continuation transition PR #124 review feedback (finding #2 — proper test coverage). --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
7 tasks
matthewevans
added a commit
that referenced
this pull request
May 20, 2026
Skullwinder: "When this creature enters, return target card from your
graveyard to your hand, then choose an opponent. That player returns a
card from their graveyard to their hand."
The engine resolved the second card selection as the *casting player's*
choice. Per CR 608.2c (the "rules of English" make "That player" the
just-chosen opponent), CR 608.2d (the player resolving the choice
announces it), and the card's own ruling ("The chosen opponent gets to
choose which card to return from their graveyard to their hand"), the
chosen opponent makes that choice.
Root cause was a parser defect compounded by a resolver gap:
1. Parser: the trigger AST dropped "then choose an opponent" entirely.
`try_parse_choose_player_to_verb` only recognized `tag("choose a ") +
tag("player")`, and `sequence::starts_clause_text_lower` did not
include "choose " so the chunk splitter glued "then choose an
opponent" onto the preceding return-card clause. The dependent
second-sentence Bounce was then scoped to `ScopedPlayer`, which
falls back to the controller — the agency bug.
2. Resolver: non-targeted graveyard-return `Bounce` had no branch at
all. `resolved_targets` returns empty for a `Typed` filter with no
pre-selected target, so the loop ran zero times and the sub-ability
silently no-op'd even after the parser fix.
Fixes:
- `parser/oracle_effect/mod.rs::try_parse_choose_player_to_verb`:
extend the head-noun dispatch from `tag("choose a ") + tag("player")`
to `tag("choose a") + alt((player_arm, opponent_arm))`, carrying the
resulting `ChoiceType` into the emitted `Effect::Choose`. Add a
leading optional `tag("then ")` strip for chunks that bypass
`strip_leading_sequence_connector`.
- `parser/oracle_effect/mod.rs::retarget_effect_to_chosen_player`:
add an `Effect::Bounce` arm that calls a new
`rebind_owned_scope(filter, index)` helper. Bounce's recipient lives
in `FilterProp::Owned { controller }` (CR 109.4 — graveyard cards
are owned, not controlled), so the rebind tree-walks the filter
properties rather than rewriting a top-level player slot.
- `parser/oracle_effect/mod.rs` (subject-application post-pass): when
the subject phrase resolved to a `ChosenPlayer { index }`-scoped
filter ("That player" after a `Choose(Opponent)`), call
`rebind_owned_scope` on the resulting `Effect::Bounce.target` so the
predicate's possessive "their" — which `parse_target` always emits
as `Owned { ScopedPlayer }` — binds to the chosen player.
- `parser/oracle_effect/subject.rs::parse_subject_application`: add a
`ChosenPlayer` arm to the "that player" subject handler so the
cross-sentence anaphora binds to the just-chosen player, mirroring
the existing `resolve_they_pronoun` `ChosenPlayer` branch (the
"They" form Gluntch exercises; this is the "That player" form
Skullwinder exercises). Replace the local pre-existing CR 608.2k
cite with CR 608.2c — the controlling rule for the anaphor.
- `parser/oracle_effect/sequence.rs::starts_clause_text_lower`: add
"choose " to the imperative-verb prefix list (one entry in an
existing `alt()` group, no permutation expansion) so chunks split
at "..., then choose an opponent". Without this, chunk #2 stays
glued to chunk #1 and `try_parse_choose_player_to_verb` is never
invoked. Adjacent bug; fixed inline.
- `game/effects/bounce.rs::resolve`: add a non-targeted
graveyard-return branch. When `resolved_targets` is empty and the
filter carries `FilterProp::InZone { Graveyard }`, walk the filter
for a `ChosenPlayer { index }` ref (top-level controller OR nested
`Owned` filterprop), recover the concrete `PlayerId` from
`ability.chosen_players`, enumerate that player's graveyard against
the filter, and either move the sole match directly or surface a
`WaitingFor::EffectZoneChoice { player: selecting_player, .. }`
routed through `EffectKind::ChangeZone` (which the existing intake
honors for graveyard → hand). The selecting player is the chosen
opponent for chosen-scoped filters; falls back to `ability.controller`
otherwise (same-controller graveyard returns).
CR cites verified against docs/MagicCompRules.txt:
- CR 608.2c (line 2789): "The controller of the spell or ability
follows its instructions in the order written. ... read the whole
text and apply the rules of English."
- CR 608.2d (line 2791): "If an effect of a spell or ability offers
any choices ... the player announces these while applying the
effect."
- CR 109.4 (line 594): "Only objects on the stack or on the
battlefield have a controller. Objects that are neither on the
stack nor on the battlefield aren't controlled by any player."
Tests (both discriminate — confirmed by inverting the fix and watching
each fail before reverting):
- `parser::oracle_effect::tests::skullwinder_etb_parses_choose_opponent`
asserts the trigger AST is `Bounce → Choose { Opponent } → Bounce`
with `Owned { ChosenPlayer { 0 } }` in the dependent filter — no
`ScopedPlayer` anywhere in the chain.
- `tests/integration/skullwinder_chosen_opponent.rs::
skullwinder_chosen_opponent_picks_their_own_card` constructs the
dependent Bounce shape the fixed parser emits, calls
`bounce::resolve` with `chosen_players = [P1]`, and asserts the
`EffectZoneChoice.player` is P1 (the chosen opponent) — not P0
(the caster). Also asserts P0's graveyard card is NOT a candidate
(ownership-scoped filtering).
The fix covers the general "Choose(Opponent) → That player <verb>"
sentence class — Skullwinder is one instance; the parser change
unlocks the full group-politics template that prints "that player
returns/draws/discards" after a chosen-player binding.
matthewevans
pushed a commit
that referenced
this pull request
May 24, 2026
Three reviewer findings on the prior PerTurnCastLimit work for Ethersworn Canonist: FINDING #1 (MEDIUM): the conditional-subject combinator inlined `tag("each player who has cast ")` and hard-coded `ProhibitionScope::AllPlayers`, bypassing the shared `strip_casting_prohibition_subject` building block. A future card phrased as "Each opponent who has cast ..." would have reduced to zero coverage. Restructured to strip the subject prefix via the shared helper first, then nom-match `who has cast (a|an) <SUBJ> spell this turn can't cast additional <OBJ> spells`. Two class tests (`each_opponent_scope`, `you_scope`) lock in the subject axis. FINDING #2 (MEDIUM): CR citations referenced 101.2 + 604.1 ("can't" beats "can" + static enforcement), but the *authorizing rule for the casting prohibition itself* (CR 601.2 + CR 601.3a) was missing. Verified both rule numbers against docs/MagicCompRules.txt before adding. Updated citations on the combinator docstring, the caller's inline comment, and the swallow_check marker comment. LOW #4 (carry-over): swallow marker was a bare `"PerTurnCastLimit"` substring — could false-positive on the literal string appearing in a description or unrelated location. Tightened to the serde external-tag shape `"\"PerTurnCastLimit\":{"` (and same for PerTurnDrawLimit), matching the precision class of the existing `"condition":{"type":"Unrecognized"` marker on the line above. Verification: - cargo fmt --all: clean - cargo clippy --all-targets -- -D warnings: clean - cargo test -p engine: 8187 lib + 422 integration + 7 doctest, all pass - oracle-gen for "ethersworn canonist": no Unimplemented, no parse_warnings, AST unchanged from baseline Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
matthewevans
added a commit
that referenced
this pull request
May 27, 2026
* feat(engine): implement Twinning Staff copy-count replacement
Twinning Staff's activated ability ({7},{T}: copy target instant/sorcery
you control) already worked, but its replacement line — "If you would
copy a spell one or more times, instead copy it that many times plus an
additional time. You may choose new targets for the additional copy." —
fell back to Effect::Unimplemented{name:"replacement_structure"}.
Implement it as a first-class CopySpell ReplacementDefinition carrying
QuantityModification::Plus{value:1}, mirroring the token/counter doubling
family (Doubling Season, Hardened Scales). The count is applied at the
copy-count chokepoint (the repeat_for loop in effects/mod.rs) via the new
helper copy_spell::copy_count_with_replacements, because copies are
produced through that loop rather than the ProposedEvent replacement
pipeline.
Correctness (CR 707.10 + CR 614.1a):
- only copies of a *spell* are bumped, not abilities (Gogo);
- only the copying player's own Staff applies ("if YOU would copy");
- the bumped count flows into total_iterations/resume stash, so each
additional copy runs the normal per-copy retarget step.
Tests: parser (line -> CopySpell/Plus{1}) + 3 engine helper tests
(count bump, opponent-Staff ignored, ability-copies excluded).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(engine): stop Twinning Staff runaway copy loop on targeted spells
Copying a *targeted* spell with Twinning Staff exploded into dozens of
copies (in-game "stuck in a loop"). Each copy pauses on CopyRetarget; the
drain driver resumes the next iteration by feeding a single-iteration
resume ability (repeat_for cleared) back through resolve_effect. The new
CopySpell count hook re-fired on every resumed iteration, re-adding the
"+1 additional copy" bonus each time and re-expanding the loop — runaway
copies (CR 614.6: a replacement applies to the copy event once, not per
copy).
Fix: add `copy_count_finalized` to ResolvedAbility. The repeat-loop
resume stash sets it, and the CopySpell count hook skips the bonus when
it is set, so the Twinning Staff bonus is folded into total_iterations
exactly once at the initial resolution. Untargeted copies (no pause) were
already correct; this only affects the pause/resume path.
Add a regression test that copies a targeted spell with Twinning Staff,
drives each retarget pause to completion, and asserts exactly two copies
(a runaway trips the loop guard). All ResolvedAbility struct literals get
the new field; `..ResolvedAbility::new(..)` spread sites are unchanged.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* refactor(engine): address review — typed CopyCountStatus, modular plural parse, zero-copy guard
Addresses PR review findings against the project's architectural rules:
- R2 (no bool fields): replace `copy_count_finalized: bool` on ResolvedAbility
with a typed `CopyCountStatus { Pending, Finalized }` enum, which expresses
the design space and reads self-documentingly at the count hook and the
repeat-loop resume stash.
- R1 / L2 (modular combinators + plural sibling coverage): rebuild the
"additional time(s)" parse from composed nom combinators along three
independent axes — count (`an` => 1, else a number), the `additional` token,
and the singular/plural `time(s)` noun — instead of full-phrase tags. Now
"plus an additional time" and "plus N additional times" both parse; added a
plural/numbered parser test.
- L4 (edge case): guard `copy_count_with_replacements` so the bonus does not
apply when the base copy count is zero (CR 614.6 — "if you would copy a spell
one or more times" has no event to replace at zero); added a regression test.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(engine): correct Twinning Staff CR annotations to verified rules
R6 self-review of the Twinning Staff copy-count work: the "applies once,
not per copy" anti-runaway guard and the "one or more times" precondition
were both annotated CR 614.6, whose body ("a replaced event never happens")
describes neither claim.
Verified against docs/MagicCompRules.txt and corrected:
- "applies once / not invoked repeatedly per copy" -> CR 614.5 (a
replacement effect gets only one opportunity to affect an event)
- "one or more times" precondition / zero-copies guard -> CR 614.1 (a
replacement effect watches for an event that would happen)
Comment/doc-comment only; no logic change.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(engine): avoid usize->u32 narrowing in copy_count_with_replacements
Keep the running copy count as usize and widen the u32 QuantityModification
values into it (value as usize, always lossless) instead of narrowing base
(usize) to u32 up front, which could truncate on 64-bit targets. Addresses
the gemini-code-assist review on PR #1. No behavior change for realistic
copy counts; removes a latent narrowing cast.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
* fix(PR-1067): harden copy-count replacement
---------
Co-authored-by: Claude Opus 4.7 <noreply@anthropic.com>
Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
matthewevans
added a commit
that referenced
this pull request
May 27, 2026
…sconnects
Three closely-related pod draft bugs surfaced during a 2-seat draft:
1. Auto-pick by one seat advanced the round for everyone. The engine's
picks_this_round: u8 was a counter, not a seat-identity set — after
pod_size picks from any combination of seats it declared "round
complete" and passed packs, even though only the host had picked.
The other player ended the draft with 0 picks.
2. Pick timer hit 0 with nothing happening. autoPickAllPending tried
to re-pick for seats that had already picked, the engine errored,
console.error swallowed it, and the timer was never restarted.
3. A player dropped mid-draft and the host UI showed no change.
DraftSeat::Human.connected was hardcoded `true` at every construction
site and never mutated; DraftPodPage never read paused/pauseReason.
Engine (crates/draft-core):
- Introduce SeatFlags newtype (#[serde(transparent)] over Vec<bool>)
with Vec::resize-style ensure_len semantics. Two parallel fields
share it: seats_picked_this_round (replaces picks_this_round counter)
and connected_seats (new authoritative runtime connection state).
- pick_pass.rs: reject duplicate picks per round with
SeatAlreadyPickedThisRound; advance the round only when every seat
with a non-empty current_pack has picked. Lazy ensure_len on first
access handles upgrade from old-shape saves.
- Add DraftAction::SetSeatConnected + DraftDelta::SeatConnectionChanged.
Rejects bot seats with DraftError::SeatIsBot.
- Remove DraftSeat::Human.connected; view.rs sources connected from
connected_seats. Single source of truth.
- Add DraftPauseReason typed enum (PlayerDisconnected, PausedByHost,
DisconnectGraceExpired) — default PascalCase serde matching every
other enum in the file.
Server (crates/server-core):
- Drop the dead connected: i == 0 / connected: true literals.
- Mirror wrapper-side connected writes through SetSeatConnected so
the engine view reflects runtime connection state.
WASM bridge (crates/draft-wasm):
- Export set_seat_connected.
Host adapter (client/src/adapter/p2p-draft-host.ts):
- handleGuestDisconnect: void-IIFE setSeatConnected(false) + broadcast,
matching the existing IIFE pattern at lines 342/1267.
- handleReconnect: setSeatConnected(true) before getViewForSeat so the
reconnect_ack carries the updated snapshot; broadcastViews after.
- autoPickAllPending: skip seats already in picksThisRound;
broadcastViews after non-empty sweep.
- Replace raw English reason strings with DraftPauseReason variants
at lines 678, 685-686, 1255-1256, 1391.
UI (client/src/pages/DraftPodPage.tsx, components/draft/SeatStatusRing.tsx):
- Amber paused banner reads `t(pauseReason.${pauseReason})` — wire
shape = i18n key = enum variant, no boundary conversion.
- Per-seat status dot turns rose-400 on disconnect; name strikethrough.
- i18n keys in en (full) + de/es/fr/it/pl/pt (stubbed with English).
Store (client/src/stores/multiplayerDraftStore.ts):
- pauseReason: DraftPauseReason | null.
- Existing test updated for typed reason.
Tests:
- Engine (cargo test -p draft-core, 114 pass):
* pick_twice_from_same_seat_returns_error
* single_seat_cannot_force_pack_pass (Bug #1 regression)
* round_completes_only_when_all_seats_with_packs_pick
* bot_seat_satisfies_round_complete_predicate
* mid_round_resume_treats_all_seats_as_not_yet_picked
* set_seat_connected_updates_state_and_emits_delta
* set_seat_connected_out_of_range_errors
* set_seat_connected_on_bot_seat_errors
* seat_flags_resize_preserves_existing_entries
- Frontend (pnpm test --run, 1018 pass): existing draft store + adapter
tests continue to pass with the typed pauseReason.
- Server-core (cargo test -p server-core, 132 pass).
Versioning: new code reads old saves (new fields default via serde,
removed field ignored). Old code cannot read new saves (no
#[serde(default)] on the removed picks_this_round). Wire-broadcast
DraftPlayerView does not include either field, so guests on older
builds are unaffected.
Out of scope (captured for follow-up):
- Retire host-side picksThisRound: Set by exposing engine per-seat
pick status in SeatPublicView.pick_status.
- Wire-trust gate on SetSeatConnected in multiplayer server's
handle_draft_action filter.
- Remove server-core wrapper connected: Vec<bool> cache.
matthewevans
referenced
this pull request
in Erckdd/phase
May 28, 2026
…hase-rs#1266) * parser: add 'is/are returned to [possessive] hand' trigger condition Add try_parse_returned_to_hand() for bounce-trigger zone-change events (CR 603.6c + CR 603.10a). Handles possessive variants: your hand, a player's hand, its/their owner's hand, bare hand. Cards unlocked: Warped Devotion, Azorius Aethermage, Stormfront Riders, Tameshi Reality Architect. * review: shared parse_hand_possessive, merge owner into valid_card, fix CR format Address review comments on PR phase-rs#1266: 1. Gemini #1: Extract local parse_returned_hand / parse_hand_possessive into a shared module-level parse_hand_possessive() that both try_parse_put_into_hand_from and try_parse_returned_to_hand call. Returns Option<ControllerRef> for cleaner semantics. 2. Gemini phase-rs#2: Reformat doc comment to CR <number>: <description> style. 3. Codex #1: Move hand-owner constraint from valid_target (not read by match_changes_zone) to valid_card via add_controller(), so the zone-change matcher correctly filters by the bounced permanent's controller. --------- Co-authored-by: Whovencroft <6.60056e+06+Whovencroft@users.noreply.github.com>
matthewevans
added a commit
that referenced
this pull request
May 29, 2026
Add a Labeling section (bug=existed-but-broken, enhancement=new engine work, feature=larger-scoped, test=test-only, refactor) applied to every handled PR, and correct the Enqueue step: main is REVIEW_REQUIRED, so an unapproved PR is silently shed from the queue. Mandate approve -> label -> enqueue and verify via GraphQL (gh CLI stdout unreliable under rtk). docs(skill): make 'correct architectural location' the top enqueue gate Per maintainer: the #1 review check is that a fix lives at the correct architectural seam, not just that CI is green. Velocity never justifies merging technical debt — a wrong-location fix that ships is worse than no fix. Adds the right-seam question as the first Architecture Review prompt and a disqualifying enqueue-checklist gate, citing the #1251 precedent.
jonathanchang31
pushed a commit
to jonathanchang31/phase
that referenced
this pull request
Jun 1, 2026
…th (phase-rs#1748) * perf(engine): skip GameObject clone on owner-zone filter fast path (phase-rs#1747) `matches_target_filter_in_owner_zone` cloned the full GameObject on every call just to override `controller := owner` for owner-scoped (hand / library / graveyard) filter matching — allocating `name`, the `counters` HashMap, and several Vecs per object. This is hot on library scans for tutors/search effects (Diabolic Tutor, Cultivate), where the clone runs once per scanned card. When `controller == owner` — the overwhelmingly common case for objects in owner zones, where control-change effects almost never apply — the override is a no-op, so the clone is pure waste. Add a fast path that calls `filter_inner_for_object` against the borrowed object directly, skipping the clone. Behavior is identical: the override only changes the result when `controller != owner`, which still takes the clone path. Adds a regression test exercising both paths (CR 109.5 / CR 400.3 owner scoping preserved: a control-changed card in an owner zone still counts as its owner's). This is bottleneck phase-rs#1 of the six in the report; the remaining items (Arc<GameEvent> triggers, DifferentNameFrom memoization, layer-flush escalation, SBA single-scan, NameMatchesAnyPermanent battlefield-only iteration) are higher-risk refactors that warrant profiling/benchmarks before landing and are deferred to follow-ups. Closes phase-rs#1747 * fix(test): borrow-safe id binding + rustfmt for owner-zone perf test - bind CardId(state.next_object_id) before the &mut state borrow in create_object (E0502: cannot read state while mutably borrowed) - keep both assert_eq!/assert_ne! comparison operands on one line per rustfmt * fix(PR-1748): preserve owner-zone filter scoping with LKI --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
7 tasks
This was referenced Jun 3, 2026
This was referenced Jun 7, 2026
Merged
matthewevans
referenced
this pull request
in andriypolanski/phase
Jul 10, 2026
…locked/combat source restriction on damage redirection (phase-rs#5518) * fix(engine): dropped tap-gate + unblocked/combat source restriction on damage redirection (Veteran Bodyguard, Weathered Bodyguards) Veteran Bodyguard ("As long as this creature is untapped, all damage that would be dealt to you by unblocked creatures is dealt to this creature instead.") and Weathered Bodyguards (same shape, scoped to combat damage) parsed to an unconditional, unrestricted redirection — dropping the leading "as long as untapped" gate (CR 604.2), the "by unblocked creatures" source filter (CR 509.1h), and the combat-only damage scope. parse_damage_redirection_replacement now parses all three, reusing existing types verbatim: ReplacementCondition::SourceTappedState, FilterProp::Unblocked (via parse_damage_source_subject_filter falling through to parse_type_phrase's existing unblocked-combat-status recognition), and CombatDamageScope::CombatOnly (via scan_combat_scope). Renamed strip_as_long_as_prefix_for_prevention to strip_as_long_as_condition_prefix since it is now shared between the prevention and redirection parsers. Palisade Giant is deliberately excluded — its real Oracle text has no "unblocked" restriction — and has its own regression-guard test proving the exclusion is correct. Discovered but out of scope: game/replacement.rs's damage_done_applier only reads redirect_target inside the ShieldKind::Redirection branch (CR 614.9); this whole card class parses to ShieldKind::Prevention with a separate redirect_target field that branch never reads, so the "instead" creature never actually takes the redirected damage today. Filed as a follow-up (shared runtime code, wide blast radius); the new tests document this and assert life-total delta rather than damage_marked as the discriminator. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs: remove Veteran Bodyguard, Weathered Bodyguards from parser-misparse-backlog Fixed by the preceding commit. Root cause #1 (relative-clause / filter restriction on target dropped): 750 -> 748 cards; totals rebased to 4761 distinct / 4795 total appearances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
andriypolanski
referenced
this pull request
in andriypolanski/phase
Jul 11, 2026
…ng block already handles it (test-only) (phase-rs#5547) * test(engine): prove Land's Edge already parses and resolves correctly Land's Edge ("Discard a card: If the discarded card was a land card, this enchantment deals 2 damage to target player or planeswalker. Any player may activate this ability.") is listed in the parser-misparse backlog's root cause #1 (dropped relative-clause/filter restriction). Investigation found this is a stale backlog entry, not a live bug: the existing AbilityCondition::CostPaidObjectMatchesFilter building block (added 2026-05-04, previously only exercised in ConditionInstead-wrapped form by Agency Coroner/Surtland Flinger/Stormscale Anarch/Grab the Prize) already correctly handles this card's bare (non-instead) composition -- condition extraction, chunk-boundary protection for the leading "if", runtime evaluation via the cost-paid discard's LKI snapshot, and a separate PlayerFilter::All "any player may activate" mechanism. Zero production code required. Adds 2 parser unit tests locking the full parse shape (CR 602.1 + 602.1a + 602.2 + 118.1 + 608.2c + 608.2k + 400.7j) and 5 GameRunner integration tests proving the runtime behavior: discarding a land deals 2 damage, discarding a nonland deals 0 (ability still resolves), the discard snapshot binds the specifically-chosen object (not any land in hand), a non-controller can activate and pays from their own hand (CR 602.1a), and a sibling without the "any player" clause rejects non-controller activation (CR 117.3d priority-gate correctly enforced). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs: remove Land's Edge from parser-misparse-backlog Confirmed fixed (test-only, no production change) by the preceding commit. Root cause phase-rs#2 (dropped intervening-if): 606 -> 605 cards; totals rebased to 4760 distinct / 4794 total appearances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * test(engine): reduce Land's Edge PR to a single discriminating parser test Addresses matthewevans' review on phase-rs#5547: the CostPaidObjectMatchesFilter building block this PR exercises already has runtime coverage via four sibling integration tests (Agency Coroner, Surtland Flinger, Stormscale Anarch, Grab the Prize). Adding a fifth card's standalone 427-line integration test file for the same already-covered condition was duplicative coverage with real suite-runtime/maintenance cost and near-zero marginal risk reduction. The one genuinely new thing Land's Edge's shape exercises is a parser-level distinction: a BARE (non-instead) composition of CostPaidObjectMatchesFilter, versus the ConditionInstead-wrapped form all four existing sibling cards use. At runtime this is not actually a new code path -- evaluate_condition's CostPaidObjectMatchesFilter arm already fires for any ability.condition, ConditionInstead or not -- so the distinction is provable at the parser level alone. Removed: - crates/engine/tests/integration/lands_edge_discard_land_condition.rs (427 lines, 5 tests) and its main.rs mod line - lands_edge_without_any_player_clause_has_no_activator_filter (a second parser test exercising the separately-established PlayerFilter::All mechanism, not the bare-condition distinction) Kept: lands_edge_discard_land_condition_parses_as_bare_intervening_if, a single parser unit test that locks the full bare parse shape (cost, condition explicitly NOT ConditionInstead, effect, activator_filter) in one assertion block -- the "single discriminating assertion" the review asked for. Verification note: could not get a fresh cargo test run to complete locally after this reduction -- 5 consecutive attempts were killed mid-compile (a background-build contention issue this fork's ~10 concurrent worktrees have hit repeatedly; the shared WORKLIST.md cargo-lock was held by another agent throughout). This change is a pure deletion (no logic modified) of an already-green test suite; the one retained test is byte-identical to its previously-verified-passing form. CI will provide the authoritative signal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This was referenced Jul 11, 2026
lourincedaging0-commits
added a commit
to lourincedaging0-commits/phase
that referenced
this pull request
Jul 14, 2026
…tion Addresses review on phase-rs#5774: the general path must not touch continuations it cannot model. Three narrowings keep the blast radius to exactly the handled class (Estrid); every richer sibling stays exactly as before and strict-fails until its full grammar lands (CR phase-rs#1: a flagged gap beats a silent misparse). - Recognizer (try_parse_do_the_same_for_type): drop the broader "repeat this process for" family; accept ONLY a pure card-type substitution — reject any filter carrying a FilterProp predicate (Gruesome Menageries
andriypolanski
referenced
this pull request
in andriypolanski/phase
Jul 18, 2026
…r filter (Herald of Kozilek, Ugin, Urza's Filter) (phase-rs#6150) * fix(parser): scope bare color-category & historic spell-cost modifiers to their filter (Herald of Kozilek, Ugin, Urza's Filter) "Colorless spells you cast cost {N} less to cast" (Herald of Kozilek, Ugin, the Ineffable, It That Heralds the End), "Multicolored spells cost {N} less to cast" (Urza's Filter), and "Historic spells you cast cost {N} less" (Jhoira's Familiar) all parsed with `spell_filter: None` — the color-category / historic restriction was silently dropped, so the modifier (mis)applied to EVERY spell instead of only the named category. Root cause: the bare-word fallback in `parse_cost_mod_spell_type_prefix` (static_helpers.rs) hand-rolled only the five NAMED colors via `parse_named_color`. A bare "colorless"/"monocolored"/"multicolored" matched neither that nor `parse_bare_supertype_spell_filter`, so the whole filter returned `None` (the noun-bearing path — "Colorless CREATURE spells" — already produced the correct `ColorCount` via `parse_type_phrase`). Fix: route the bare-word subject through a single `parse_bare_spell_subject_filter` authority that resolves the color word via the existing `nom_filter::parse_color_property` combinator — so the color-CATEGORY axis resolves identically to the noun-bearing path (colorless → ColorCount{EQ,0}, monocolored → {EQ,1}, multicolored → {GE,2}, named color → HasColor) — plus "historic" (FilterProp::Historic) and the pre-existing bare-supertype path. This replaces the partial `parse_named_color` special-case with the complete color-property authority; it composes with the trailing mana-value qualifier (It That Heralds the End → ColorCount + Cmc). Parse-only: FilterProp::ColorCount and Historic already exist and are evaluated by the spell-cost filter path. CR 105.2 (object colors; colorless = zero colors) / CR 700.6 (historic) / CR 205.4a (supertypes) / CR 601.2f (cost determination). Tests: parser unit test over the full bare-subject axis (colorless/mono/multi/ historic + named-color & supertype regression + MV composition); runtime cast-cost differential driving parse -> cast -> cost determination — a colorless spell gets the {1} discount and a colored spell does NOT (revert-failing: before the fix the filter was None and every spell was discounted). Backlog: removed It That Heralds the End and Urza's Filter from root cause #1 (both now parse fully clean); decremented the associated counts. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(PR-6150): address parser review comments --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
4 tasks
This was referenced Jul 23, 2026
Merged
matthewevans
pushed a commit
that referenced
this pull request
Aug 2, 2026
…tion Addresses review on #5774: the general path must not touch continuations it cannot model. Three narrowings keep the blast radius to exactly the handled class (Estrid); every richer sibling stays exactly as before and strict-fails until its full grammar lands (CR #1: a flagged gap beats a silent misparse). - Recognizer (try_parse_do_the_same_for_type): drop the broader "repeat this process for" family; accept ONLY a pure card-type substitution — reject any filter carrying a FilterProp predicate (Gruesome Menageries
matthewevans
pushed a commit
that referenced
this pull request
Aug 2, 2026
…tion Addresses review on #5774: the general path must not touch continuations it cannot model. Three narrowings keep the blast radius to exactly the handled class (Estrid); every richer sibling stays exactly as before and strict-fails until its full grammar lands (CR #1: a flagged gap beats a silent misparse). - Recognizer (try_parse_do_the_same_for_type): drop the broader "repeat this process for" family; accept ONLY a pure card-type substitution — reject any filter carrying a FilterProp predicate (Gruesome Menageries
matthewevans
pushed a commit
that referenced
this pull request
Aug 2, 2026
…tion Addresses review on #5774: the general path must not touch continuations it cannot model. Three narrowings keep the blast radius to exactly the handled class (Estrid); every richer sibling stays exactly as before and strict-fails until its full grammar lands (CR #1: a flagged gap beats a silent misparse). - Recognizer (try_parse_do_the_same_for_type): drop the broader "repeat this process for" family; accept ONLY a pure card-type substitution — reject any filter carrying a FilterProp predicate (Gruesome Menageries
matthewevans
added a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 2, 2026
…se-rs#6854) * fix(parser): return Auras via "do the same for <type> cards" (Estrid) "..., then do the same for <type> cards" replicates the immediately-preceding mass zone-change for a sibling card type. Estrid, the Masked's ult — "Return all non-Aura enchantment cards from your graveyard to the battlefield, then do the same for Aura cards." — dropped the Aura return entirely (phase-rs#4779): the comma-"then do the same" tail was glued into the first clause, and the "do the same for <type>" verb was never recognized. Parser-only, no new engine variant: - split_comma_clause_boundary: treat ", then do the same for <type>" as a Then boundary. The "do the same" verb is not in the imperative-verb table, so — mirroring the villainous-choice guard directly above — the continuation was glued into the prior clause and dropped. - new try_parse_do_the_same_for_type recognizer + chunk-loop dispatch that clones the antecedent sibling effect and swaps its type filter. This is the same antecedent-clone mechanic try_parse_scoped_does_the_same uses for the player-scoped fan-out, so it emits an ordinary sibling Effect (no disposition, resolver, or scope added). Estrid now emits both returns: non-Aura enchantments, then Auras — zones and controller preserved (CR 608.2c: the antecedent action is replicated modulo the stated type substitution). Building-block level: covers the "do the same for <type>" clause class, not Estrid alone. Closes phase-rs#4779. * fix(parser): narrow "do the same for <type>" to a clean type substitution Addresses review on phase-rs#5774: the general path must not touch continuations it cannot model. Three narrowings keep the blast radius to exactly the handled class (Estrid); every richer sibling stays exactly as before and strict-fails until its full grammar lands (CR phase-rs#1: a flagged gap beats a silent misparse). - Recognizer (try_parse_do_the_same_for_type): drop the broader "repeat this process for" family; accept ONLY a pure card-type substitution — reject any filter carrying a FilterProp predicate (Gruesome Menageries * fix(parser): preserve filtered partitions in do-the-same clauses --------- Co-authored-by: Lourince Daging <lourincedaging0@gmail.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
4 tasks
4 tasks
JacobWoodson
pushed a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 4, 2026
…ment fan use it (phase-rs#6992) * fix(client): one authority for object activation; adopt it at the six deciding sites Four call sites each re-derived "may I activate this object, and what does a click do" from a different premise, so they disagreed. The attachments dialog and the command-zone emblem chip gated on a raw `<x>Actions.length > 0` bucket check with NEITHER a `WaitingFor` gate NOR a seat gate: an opponent's emblem chip was clickable from this viewer's seat, and the attachments dialog stayed interactive at DeclareBlockers. `viewmodel/cardActionChoice.ts` gains the two authorities: - `deriveActivationAffordances(waitingFor, canAct, legalActionsByObject, objects)` returns the `activatableObjectIds` / `manaTappableObjectIds` pair, applying the timing and seat gates once. - `resolveObjectActivation(actions, object, canTapForMana)` returns a typed `ObjectActivation` verdict — `none` / `dispatch` / `choose` — and owns the CR 605.1a mana/non-mana partition and the phase-rs#506 confirmation gate, so no call site inspects an action list again. `GameBoard` now derives the pair from that authority instead of an inline copy and publishes it unchanged on `BoardInteractionContext`. `PermanentCard`, `DialogAttachmentCard` and `CommandZone` consume it; `LibraryPile` is refactor-only. Tests. The authority itself is covered at the unit level (V14/V15/V18/V18b/ V19/V19b-V19e/V20c2/V20d). `GameBoardInteractionWiring.test.tsx` renders the real `<GameBoard/>` and reads both sets back out of the real provider from a child consumer, asserting CONTENTS rather than sizes — a parity test alone could not have caught an unwired provider. The two newly gated call sites get their own timing and seat arms. `BattlefieldZoneOverflow` is the second consumer of the affordance pair and its `activatableObjectIds` read was previously silent: deleting it left all 130 board tests green, so a row that turns it red ships with it. Assisted-by: ClaudeCode:claude-opus-5 * fix(client): make the attachment fan and the host click reach that authority THE BUG: with Kilo, Apogee Mind (401) enchanted by Freed from the Real (408), the engine published `legalActionsByObject["408"] = [#0, phase-rs#1]` and the fan was inert — no ring, and clicking Freed did nothing. The fan had exactly one source, an open interaction's projection, so a populated action bucket with no prompt open reached nothing at all. Freed's own `{U}: Untap` was unreachable. `AttachmentFan` gains a second mode. Mode 1 (an engine interaction owns the prompt) is byte-unchanged and still returns early, so exactly one authority can win. Mode 2 runs only when no prompt is open: the ring and the click both come from the shared authority the battlefield already uses, so the fan can never offer what the board would not. The fan is closed BEFORE the chooser opens — it is a `fixed inset-0 z-[120]` backdrop with an `onClick` catcher and `DialogHost` anchors at z-40, so a fan left mounted would paint over the modal it just opened and swallow its clicks. The host card is the fan's anchor and is never one of its picks, even when the engine publishes actions for the host too. `PermanentCard` gains the matching host click (hunk B). An attached Aura/Equipment/Fortification is its own object (CR 301.5 / CR 303.4), but its only in-place affordance is a ~22px peek rendered BELOW the host, under the 44px touch-target floor. When the host itself offers nothing and an attachment does, the click falls through to the full-card chooser. The branch is placed LAST so it can never pre-empt the host's own target / activation / undo intent, and reads the same affordance sets as the host's own ring rather than the raw bucket. Its selection is UNCONDITIONAL, deliberately unlike the plain-click fallback which toggles: a toggle would strand the fan open over a host that just lost its ring and its attachment expansion. Tests. `AttachmentFan.test.tsx` covers mode 2 including a multi-authority hostile fixture where a prompt and a bucket are live at once. `abilityChoiceConsumerWiring.test.tsx` drives the producer and observes the real consumer — `DialogHost`'s overlay — so the store hand-off is measured rather than assumed; both its assertions are `expect.soft` so neither can pre-empt the other, and its third arm exists because the first two are blind to an always-true `selectable`. `PermanentCard.test.tsx` pins the branch ORDER with the actionable attachment held fixed across every arm. Assisted-by: ClaudeCode:claude-opus-5 * fix(client): give the activation authority its whole gate, its own id, and an exhaustive verdict Review round on phase-rs#6992. Five fixes, all frontend, all routed through the same two authorities the PR introduced — no new decision site. 1. Exhaustive `switch` + `never` at all FOUR `resolveObjectActivation` consumers (AttachmentFan, PermanentCard, DialogAttachmentCard, CommandZone). Only AttachmentFan's bare `else` errored on a new union variant; the other three compiled clean and silently dropped it. Behaviour is unchanged — `kind: "none"` still does nothing (it is reachable only through the render->click staleness window), and the fan's `close()` stays inside the two acting arms. CLAUDE.md: exhaustive match, no fallback default. 2. `resolveObjectActivation` took only the mana bit of a two-bit gate, so the non-mana partition was merged unconditionally and a cost-payment prompt offered activations the board itself refuses. It now takes the whole `ActivationAffordances` plus the object id and drops the non-mana partition when the activation ring is closed (CR 113.3b), mirroring the existing mana drop. Taking the pair rather than two loose booleans makes "half the gate" unrepresentable at a call site. 3. Sibling sweep: `CommanderCardZone` still gated `canCast`/`canNinjutsu` on the raw bucket with neither a `WaitingFor` nor a seat gate — the exact predicate this PR replaced at `CommandZone.EmblemCard`, in a component `PlayerArea` renders for every seat. Pre-existing, not introduced. 4. Grouped-emblem activation identity (maintainer review, [MED]; CodeRabbit comment 3). `GroupedEmblem` kept a representative plus a tally, so an action the engine published against a non-representative member was unreachable. It now retains every member; the chip acts on the member the shared affordance sets name, and the count badge is that list's length. This changes which id the authority is asked about, not who decides. 5. A `PermanentCard` fixture seeded a mana-only affordance pair while omitting `is_mana_ability` on the abilities — a state the engine cannot emit, since the deriver and the resolver classify through the same `isManaObjectAction`. Fix 2 surfaced it; the flags make the row exercise the mana partition. Two-sided controls, each flipping its own named assertion: the union-variant arms (1 error unfixed / 4 errors fixed / 0 with the real union); `if (true)` and `if (false)` on the activation ring; raw-bucket-restore and constant-false on the commander flags; representative-only and always-last-member on the emblem group. Assisted-by: ClaudeCode:claude-opus-5
matthewevans
referenced
this pull request
in nishu-builder/phase
Aug 5, 2026
* fix(parser): compose the cast-type gate axes, and enforce it at runtime (Epic Experiment) Closes phase-rs#6960. `parse_cast_type_disjunction` handled exactly one shape: `" or "` between two bare core types, with an optional article and a required trailing "spell(s)"/ "card(s)". Everything else in the "cast from among them" class kept a bare `TargetFilter::ExiledBySource` with no card-type leg, so any card type could be cast from the exiled set. Rewritten as a per-axis composed grammar: opt(quantifier) opt(article) leg (sep leg)* head_noun reusing `oracle_nom::target::parse_type_filter_word` for the leg alphabet (core types AND subtypes) and mirroring `oracle_nom/enchant.rs` for the separator/list pair. `and`, `or`, `and/or` and serial commas all lower to `TypeFilter::AnyOf`: per CR 205.2b conjunction is a property of ADJACENT type words ("artifact creature"), while a connector enumerates alternatives. A literal `And` would be a total no-op — no card carries both Instant and Sorcery. Seven cards gain the gate they were missing: Epic Experiment, Ral Leyline Prodigy, Kylox "instant and/or sorcery spells" Collected Conjuring "up to two sorcery spells" Sanwell, Avenger Ace "a Vehicle or artifact creature spell" Wand of Wonder "up to X instant and/or sorcery spells" Scarlet Witch, Chaotic Avenger "a Hero or noncreature spell" Acceptance requires either two or more legs or a consumed quantifier, which is also the anti-swallow guard: "cast a spell from among them" (Aetherworks Marvel, Svella, Apex of Power) still yields no gate. The parser half alone was inert. The chain seam forwards every exiled card as the sub-ability's targets, so `target_ids` arrived non-empty and skipped the one site where the cast filter was applied — every exiled card got the permission regardless of type. `cast_from_zone` now retains only forwarded ids matching the clause's own legs, using a new `TargetFilter::without_exile_anaphor()` that discharges the `ExiledBySource` leg the seam already satisfied while preserving And/Or structure. Re-evaluating the anaphor here would be actively wrong: on a triggered ability it reads a snapshot captured before this ability's own exile step, which would drop every id and turn the bug into a total no-op. Scoped to filters that reference the exile anaphor, so explicitly targeted grants (Emry, Bring to Light, Urza) are untouched, and the 51 bare-anaphor rows residualize to None and keep the full forwarded set. Removes Scarlet Witch, Chaotic Avenger from parser-misparse-backlog root cause phase-rs#6. Epic Experiment stays under #1: its "that weren't cast" cleanup clause is still dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(PR-6996): preserve hand cast gate predicates * test(PR-6996): distinguish hand cast type gate * fix(PR-6996): complete hand gate regression loop * test(PR-6996): select hand binding in rich gate regression --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
matthewevans
pushed a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 24, 2026
…phase-rs#7703) * docs: custom format engine design proposal (discussion phase-rs#5312) Research + design for a general, data-driven custom-format layer, validated by expressing four Eternal Central retro formats (Old School 93-94, Old School 95, Middle School, Classic Magic) as data on top of it rather than four hardcoded GameFormat variants. Schema splits CustomFormatRules into two independent axes: StructuralRules (life, players, deck size, range of influence, team-based, singleton — already FormatConfig fields, already partially host-adjustable in the lobby) and LegalityRules (legal sets, banned/restricted, legacy rules like mana burn / damage-on-stack / pre-M10 Wish / legend-rule scope). Delivery recommendation is a "save as custom format" action on the existing lobby first (Axis A), with the four EC formats shipping as audited presets on the same schema (Axis B) in parallel. No engine or frontend code — design/research docs only, opened for maintainer review. * docs(custom-format-engine): narrow MVP to Swedish Old School + Axis A only Full LegacyRuleSet engine wiring (mana burn, damage-on-the-stack, pre-M10 Wish, legend-rule scope) is real risk and makes the MVP harder to test. Swedish Old School 93/94 — a distinct, real ruleset verified this session against oldschool-mtg.blogspot.com/p/banrestriction.html — has its own restricted list (23 names, different from EC's), an empty banned list, and no mention of mana burn or any other legacy rule, so it needs none of that wiring. Premodern already exists as a native GameFormat and needs no new work at all; it's cited only as an existing precedent for the same shape. Re-sequences to two phases: phase 1 ships the general engine + the Axis A lobby-save action + swedish_old_school() as the only new Axis B preset (zero LegacyRuleSet wiring exercised); phase 2 ships the four EC formats plus the legacy-rules engine work they actually need. Nothing is cut — the four EC formats and full legacy-rules axis remain the target, they just move to a phase that ships once the schema is already proven end-to-end by something smaller. Flags two new open items: ante-card handling (a third list-shaped rule, distinct from banned/restricted, with no schema slot yet) and Swedish Old School's reprint policy (unconfirmed against the primary source). * docs(custom-format-engine): address matthewevans's CHANGES_REQUESTED review Fixes all five design-correctness gaps from the round-2 review: 1. legal_sets was a bare Vec<SetCode>, so Axis A's default (no set restriction) evaluated as "restricted to nothing" and rejected every card. Changed to Option<Vec<SetCode>> (None = unrestricted). 2. StructuralRules dropped command_zone, commander_damage_threshold, and archenemy_player, and had no source for sideboard_policy (which turns out to be a GameFormat method, not a FormatConfig field, so Custom has no derivation path for it). Added all four; uses_commander is now derived from commander_damage_threshold rather than stored redundantly; supplies_fixed_deck stays false for Custom; allow_debug_actions is correctly excluded (orthogonal to format per its own doc comment). 3. No identity/persistence/transport contract existed for a lobby-saved format. Resolved by separating two conflated concerns: in-game peer agreement (already solved -- FormatConfig.custom_rules carries the full payload, not a lookup key) from a player's reusable saved-format library (client-side-only, never an engine/WASM type). Flags the real version-skew risk (an old client can't be rescued by serde(default) on an enum variant it doesn't know) as a lobby-join-handshake requirement. 4. Mana burn was modeled as damage at every engine Phase-enum transition. Verified against docs/MagicCompRules.txt:8278: it's life loss, not damage. Verified against the engine's own Phase enum (types/phase.rs): it flattens MTG's steps and phases into one flat list, so gating on every transition fires mid-phase (e.g. DeclareAttackers -> DeclareBlockers), not just at real phase boundaries. Also found an existing generic mechanism this can reuse: player_unspent_mana_loss_causes_life_loss / apply_empty_mana_pool_event, currently used for a Yurlok-class card-granted ability at full CR 500.5 granularity. Redesigned as a phase-group-boundary-gated second contribution to the same event, independent of the Yurlok-class check. 5. Swedish Old School's restricted list was mislabeled 23 when 25 names are enumerated, and the preset sketch had dropped Summer Magic from the legal-sets list. Classic Magic's restricted list was labeled 37 when 44 are enumerated. All four fixed and cross-checked between CONTEXT.md, PLAN.md, and RESEARCH.md. * docs(custom-format-engine): address round-2 CHANGES_REQUESTED (matthewevans) Round 2's fixes were themselves incomplete or wrong on all five points matthewevans re-flagged. Each re-verified directly against the cited engine source before treating the review as correct, not accepted at face value: 1. Round 2 only gated the mana-burn LIFE-LOSS check to phase-group boundaries, leaving the pool-emptying event firing unconditionally on every Phase transition -- so by the time a boundary was reached, the pool was already silently drained with nothing left to burn. Fixed by reusing an existing mechanism instead of gating a side-effect on an event that already ran: the engine's ManaExpiry type already has EndOfCombat ("persists through combat steps, drains at EndCombat -> PostCombatMain", used by Firebending) -- generalized with a third variant, EndOfPhaseGroup, so mana_burn-tagged mana actually persists across intra-phase-group steps and only drops (and burns) at a real phase-group crossing. 2. StructuralRules.sideboard_policy had no accessor -- GameFormat:: sideboard_policy() can't see FormatConfig.custom_rules at all. Added FormatConfig::sideboard_policy() as the single canonical accessor and specified migrating both production call sites (deck_loading.rs, match_flow.rs) to it. 3. archenemy_player is per-game seat identity validated against that game's player count (FormatConfig::archenemy_player/validate_for_player_count), not a reusable structural setting -- removed from StructuralRules entirely; Axis A doesn't support the Archenemy topology. 4. uses_commander was derived from commander_damage_threshold alone; GameFormat::uses_commander()'s own doc comment states the real invariant requires command_zone AND the threshold. Fixed the derivation to use both conditions. 5. ReprintPolicy is declared but never consumed by the evaluator, and LegendRuleScope::PreM14AnyController's historical conflation was never resolved. Added a general preset-readiness rule: a preset can't be registered as selectable until every legality/legacy field it declares is both specified and actually enforced -- this currently blocks swedish_old_school() specifically. Also fixed two more lingering 23-vs-25 restricted-list references round 2 missed. * docs(custom-format-engine): full audit of all 4 review rounds, address round 3 Per direct instruction, re-audited every point raised across all four review rounds against current source (not against prior claims) rather than only fixing the newest round. Found one additional real gap nobody had named yet: deck_validation.rs's DeckCompatibilityRequest.selected_format has the same bare-GameFormat problem as companion.rs, just never flagged by anyone. 1. Round 3's sideboard_policy fix used FormatConfig::sideboard_policy() as a method with .expect("Custom format must carry custom_rules") -- a production panic path -- and migrated only 2 of 7 real consumer call sites (companion.rs x4, deck_loading.rs x2, match_flow.rs x2, deck_validation.rs x5). Fixed: fallible validation of the format/custom_rules invariant at every construction/ingestion point (malformed values rejected at the boundary, never constructed); sideboard_policy becomes a stored FormatConfig field matching the existing uses_commander/supplies_fixed_deck pattern (verified via their own consistency test at format.rs:1512-1513), not a new method; every real consumer migrates to read it, with signatures widened wherever they only carry a bare GameFormat today (companion_offers, DeckCompatibilityRequest). 2. from_lobby_config never specified where sideboard_policy comes from. Fixed: config.format.sideboard_policy(), valid because the conversion's input is always a built-in format at save time. 3. LegacyRuleSet's three bools (mana_burn, damage_uses_stack, pre_m10_wish_reaches_exile) become typed enums (ManaBurnPolicy, CombatDamageTiming, WishOutsideGameScope), matching LegendRuleScope's existing shape. Tightened the preset-readiness gate: no preset may ship in a "playable with a caveat" state -- retracts this doc's own earlier claim that Middle School/Classic Magic could ship before damage-on-stack lands. 4. Designed the version-skew compatibility fix concretely instead of flagging it: reuse the engine's existing PROTOCOL_VERSION/ MIN_SUPPORTED_PROTOCOL handshake gate (server-core/protocol.rs) rather than inventing new negotiation. For ReprintPolicy enforcement, named two sufficient resolution paths (the general printing cross-reference, or a one-preset verification pass) rather than building the full model now. Also fixed two stale pre_m10_wish_templating references in RESEARCH.md that survived since round 1 despite PLAN.md already using the canonical name throughout. * docs(custom-format-engine): inline source citations per format, not just once Every card list/count in RESEARCH.md shared one citation at the top of section 1, and Swedish Old School -- the actual phase-1 target preset -- had no RESEARCH.md presence at all, only scattered mentions in CONTEXT.md. That makes independent verification harder than it should be, especially given every "preset data inconsistency" finding across four review rounds was an internal cross-reference mismatch, not an external-source check -- worth making the external source trivially reachable at the point of data. - Added a "Source:" line to each of the four existing EC format subsections. - Added a full "Swedish Old School 93/94" subsection to RESEARCH.md phase-rs#1, matching the EC formats' treatment: direct source URL, its own verbatim legal-sets/banned/restricted/ante/legacy-rules data, and an explicit side-by-side comparison against EC's 93-94 restricted list proving these are two different, real rulesets, not one re-presented as two. - Added the same two source URLs inline at the top of PLAN.md phase-rs#2, so the preset constructors are checkable without cross-referencing another file. * docs(custom-format-engine): address CodeRabbit findings on the round-4 commit Automated review on the round-4 fix commit (3aa20f1) caught 5 things, at least 3 of which are real: 1. A genuine mistake in round 4's own mechanical rename fix: the search_outside_game pseudocode was updated from the round-1 placeholder name to pre_m10_wish_reaches_exile, but the SAME round-4 commit also converted that field to the typed wish_scope: WishOutsideGameScope enum -- the pseudocode never got updated to match, in the same commit that introduced the mismatch. Fixed, with the naming-history note now tracking both renames. 2. sideboard_policy/uses_commander are plain serialized FormatConfig fields (no #[serde(skip)]) that could diverge from what's derivable from `format`/`custom_rules` on a malformed wire payload -- a real gap that already exists for built-in formats today, not just custom ones. Widened validate_custom_rules_consistency to check derived-field agreement for every format, not only Custom. 3. DeckCompatibilityRequest.selected_format specifically needs the full CustomFormatRules (legal_sets/banned/restricted), not the lighter ResolvedFormatFacts struct that's sufficient for companion.rs's two call sites -- tightened from an ambiguous "per-site judgment call" to an explicit distinction. 4. The protocol-version fix only bumped PROTOCOL_VERSION; format selection happens during lobby setup, so LOBBY_PROTOCOL_VERSION / MIN_SUPPORTED_LOBBY_PROTOCOL (a separate, real constant pair, confirmed this session) needs bumping too. 5. CombatDamageTiming::OnStack's doc comment mischaracterized historical combat damage as a triggered ability; RESEARCH.md phase-rs#6 already correctly describes it as assigned damage placed on the stack as a stack object. Fixed to match. Also strengthened the preset-readiness gate from a documented convention into an actual technical mechanism (custom_format_registry() validates against a static implemented-axes table before returning a preset) -- responds to a stricter CodeRabbit read of the ReprintPolicy gate without fully adopting its more extreme "remove the field" suggestion, which goes beyond what the human reviewer (matthewevans) actually asked for. * docs(custom-format-engine): address round-5 CHANGES_REQUESTED (matthewevans) Round 5 opened with "the proposal now resolves the previously-requested custom-context, typed-policy, compatibility, and no-caveated-preset concerns" -- confirming round 4 in full. Two narrower points remained: 1. StructuralRules.singleton was declared (since round 2's full-fidelity fix) but evaluate_custom_format's step 5 always called copy_limit_violations(db, &counts, 4), never reading it, and there was no test. Fixed: parameterize on rules.structural.singleton (1 vs 4) -- confirmed this round that copy_limit_violations already takes exactly this parameter and every built-in singleton format already calls it with 1, so this is parameterizing an existing call, not new logic. Card-intrinsic overrides (Relentless Rats, DeckCopyLimit::UpTo) already compose correctly under any limit per the helper's own existing tests. 2. The registry gate (IMPLEMENTED_LEGACY_AXES) covered only LegacyRuleSet's four axes, not ReprintPolicy -- a preset could still register while declaring an unenforced ReprintPolicy. Rather than broadening the gate to a field never designed to be independently enforceable, re-read this document's own original research (RESEARCH.md phase-rs#3, predating any review round): it already concluded ReprintPolicy's behavior is fully absorbed into legal_sets curation, with only the frame/art-level distinction being a real gap (Open item 2's, not this field's). Resolution: reprint_policy becomes documentation metadata, deliberately not consumed by the evaluator and deliberately outside the registration gate's scope -- satisfying the "keep/reduce to non-selectable/deferred metadata" branch round 5 explicitly offered as acceptable. * docs(custom-format-engine): address round-6 CHANGES_REQUESTED (matthewevans) Round 6 confirmed the singleton fix landed correctly and found one remaining issue: round 5 resolved ReprintPolicy to "documentation metadata, never consumed" but left the field sitting inside LegalityRules/ CustomFormatRules -- the resolved, engine-consumed, wire-traveling payload -- while a comment beside it (unchanged since round 1) still said it "gates LEGALITY". Two incompatible claims about the same field's contract: a struct's shape is itself a claim about what travels with and is enforced by the resolved ruleset, and no comment disclaiming that changes what the type says. Resolved structurally, not documentarily: reprint_policy moves out of CustomFormatRules/LegalityRules entirely, onto a newly-sketched CustomFormatDef struct (previously only described in prose as "display metadata + CustomFormatRules" -- exactly the ambiguity that let this happen), alongside label/short_label/description. This is one of the two resolutions matthewevans offered, chosen over building real engine-owned printing enforcement, for the same reason round 5 established: this document's own original research already showed the field's behavior is fully absorbed by legal_sets curation. Updated every preset sketch, the preset-readiness gate's scope description, and the registry-gate reasoning to match -- the gate no longer needs to exempt reprint_policy since it isn't on the resolved-rules struct at all. Added a discriminating test proving two CustomFormatDef values with identical rules but different reprint_policy produce identical evaluation results. * docs(custom-format-engine): address round-7 CHANGES_REQUESTED (matthewevans) Round 7 confirmed the round-6 structural move was correct ("correctly resolves the prior semantic contradiction") and found one construction gap: CustomFormatDef.reprint_policy was a required ReprintPolicy, but from_lobby_config has no legitimate value to put there -- a lobby-saved format has no authored paper-format reprint intent at all, so forcing any of the three real variants onto it would be fabricated metadata. Fixed: reprint_policy: Option<ReprintPolicy>. None for from_lobby_config (and, for now, swedish_old_school() pending Open item 6) -- the same Option<T>-over-forcing-a-value pattern this proposal already uses for legal_sets and range_of_influence. Some(_) with a real, sourced value for every other Axis B preset. Also fixed a stale round-1 cross-reference in CONTEXT.md's original Axis B field list that still named reprint_policy alongside the genuinely resolved-legality fields, never updated when round 6 moved it. Added a test requiring each of the five preset constructors to set its own specific value, not one shared "is Some" check. * docs(custom-format-engine): address round-8 CHANGES_REQUESTED (matthewevans) from_lobby_config's signature (name + &FormatConfig) never specified how a lobby save derives short_label/description, but CustomFormatDef requires both non-optionally since round 6. Fixes by deriving short_label as name's first 3 alphanumeric chars uppercased (the same convention the frontend already falls back to independently for unrecognized formats) and description via a new derive_structural_description(&StructuralRules) helper mirroring built-in formats' existing comma-joined structural phrasing — both derived from name/config alone, no invented metadata. Extends the existing lobby-save round-trip test to assert the derivation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-9 CHANGES_REQUESTED (matthewevans) The round-8 fix put label/short_label/description on CustomFormatDef, but that struct never travels past the lobby/picker — FormatConfig.custom_rules carries only CustomFormatRules, and SavedCustomFormat.name is explicitly client-local by round 2's own identity design. GameFormat::label() also structurally cannot return CustomFormatDef.label (a String) from a &'static str-returning function regardless of what's threaded in. Fixes both without adding wire surface: label() becomes Cow<'static, str>, resolving Axis-B presets via a custom_format_registry() lookup (a stable id every peer already shares) and falling back to a fixed "Custom Format" string for Axis-A ad-hoc saves, which have no registry entry to resolve at all. Retracts PLAN.md's prior "label/for_format get a Custom arm reading the resolved def" claim, which was never true for Axis A. Also found and fixed a second, independent instance of the same root cause: FormatConfig::for_format(bare GameFormat) is called for .deck_size at two real deck-validation call sites, silently returning a wrong default size for Custom formats. Gives for_format the same unreachable!() guard sideboard_policy/uses_commander already use, migrating both call sites to read custom_rules.structural.deck_size when present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-10 CHANGES_REQUESTED (matthewevans) Matt flagged that Old School 93-94/95 register once mana burn lands with no gate tied to their source rules' printing-fidelity requirement (non-foil, original frame + art) - legal_sets is set-code-only and can't express this, and CONTEXT.md's own open item 2 already flagged the gap as unresolved, but the rollout plan had drifted out of sync with it. Resolved directly (discussed with the repo owner rather than decided unilaterally, per the doc's own "do not resolve unilaterally" instruction on that open item): legality enforcement stays legal_sets-only permanently -- no foil/frame data exists anywhere in the engine to enforce against. The source rules' spirit is instead honored by a general display fix: ArtChainEntry's existing {type: "oldest"} per-player preference becomes legal_sets-aware, so "oldest printing" respects the active format's legal set list instead of picking a promo/non-tournament printing the format doesn't recognize. Zero engine change, not gated on ReprintPolicy, benefits every format with a legal_sets restriction, not just the three Axis-B presets that declare one. Also backfills CONTEXT.md's round-history log with rounds 8 and 9, which only got PLAN.md updates in the prior two commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-11 CHANGES_REQUESTED (matthewevans) Round 10 tried to resolve Old School 93-94/95's printing-fidelity gap by pairing legal_sets-only legality with a cosmetic ArtChainEntry display default. Matt correctly rejected this: an optional rendering preference is not a legality resolution. Resolved via a third path, grounded in existing precedent rather than a new policy call: confirmed GameFormat::Premodern's legality (LegalityFormat::Premodern) is oracle-card-level only, like every format in this engine -- none has ever checked printing, frame, or foil, and PrintedCardRef has no set-code field for any of them. legal_sets membership isn't an old-school-specific approximation needing a gate; it's this engine's one existing legality model, applied the same way to every format. Registration reverts to mana-burn-only, matching the original pre-round-10 gate. The ArtChainEntry display fix and genuine per-card printing selection (briefly surveyed: a moderate plumbing lift reusing existing PrintingPickerModal/sourcePrinting infrastructure, not from-scratch) are both retracted from this proposal as separate, real, future ideas -- general to every format, not bundled into this proposal's legality story. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-12 CHANGES_REQUESTED (matthewevans) — Old School printing fidelity Round 11's resolution ("legal_sets membership isn't an approximation of Old School 93-94/95's legality — it's the same oracle-card-level model Premodern and every other format already uses") was reviewed again and correctly rejected: Premodern never claimed a printing-level requirement in the first place, so it not enforcing one isn't an approximation of anything. Old School 93-94/95's own cited source (RESEARCH.md §1) explicitly requires non-foil original-frame/art reprints, and legal_sets genuinely falls short of that specific stated rule regardless of what every other format checks. Product decision (not resolved away by precedent this time): decline to build engine-owned printing/frame enforcement — the engine's and frontend's printing systems are confirmed disconnected, and wiring them is a real, separate, moderate-lift future feature, not old-school-specific. Accept the set-code-only approximation on its own terms: in a digital-only client, a printing's frame/border/foil status has zero gameplay consequence, since two printings with identical Oracle text are identical for every rules purpose the engine cares about. The paper community's frame/art requirement serves an anti-counterfeiting/provenance function specific to a physical table that has no digital equivalent. Takes the maintainer's own second offered resolution (explicitly scope the presets as an oracle-card/set-code approximation) and makes it structural rather than documentary, per this proposal's own established standard that conventions must be enforced, not just written down: - New `CustomFormatDef.printing_fidelity: PrintingFidelity` field (NotApplicable / SetCodeApproximation), required and paired with `reprint_policy` at construction time. - All four EC presets (old_school_93_94, old_school_95, middle_school, classic_magic) set SetCodeApproximation and must disclose the limitation in their player-facing `description`, not just a doc comment. - New registry gate (§7) and test (§6) enforce the reprint_policy/ printing_fidelity pairing and the description disclosure — separate from the existing IMPLEMENTED_LEGACY_AXES gate, which covers unimplemented engine work; this one covers an authoring omission. - CONTEXT.md's Open item 2 log gets an appended correction + final resolution (round 12), not an overwrite of round 11's now-superseded entry. Also fixes the CodeRabbit-flagged [MEDIUM]: old_school_95()'s builder called `d.legal_sets.extend(...)` directly, but legal_sets is `Option<Vec<SetCode>>` per §1 — `.extend()` doesn't exist on `Option`. Uses `get_or_insert_with(Vec::new).extend(...)` so it stays correct even if a future refactor changes the base preset's Some/None invariant. Model: claude-sonnet-5 Tier: Frontier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ1bpYEt331DvjqPLSFJEH * docs(custom-format-engine): fix old_school_95() field nesting + add inheritance test Maintainer review of the round-12 commit confirmed the printing-fidelity blocker resolved, but caught that old_school_95()'s builder sketch mutates d.legal_sets/restricted/banned directly — those aren't fields on CustomFormatDef at all; they live at d.rules.legality.* per §1's own declared schema (CustomFormatDef.rules: CustomFormatRules, CustomFormatRules.legality: LegalityRules). The prior get_or_insert_with fix was correct for the Option<Vec<SetCode>> detail but applied at the wrong nesting level. Every mutation now composes through the real path (d.rules.legality.legal_sets.get_or_insert_with(...).extend(...), etc.), and adds the requested preset-inheritance test: asserts old_school_95()'s resolved legal_sets/restricted/banned each equal old_school_93_94()'s base plus exactly its own declared delta (an exact-set comparison, not a superset check), which would also have caught this nesting bug mechanically. Model: claude-sonnet-5 Tier: Frontier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ1bpYEt331DvjqPLSFJEH --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.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
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.