Skip to content

fix(engine): make the "before attackers declared" window phase-based (CR 508.1/508.2)#5192

Merged
matthewevans merged 1 commit into
phase-rs:mainfrom
Spaceint:fix/before-attackers-window
Jul 6, 2026
Merged

fix(engine): make the "before attackers declared" window phase-based (CR 508.1/508.2)#5192
matthewevans merged 1 commit into
phase-rs:mainfrom
Spaceint:fix/before-attackers-window

Conversation

@Spaceint

@Spaceint Spaceint commented Jul 6, 2026

Copy link
Copy Markdown
Contributor

Summary

is_before_attackers_declared (the shared predicate behind the BeforeAttackersDeclared casting and activation restrictions) gated the window on priority_seat(state) == state.active_player. That clause is only ever satisfiable by the active player, so it wrongly closed the "before attackers are declared" window for non-active actors.

Per CR 508.1 ("First, the active player declares attackers. This turn-based action doesn't use the stack") + CR 508.2 ("Second, the active player gets priority"), attackers are declared as a turn-based action before any player receives priority. So "before attackers are declared" is a pure phase property — the PreCombatMain | BeginCombat run-up — independent of who currently holds priority. This PR drops the priority-seat clause and keeps the phase check.

What was broken (and is now fixed)

  • Master Warcraft — "Cast this spell only before attackers are declared." A non-active player could never cast it during another player's combat.
  • Siren's Call — "…only during an opponent's turn, before attackers are declared." Its two casting restrictions were mutually exclusive under the old clause (DuringOpponentsTurn needs active_player != caster, but the window needed the caster's seat == active player) → entirely uncastable front-door.

Why it's safe

26 of the 28 BeforeAttackersDeclared cards are [DuringYourTurn, BeforeAttackersDeclared] (Portal Three Kingdoms tap-ability cycle, King's Assassin, etc.). Restrictions are AND-composed, and DuringYourTurn => active_player == player independently pins those to the active player — so a phase-only window cannot widen them. The only two BeforeAttackersDeclared-without-DuringYourTurn cards (Master Warcraft, Siren's Call) are exactly the intended beneficiaries. No existing test exercises the old priority-gated behavior; turn-control (CR 723) can't affect a check that never reads priority.

Anchored on

  • crates/engine/src/game/restrictions.rs — is_before_combat_damage / is_sorcery_speed_window: sibling timing predicates that key on state.phase (phase-based window pattern).
  • crates/engine/src/game/restrictions.rs — ActivationRestriction::DuringYourTurn => state.active_player == player: the AND-composed active-player gate that keeps the 26 turn-qualified cards pinned.

Test Plan

Three runtime restriction-check tests drive the production entry points (check_casting_restrictions / check_activation_restrictions):

  • before_attackers_window_admits_non_active_caster — a non-active player is inside the window in both PreCombatMain and BeginCombat (with an active-player reach-guard). Fail-on-revert measured: restoring the priority-seat clause makes only this test fail.
  • before_attackers_window_closes_at_and_after_declaration — window closed at DeclareAttackers/DeclareBlockers/PostCombatMain, open at BeginCombat (reach-guard).
  • during_your_turn_before_attackers_stays_active_player_gated[DuringYourTurn, BeforeAttackersDeclared] stays legal on the controller's own turn / illegal on an opponent's turn, across both phases (non-regression).

Verification (Non-developer track; Tilt down → direct cargo)

  • cargo test -p engine — 196 test binaries, 0 failures
  • CI-parity clippy (--workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings) — exit 0
  • cargo check -p engine-wasm --target wasm32-unknown-unknown — exit 0
  • cargo fmt --all — clean
  • Gate A (./scripts/check-parser-combinators.sh) — exit 0 (no parser change)

Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Track: Non-developer (card-data.json integration deferred to CI per docs/AI-CONTRIBUTOR.md §2)

…(CR 508.1/508.2)

`is_before_attackers_declared` gated the window on `priority_seat == active_player`,
which is only ever true for the active player. Per CR 508.1/508.2 attackers are
declared as the first turn-based action of the declare-attackers step, before any
player receives priority, so "before attackers are declared" is a pure phase
property (PreCombatMain | BeginCombat) independent of who holds priority.

The old clause wrongly blocked non-active actors' "before attackers" cast/activation:
- Master Warcraft ("Cast this spell only before attackers are declared") was
  uncastable by a non-active player during another player's combat.
- Siren's Call ("only during an opponent's turn, before attackers are declared")
  was entirely uncastable front-door (the two casting restrictions were mutually
  exclusive under the priority-seat clause).

Safe for the 26 `[DuringYourTurn, BeforeAttackersDeclared]` cards: DuringYourTurn
(AND-composed) independently pins them to the active player, so the phase-only
window cannot widen them. Adds three runtime restriction-check tests (non-active
caster admitted with an active-player reach-guard; window closes at/after
declaration; DuringYourTurn cards stay active-player-gated across both phases).

Co-Authored-By: Claude Opus 4.8 <[email protected]>
@Spaceint Spaceint requested a review from matthewevans as a code owner July 6, 2026 05:53
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

@github-actions

github-actions Bot commented Jul 6, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

@matthewevans matthewevans self-assigned this Jul 6, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved. I checked the restriction seam and CR boundary: BeforeAttackersDeclared is enforced in the shared casting/activation restriction predicate, and CR 508.1/508.2 make this a phase boundary before declare-attackers priority, not an active-player priority-seat check.

The added tests are discriminating: the non-active caster case fails on the old priority-seat gate, the declare-attackers/post-combat phases stay closed, and [DuringYourTurn, BeforeAttackersDeclared] activations remain active-player gated.

@matthewevans matthewevans added the bug Bug fix label Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
@github-merge-queue github-merge-queue Bot removed this pull request from the merge queue due to failed status checks Jul 6, 2026
@matthewevans matthewevans self-assigned this Jul 6, 2026
@matthewevans matthewevans added this pull request to the merge queue Jul 6, 2026
@matthewevans matthewevans removed their assignment Jul 6, 2026
Merged via the queue into phase-rs:main with commit 3098074 Jul 6, 2026
11 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Bug fix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants