feat(parser): Grisly Anglerfish — force-attack activated ability#1
Open
mike-theDude wants to merge 10 commits into
Open
feat(parser): Grisly Anglerfish — force-attack activated ability#1mike-theDude wants to merge 10 commits into
mike-theDude wants to merge 10 commits into
Conversation
… effect (phase-rs#662) * feat(parser): support 'can block an additional creature this turn' as effect CR 509.1a + CR 509.1b: Add try_parse_can_block_additional handler in the subject-predicate effect parser to recognize activated/triggered ability text of the form '[subject] can block an additional creature this turn' and '[subject] can block any number of creatures this turn'. Previously, this pattern was only recognized as a permanent static ability (in oracle_static.rs). When it appeared as an activated ability effect (e.g. '{2}: ~ can block an additional creature this turn.'), the effect parser could not produce the ExtraBlockers static mode, leaving it as an unimplemented gap. The new handler mirrors try_parse_can_attack_with_defender: it intercepts the text before the generic continuous-clause parser, extracts the subject, and emits a GenericEffect carrying: - StaticMode::ExtraBlockers { count: Some(1) } (or count: None for 'any number') - ContinuousModification::AddStaticMode with the same mode - Duration::UntilEndOfTurn This unlocks Luminous Guardian, Coastline Chimera, Anurid Swarmsnapper, and 13+ other cards whose only gap was this effect pattern. Includes two unit tests validating both the 'additional' and 'any number' variants. * fix: map 'this combat' to Duration::UntilEndOfCombat (CR 514.2) Codex review correctly identified that 'can block an additional creature this combat' was mapped to UntilEndOfTurn. Per CR 514.2, 'this combat' means until end of the current combat phase, not end of turn. Split the duration check into two branches: - 'this turn' → Duration::UntilEndOfTurn - 'this combat' → Duration::UntilEndOfCombat * fix: use nom combinators for can_block_additional parser Replaced string-based dispatch (.find(), .contains()) with modular nom combinators (alt, tag, opt, preceded) to comply with the repository's architectural rule (R1) for robust Oracle phrase parsing. * fix(PR-662): parse extra blocker grants structurally --------- Co-authored-by: Whovencroft <6.60056e+06+Whovencroft@users.noreply.github.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
* Add Stoneshock Giant * fix(PR-578): decompose monstrous trigger parser --------- Co-authored-by: AI Assistant <hadamek@example.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…hase-rs#663) * fix(coverage): mark CantBeBlockedExceptBy:Quality as data-carrying CR 509.1b: the Quality variant of CantBeBlockedExceptBy carries an open-value TargetFilter (e.g. Subtype("Wall"), Flying, artifact creatures) whose runtime enforcement already lives in combat.rs::validate_blockers. The variant was absent from is_data_carrying_static(), so coverage incorrectly reported the following four cards as unsupported: - Prowler's Helm (can't be blocked except by Walls) - Invisibility (can't be blocked except by Walls) - Seeker (can't be blocked except by artifact creatures) - Canopy Cover (can't be blocked except by creatures with flying) The MinBlockers variant is intentionally excluded: it is handled through the Menace-style static-registry path and must NOT be data-carrying. Tests added: - cant_be_blocked_except_by_quality_is_data_carrying - cant_be_blocked_except_by_min_blockers_is_not_data_carrying (regression guard) * fix: guard against TargetFilter::Any in CantBeBlockedExceptBy coverage fallbacks don't incorrectly mark unsupported cards as supported. --------- Co-authored-by: Whovencroft <6.60056e+06+Whovencroft@users.noreply.github.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
* Add Heed the Mists
Extends the possessive-participle recogniser to cover "milled" so that
"draw cards equal to the milled card's mana value" (Heed the Mists,
Mindshrieker, Infernal Genesis class) resolves to
`ObjectManaValue { CostPaidObject }` instead of `Unimplemented`.
Fix 1 — `oracle_quantity.rs` `is_event_context_referent`: adds "milled"
to the `event_adjectives` array (CR 701.17a + 701.17c).
Fix 2 — `oracle_nom/quantity.rs` `parse_cost_paid_object_ref`: adds
`tag("milled ")` to the participle `alt()` so CDA/quantity-ref paths
also parse "the milled card's mana value" (CR 701.17a).
Three new tests: building-block tests in both quantity modules, plus a
full-chain parser test in oracle_effect that asserts Mill → Draw
chaining with the correct ObjectManaValue count.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Fix: correct CR 117.1→608.2k annotation in parse_cost_paid_object_ref
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* Fix: remove milled from event_adjectives, use parse_cost_paid_object_ref path
Cost-paid participle adjectives (sacrificed/exiled/discarded/milled) are
deliberately excluded from is_event_context_referent per the design comment
at that site. The fallback parse_quantity_ref → parse_cost_paid_object_ref
path handles "the milled card's mana value" correctly.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
* fix(PR-756): tighten milled quantity CR annotations
---------
Co-authored-by: Michael Briningstool <mbriningstool@gmail.com>
Co-authored-by: Claude Sonnet 4.6 <noreply@anthropic.com>
Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…y Anglerfish
CR 508.1d: Add a scanner in `try_parse_subject_restriction_clause` that finds
the attack-if-able duration phrase at word boundaries, extracts everything
before it as the subject, and builds a `GenericEffect { MustAttack }` with the
subject filter propagated to both `static_def.affected` and the outer broadcast
`target`. Handles plural-noun-phrase subjects like "Creatures your opponents
control" that don't start with a recognized `starts_with_subject_prefix` token
and so were previously unreachable via the `strip_subject_clause` path.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
mike-theDude
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 phase-rs#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 phase-rs#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>
mike-theDude
pushed a commit
that referenced
this pull request
May 25, 2026
…ct count - engine.rs: populate current_trigger_match_count from pending_optional_trigger_match_count before drain_pending_continuation so EventContextAmount resolves the correct "that many" in the resumed continuation (Gemini comment #1 — logic error in state restoration) - trigger_matchers.rs: add CR 603.2c doc annotation to trigger_event_subject_ids (comment phase-rs#2); extend match to return ObjectId arms for TokenCreated, CreatureDestroyed, PermanentSacrificed, PermanentTapped, PermanentUntapped, and DamageDealt (object target only, per CR 120.3) — previously these all fell through to std::iter::empty() (comment phase-rs#3) - quantity.rs: replace .map(|n| n as i32) with .map(u32_to_i32_saturating) for consistency with the rest of the file (comment phase-rs#4) Verified: cargo clippy --all-targets -- -D warnings (clean), cargo test -p engine (8956 passed) Co-Authored-By: Claude Sonnet 4.6 <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>
mike-theDude
pushed 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
pushed a commit
that referenced
this pull request
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>
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
6 participants
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.
Summary
try_parse_subject_restriction_clausefor the CR 508.1d pattern[subject] attack(s) [this turn/this combat] if ablescan_preceded+parse_attack_if_able_durationto find the phrase at word boundaries; everything before it becomes the subjectparse_subject_applicationhandles plural-noun-phrase subjects like "Creatures your opponents control" via the existing bare-plural path (prepends "all " → delegates toparse_type_phrase)static_def.affected(layer enforcement) and the outer broadcasttarget(mirrors whatinject_subject_targetwould do)Test plan
creatures_opponents_control_attack_this_turn_if_ableinoracle.rsverifies theMustAttackGenericEffectwithUntilEndOfTurnduration,affected.is_some(), andtarget = Some(Typed(_))cargo clippy --all-targets -- -D warningsclean🤖 Generated with Claude Code