Skip to content

fix(engine): enforce event-bound forced blocks#6613

Merged
matthewevans merged 10 commits into
mainfrom
ship/enforce-event-bound-forced-blocks
Jul 25, 2026
Merged

fix(engine): enforce event-bound forced blocks#6613
matthewevans merged 10 commits into
mainfrom
ship/enforce-event-bound-forced-blocks

Conversation

@matthewevans

@matthewevans matthewevans commented Jul 24, 2026

Copy link
Copy Markdown
Member

Summary by CodeRabbit

  • New Features

    • Blocker assignments now support multiple attacker targets per blocker, with pair-based selection and rendering.
    • Combat UI can show exact must-block requirements by attacker, with updated localized “must block {{attacker}}” labels.
    • Block declarations use a more complete rules solver for complex blocker/requirement combinations, and AI now submits completed, engine-validated proposals.
  • Bug Fixes

    • Improved must-block confirmation gating and cleared partial selections when prompts change.
    • Safer combat-tax handling by restoring snapped blocker declarations on accept and rebuilding prompts on decline.
    • Forced-block effects now preserve the correct attacker identity, duration, and incarnation across combat.

@matthewevans
matthewevans enabled auto-merge July 24, 2026 23:30
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: ab1371b7-2077-4922-9c72-67d8c5195c3d

📥 Commits

Reviewing files that changed from the base of the PR and between 9d2e929 and b9b65a6.

📒 Files selected for processing (2)
  • crates/engine/src/database/synthesis.rs
  • crates/engine/src/game/triggers.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/engine/src/database/synthesis.rs
  • crates/engine/src/game/triggers.rs

📝 Walkthrough

Walkthrough

The change adds exact attacker and object-incarnation tracking for force-block effects, introduces global blocker-declaration validation and tax snapshot restoration, updates AI completion, and changes the client to support multiple attacker pairs per blocker.

Changes

Force-block parsing and provenance

Layer / File(s) Summary
Force-block contracts and parsing
crates/engine/src/types/ability.rs, crates/engine/src/parser/..., crates/engine/src/game/ability_*.rs
Force-block effects and AST nodes now carry optional attacker references and explicit durations; parsing recognizes event/source attacker referents and SourceAttackedThisCombat.
Attacker identity and effect lifetime
crates/engine/src/game/triggers.rs, crates/engine/src/game/effects/force_block.rs, crates/engine/src/types/game_state.rs, crates/engine/src/game/layers.rs
Triggered attacks are split into singleton events, exact attacker incarnations are bound, and transient effects are restricted to their recorded recipients and incarnations.

Blocker declaration flow

Layer / File(s) Summary
Blocker legality, tax, and AI completion
crates/engine/src/game/combat.rs, crates/engine/src/game/engine_combat.rs, crates/engine/src/ai_support/candidates.rs, crates/phase-ai/src/search.rs
Blocker validation now maximizes globally satisfiable requirements, rejects duplicate pairs, deduplicates blocker taxes, snapshots declarations across tax prompts, and completes AI proposals through the engine.
Client multi-pair blocker interaction
client/src/stores/uiStore.ts, client/src/components/board/..., client/src/hooks/useRafPositions.ts, client/src/viewmodel/blockerSorting.ts
Client blocker state uses blocker-to-attacker sets, rendering and sorting preserve all pairs, and confirmation dispatches the flattened pair list.
Combat requirement presentation
client/src/components/combat/..., client/src/adapter/types.ts, client/src/i18n/locales/*/game.json
Engine-provided attacker IDs are exposed in requirement payloads and rendered with exact-attacker badge labels; local assignment status is no longer used for client-side legality gating.

Estimated code review effort: 5 (Critical) | ~120 minutes

Possibly related PRs

  • phase-rs/phase#6330: Both changes modify blocker-legality validation in crates/engine/src/game/combat.rs.

Suggested labels: bug

Sequence Diagram(s)

sequenceDiagram
  participant OracleParser
  participant TriggerSystem
  participant CombatEngine
  participant BlockerUI
  OracleParser->>TriggerSystem: parse force-block attacker and duration
  TriggerSystem->>CombatEngine: bind exact attacker incarnation
  CombatEngine->>BlockerUI: publish blocker requirements and attacker IDs
  BlockerUI->>CombatEngine: submit blocker-attacker pairs
  CombatEngine->>BlockerUI: accept declaration or rebuild blocker prompt
Loading
🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main change: event-bound forced blocks in the engine.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch ship/enforce-event-bound-forced-blocks

Comment @coderabbitai help to get the list of available commands.

@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 27 card(s), 6 signature(s) (baseline: main 6ae9c62794e9)

🟢 Added (1 signature)

  • 9 cards · ➕ ability/Untap · added: Untap (target=defending player controls creature)
    • Affected (first 3): Brontotherium, Crested Craghorn, Deftblade Elite (+6 more)

🔴 Removed (1 signature)

  • 9 cards · ➖ ability/Untap · removed: Untap (target=defending player controls creature)
    • Affected (first 3): Brontotherium, Crested Craghorn, Deftblade Elite (+6 more)

🟡 Modified fields (4 signatures)

  • 13 cards · 🔄 ability/ForceBlock · changed field attacker: Source
    • Affected (first 3): Auriok Siege Sled, Burning-Tree Bloodscale, Crashing Boars (+10 more)
  • 4 cards · 🔄 ability/ForceBlock · changed field duration: UntilEndOfCombat
    • Affected (first 3): Avalanche Tusker, Fighter Class, Impetuous Devils (+1 more)
  • 3 cards · 🔄 ability/ForceBlock · changed field attacker: EventSource
    • Affected (first 3): Fighter Class, Magnetic Web, Tolsimir, Midnight's Light
  • 1 card · 🔄 trigger/Attacks · changed field condition: source attacked this combat
    • Affected (first 3): Tolsimir, Midnight's Light

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Actionable comments posted: 13

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (3)
client/src/components/board/ActionButton.tsx (1)

176-179: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Reset the pending blocker on prompt replacement.

This effect only tracks mode; consecutive DeclareBlockers prompts keep "combat-blockers", so a stale pendingBlocker can carry into the next prompt and make the next attacker click assign unexpectedly. Reset against a blocker-prompt identity (for example, waitingFor) as well.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/src/components/board/ActionButton.tsx` around lines 176 - 179, Update
the pending-blocker reset effect in ActionButton to also depend on the blocker
prompt identity, such as waitingFor, not only mode. Ensure pendingBlocker is
cleared whenever a new DeclareBlockers prompt replaces the previous one, while
preserving the existing reset on mode changes.

Source: Path instructions

crates/engine/src/parser/swallow_check.rs (1)

4092-4108: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Misplaced exemption breaks the file's own documented *ThisTurn invariant.

TriggerCondition::SourceAttackedThisCombat is added to detect_duration_this_turn's exemption set, but the surrounding comment (lines 3990-3995, 4046-4047) explicitly states the checkable rule for this arm: every variant named here must contain ThisTurn — this one contains ThisCombat instead. It's also unreachable for the very card this PR targets (Tolsimir's text has no " this turn" substring at all, so the function returns at line 3824 before reaching this arm), and Effect::ForceBlock { .. } (line 4197) already exempts the actual force-block-with-combat-history shape. As written, this creates a latent false-negative: a future card combining "attacked this combat" with an unrelated, genuinely swallowed "this turn" elsewhere would have that real bug masked.

🐛 Proposed fix: remove the misplaced arm
     if evidence.any::<TriggerCondition>(|x| {
         matches!(
             x,
             TriggerCondition::SourceEnteredThisTurn
-                | TriggerCondition::SourceAttackedThisCombat
                 | TriggerCondition::DealtDamageBySourceThisTurn
                 | TriggerCondition::DealtDamageThisTurnBySource { .. }
                 | TriggerCondition::FirstTimeObjectTappedThisTurn
                 | TriggerCondition::FirstTimeObjectCountersAddedThisTurn
                 | TriggerCondition::AttackedThisTurn
                 | TriggerCondition::CastSpellThisTurn { .. }
                 | TriggerCondition::SpellCastWithVariantThisTurn { .. }
                 | TriggerCondition::CounterAddedThisTurn
         )
     }) {
         return;
     }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/parser/swallow_check.rs` around lines 4092 - 4108, Remove
TriggerCondition::SourceAttackedThisCombat from the exemption matches in
detect_duration_this_turn, leaving only variants whose names contain ThisTurn.
Preserve the existing exemptions and the separate Effect::ForceBlock handling.
crates/engine/src/game/ability_scan.rs (1)

1146-1150: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Do not hide the new ForceBlock payloads behind ...

This scanner now visits only target, while the PR adds attacker-provenance and duration fields to force-block effects. Any read-bearing attacker or duration payload is therefore omitted, allowing false-negative axis results and unsafe ordering/loop classifications. Destructure every field and scan each applicable payload, especially through scan_duration, to preserve fail-closed behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/game/ability_scan.rs` around lines 1146 - 1150, Update the
ForceBlock arm in the effect scanner to explicitly destructure every payload
field instead of using .., and scan each read-bearing attacker and duration
value with the appropriate scanner, including scan_duration. Preserve scanning
target while combining all applicable axes so ordering and loop classification
remains fail-closed.
🧹 Nitpick comments (2)
crates/engine/src/types/ability.rs (1)

11518-11529: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Consider a serde round-trip test for the new ForceBlock shape.

This file otherwise adds a dedicated round-trip test for nearly every new/changed serialized field shape (enters_under_*, duration_roundtrip, zone_owner_serde_round_trips_each_variant, etc.). A small test covering Effect::ForceBlock { attacker: Some(ForceBlockAttackerRef::EventSource), duration: Duration::UntilEndOfCombat, .. } (plus the legacy/no-attacker/no-duration deserialize path) would guard the new backward-compat defaults the same way the rest of the file does.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/types/ability.rs` around lines 11518 - 11529, Add a serde
round-trip test near the existing serialized-field tests for the ForceBlock
variant, covering attacker set to ForceBlockAttackerRef::EventSource and
duration set to Duration::UntilEndOfCombat. Also deserialize a legacy ForceBlock
payload without attacker or duration and assert the defaults preserve the
generic attacker behavior and UntilEndOfTurn lifetime.
crates/engine/src/game/layers.rs (1)

4606-4615: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

Add a CR citation for the new incarnation-liveness guard.

The new affected_recipient mismatch check is rules-touching (it determines whether a "locked-in" continuous effect still applies to its bound recipient across zone changes/incarnation changes), but it has no CR annotation, unlike the adjacent UntilHostLeavesPlay check right below it. CR 400.7 ("if an effect causes an object to move to a public zone, other parts of that effect can find that object" — with the corollary that a locked-in effect stops applying once the referenced object is no longer the same object, absent a specific 400.7a-k carve-out) is the applicable rule for this guard.

As per path instructions, crates/engine/** code implementing MTG game rules must carry a verified CR <number>: <description> annotation: "Verify every implementation of MTG game rules against the relevant Comprehensive Rules section before completion, and annotate rules-related code with a verified CR citation and description."

📝 Suggested comment
 pub(crate) fn transient_effect_is_live(state: &GameState, tce: &TransientContinuousEffect) -> bool {
+    // CR 400.7: a continuous effect "locked in" to a specific object stops applying
+    // once that object is no longer the same object (e.g. it left and re-entered the
+    // battlefield as a new incarnation), absent a specific 400.7a-k carve-out.
     if let Some(recipient) = tce.affected_recipient {
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/game/layers.rs` around lines 4606 - 4615, Add a verified CR
400.7 annotation describing the affected_recipient incarnation-liveness guard in
transient_effect_is_live, placing it alongside the guard and before or adjacent
to the existing UntilHostLeavesPlay rule check. Keep the implementation
unchanged and ensure the citation explains that the locked-in effect no longer
applies when the referenced object is a different incarnation.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@client/src/components/board/__tests__/BlockAssignmentLines.test.ts`:
- Around line 7-20: Update the test data in the filterVisibleBlockerPairs test
to include two pairs sharing blocker ID 10, such as [10, 100] and [10, 200], and
ensure blocker 10 is visible in the selected IDs. Assert that both pairs remain
in the filtered result, while preserving coverage of the existing
distinct-blocker cases.

In `@client/src/components/board/blockAssignmentVisibility.ts`:
- Around line 11-13: Remove the client-side controller-based filtering from the
assignment-visibility logic around the blocker-pair filter. Add or use an
engine/adapter API that returns blocker pairs already filtered for the viewing
player, and have the frontend render that engine-provided collection directly
without inspecting objects, controllers, or visiblePlayerIds.

In `@client/src/components/combat/useBlockerConstraints.ts`:
- Around line 51-64: Update the MustBlock handling in the constraint annotation
logic around blockerAssignments so it no longer derives satisfaction from
selectedAttackers or requiredAttackers. Reuse the engine-provided progress or
validation result for each constraint, ensuring the displayed status matches the
engine’s global solver and cannot report satisfied when declaration is rejected.

In `@client/src/components/combat/useBlockRequirements.ts`:
- Around line 49-64: Replace the local assignedPerAttacker counting and status
derivation in the useBlockRequirements flow with the engine-provided
preview/status for the currently selected blocker pairs. Use the existing engine
API or helper that evaluates those assignments, and pass through its statuses
without calculating, filtering, or reinterpreting requirement state in the
frontend.

In `@crates/engine/src/database/synthesis.rs`:
- Around line 5171-5172: Update the Provoke effect using
ForceBlockAttackerRef::Source to use Duration::UntilEndOfCombat instead of
Duration::UntilEndOfTurn, and revise its related description and tests to assert
the requirement ends at combat.

In `@crates/engine/src/game/ability_rw.rs`:
- Line 2834: Update the Effect::ForceBlock match arm to bind duration explicitly
instead of hiding it behind .., then invoke legacy_duration(duration) within its
legacy-effect classification. Keep the existing ForceBlock handling unchanged
otherwise and align it with the neighboring ForceAttack logic.

In `@crates/engine/src/game/combat.rs`:
- Around line 1327-1630: Bound BlockDeclarationConstraints::search_free with a
deterministic node budget so wide blocker-choice Cartesian products cannot
exhaustively stall declaration. Track visited search nodes across
best_free_declaration and stop exploring once the budget is reached, while
preserving score/length/lexicographic ordering for fully explored results. Add a
fallback that returns the best legal, tax-free declaration found so far (or the
existing empty declaration when none is found), including MustBeBlockedByAll and
lure requirements.

In `@crates/engine/src/game/coverage.rs`:
- Line 2356: Update the Effect::ForceBlock coverage handling to destructure its
named attacker and duration fields instead of discarding them with `..`. Include
both non-default attacker and duration details in the coverage output alongside
target, preserving the existing output behavior for default values.

In `@crates/engine/src/game/triggers.rs`:
- Around line 12198-12239: Add positive reach guards to the no_tolsimir_attack
test: verify parsed.condition is
Some(TriggerCondition::SourceAttackedThisCombat) and parsed.execute is present,
then confirm the same fixture produces a pending trigger after
attacking_incarnations_this_combat includes Tolsimir. Keep the existing empty
assertion so it specifically validates the intervening-if condition rather than
an upstream parser, matcher, or setup failure.
- Around line 12282-12308: The test currently assigns targets directly in the
pending-trigger loop, bypassing production target-selection. Replace the manual
TargetRef assignment with begin_pending_trigger_target_selection and apply the
resulting ChooseTarget/SelectTargets actions, asserting each trigger exposes its
own legal opponent-controlled-creature target prompt before pushing it onto the
stack. Ensure the scenario exercises the failure path through WaitingFor and the
normal apply pipeline, while preserving the attacker-to-blocker target mapping.
- Around line 1482-1502: Remove the unreachable! guard in the YouAttack branch
of the trigger processing flow. Reuse the result of matching_you_attack_pairs
directly, allowing non-AttackersDeclared events to produce an empty result
instead of panicking; preserve singleton_attack_events behavior for valid
attack-declaration events.

In `@crates/engine/src/parser/oracle_effect/imperative.rs`:
- Around line 11849-11861: Update the local parse_duration closure to accept an
optional trailing period after either supported duration phrase while remaining
all-consuming. Preserve the existing Duration mappings for “this turn if able”
and “this combat if able” so punctuated final clauses avoid the scan_contains
fallback and retain the attacker binding.

In `@crates/engine/src/parser/oracle_effect/subject.rs`:
- Around line 6806-6808: Strengthen the ForceBlock regression assertions: in
crates/engine/src/parser/oracle_effect/subject.rs lines 6806-6808, destructure
and assert the parsed duration and intended attacker value in addition to
target; in crates/engine/src/database/synthesis.rs lines 13579-13582, assert
attacker provenance is ForceBlockAttackerRef::Source and duration is
Duration::UntilEndOfCombat. Preserve each test’s existing target assertion.

---

Outside diff comments:
In `@client/src/components/board/ActionButton.tsx`:
- Around line 176-179: Update the pending-blocker reset effect in ActionButton
to also depend on the blocker prompt identity, such as waitingFor, not only
mode. Ensure pendingBlocker is cleared whenever a new DeclareBlockers prompt
replaces the previous one, while preserving the existing reset on mode changes.

In `@crates/engine/src/game/ability_scan.rs`:
- Around line 1146-1150: Update the ForceBlock arm in the effect scanner to
explicitly destructure every payload field instead of using .., and scan each
read-bearing attacker and duration value with the appropriate scanner, including
scan_duration. Preserve scanning target while combining all applicable axes so
ordering and loop classification remains fail-closed.

In `@crates/engine/src/parser/swallow_check.rs`:
- Around line 4092-4108: Remove TriggerCondition::SourceAttackedThisCombat from
the exemption matches in detect_duration_this_turn, leaving only variants whose
names contain ThisTurn. Preserve the existing exemptions and the separate
Effect::ForceBlock handling.

---

Nitpick comments:
In `@crates/engine/src/game/layers.rs`:
- Around line 4606-4615: Add a verified CR 400.7 annotation describing the
affected_recipient incarnation-liveness guard in transient_effect_is_live,
placing it alongside the guard and before or adjacent to the existing
UntilHostLeavesPlay rule check. Keep the implementation unchanged and ensure the
citation explains that the locked-in effect no longer applies when the
referenced object is a different incarnation.

In `@crates/engine/src/types/ability.rs`:
- Around line 11518-11529: Add a serde round-trip test near the existing
serialized-field tests for the ForceBlock variant, covering attacker set to
ForceBlockAttackerRef::EventSource and duration set to
Duration::UntilEndOfCombat. Also deserialize a legacy ForceBlock payload without
attacker or duration and assert the defaults preserve the generic attacker
behavior and UntilEndOfTurn lifetime.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 0067480f-bb18-47bd-8af1-f4fa856facb8

📥 Commits

Reviewing files that changed from the base of the PR and between b056ad6 and d8248ce.

📒 Files selected for processing (60)
  • client/src/adapter/types.ts
  • client/src/components/board/ActionButton.tsx
  • client/src/components/board/BlockAssignmentLines.tsx
  • client/src/components/board/GroupedPermanent.tsx
  • client/src/components/board/__tests__/ActionButton.test.tsx
  • client/src/components/board/__tests__/BlockAssignmentLines.test.ts
  • client/src/components/board/blockAssignmentVisibility.ts
  • client/src/components/combat/BlockerConstraintBadges.tsx
  • client/src/components/combat/__tests__/combatRequirements.test.tsx
  • client/src/components/combat/useBlockRequirements.ts
  • client/src/components/combat/useBlockerConstraints.ts
  • client/src/hooks/useRafPositions.ts
  • client/src/i18n/locales/de/game.json
  • client/src/i18n/locales/en/game.json
  • client/src/i18n/locales/es/game.json
  • client/src/i18n/locales/fr/game.json
  • client/src/i18n/locales/it/game.json
  • client/src/i18n/locales/pl/game.json
  • client/src/i18n/locales/pt/game.json
  • client/src/stores/__tests__/uiStore.test.ts
  • client/src/stores/uiStore.ts
  • client/src/viewmodel/__tests__/blockerSorting.test.ts
  • client/src/viewmodel/blockerSorting.ts
  • crates/engine/src/ai_support/candidates.rs
  • crates/engine/src/database/synthesis.rs
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/ability_utils.rs
  • crates/engine/src/game/combat.rs
  • crates/engine/src/game/combat_damage.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/effects/additional_phase.rs
  • crates/engine/src/game/effects/double.rs
  • crates/engine/src/game/effects/extra_turn.rs
  • crates/engine/src/game/effects/force_block.rs
  • crates/engine/src/game/effects/grant_extra_loyalty_activations.rs
  • crates/engine/src/game/effects/player_counter.rs
  • crates/engine/src/game/effects/reverse_turn_order.rs
  • crates/engine/src/game/effects/skip_next_step.rs
  • crates/engine/src/game/effects/skip_next_turn.rs
  • crates/engine/src/game/effects/vote.rs
  • crates/engine/src/game/engine_combat.rs
  • crates/engine/src/game/layers.rs
  • crates/engine/src/game/meld_tests.rs
  • crates/engine/src/game/stack.rs
  • crates/engine/src/game/triggers.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_effect/subject.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/parser/oracle_ir/ast.rs
  • crates/engine/src/parser/oracle_nom/target.rs
  • crates/engine/src/parser/oracle_trigger.rs
  • crates/engine/src/parser/oracle_trigger_tests.rs
  • crates/engine/src/parser/swallow_check.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/types/game_state.rs
  • crates/engine/src/types/statics.rs
  • crates/engine/tests/integration/the_chain_veil_loyalty_grants.rs
  • crates/phase-ai/src/search.rs
💤 Files with no reviewable changes (1)
  • client/src/components/board/GroupedPermanent.tsx

Comment thread client/src/components/board/__tests__/BlockAssignmentLines.test.ts
Comment on lines +11 to +13
return pairs.filter(([blockerId]) => {
const blockerController = objects[String(blockerId)]?.controller;
return blockerController == null || visiblePlayerIds.has(blockerController);

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift

Move assignment-visibility filtering into the engine.

Lines 11-13 derive which blocker assignments are visible from game-object controller data in the client. Expose viewer-filtered blocker pairs through the engine/adapters instead, so every transport renders the same authorized state.

As per coding guidelines and path instructions, the frontend must only render engine-provided state and must not calculate, filter, derive, or reinterpret game data.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@client/src/components/board/blockAssignmentVisibility.ts` around lines 11 -
13, Remove the client-side controller-based filtering from the
assignment-visibility logic around the blocker-pair filter. Add or use an
engine/adapter API that returns blocker pairs already filtered for the viewing
player, and have the frontend render that engine-provided collection directly
without inspecting objects, controllers, or visiblePlayerIds.

Sources: Coding guidelines, Path instructions

Comment thread client/src/components/combat/useBlockerConstraints.ts Outdated
Comment thread client/src/components/combat/useBlockRequirements.ts Outdated
Comment thread crates/engine/src/database/synthesis.rs Outdated
Comment thread crates/engine/src/game/triggers.rs
Comment thread crates/engine/src/game/triggers.rs
Comment thread crates/engine/src/game/triggers.rs Outdated
Comment thread crates/engine/src/parser/oracle_effect/imperative.rs
Comment thread crates/engine/src/parser/oracle_effect/subject.rs Outdated

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (2)
crates/engine/src/database/synthesis.rs (2)

5039-5043: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Match the complete Provoke force-block shape.

Using .. ignores both attacker and duration. Because this predicate is used for Provoke idempotency, an unrelated optional attack trigger with ParentTarget force-blocking could be mistaken for Provoke and prevent the real Provoke trigger from being synthesized.

Match Source and UntilEndOfCombat explicitly.

As per path instructions, typed exact-attacker axes must remain explicit and composable.

Proposed fix
- Effect::ForceBlock {
-     target: TargetFilter::ParentTarget,
-     ..
- }
+ Effect::ForceBlock {
+     target: TargetFilter::ParentTarget,
+     attacker: Some(crate::types::ability::ForceBlockAttackerRef::Source),
+     duration: Duration::UntilEndOfCombat,
+ }
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/database/synthesis.rs` around lines 5039 - 5043, Update the
ForceBlock pattern in the Provoke idempotency predicate to match attacker as
Source and duration as UntilEndOfCombat explicitly, while retaining
TargetFilter::ParentTarget. Do not use `..`; preserve the existing typed
exact-attacker axes so only the complete Provoke force-block shape is
recognized.

Source: Path instructions


13903-13918: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Assert the complete forced-block effect, not independent fields.

Comparing only attacker.object_id does not validate the incarnation, and the separate .any() duration check can pass because of an unrelated transient effect. Locate the effect containing MustBlockAttacker, then assert its full ObjectIncarnationRef and UntilEndOfCombat duration.

As per path instructions, the force-block regression test must verify exact attacker/event-bound behavior.

🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/database/synthesis.rs` around lines 13903 - 13918, The
force-block regression assertions must validate one complete effect rather than
independent properties. Update the test around the `forced` assertion to locate
the transient continuous effect containing `MustBlockAttacker`, then assert that
effect’s full `ObjectIncarnationRef` matches the provoking attacker and that its
duration is `UntilEndOfCombat`; remove the separate unrelated duration check.

Source: Path instructions

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Outside diff comments:
In `@crates/engine/src/database/synthesis.rs`:
- Around line 5039-5043: Update the ForceBlock pattern in the Provoke
idempotency predicate to match attacker as Source and duration as
UntilEndOfCombat explicitly, while retaining TargetFilter::ParentTarget. Do not
use `..`; preserve the existing typed exact-attacker axes so only the complete
Provoke force-block shape is recognized.
- Around line 13903-13918: The force-block regression assertions must validate
one complete effect rather than independent properties. Update the test around
the `forced` assertion to locate the transient continuous effect containing
`MustBlockAttacker`, then assert that effect’s full `ObjectIncarnationRef`
matches the provoking attacker and that its duration is `UntilEndOfCombat`;
remove the separate unrelated duration check.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 8baa41d2-ee63-4da4-a6b1-d38e0d2cb304

📥 Commits

Reviewing files that changed from the base of the PR and between d8248ce and 9d2e929.

📒 Files selected for processing (18)
  • client/src/components/board/ActionButton.tsx
  • client/src/components/board/__tests__/ActionButton.test.tsx
  • client/src/components/board/__tests__/BlockAssignmentLines.test.ts
  • client/src/components/combat/BlockRequirementBadges.tsx
  • client/src/components/combat/BlockerConstraintBadges.tsx
  • client/src/components/combat/__tests__/combatRequirements.test.tsx
  • client/src/components/combat/useBlockRequirements.ts
  • client/src/components/combat/useBlockerConstraints.ts
  • crates/engine/src/database/synthesis.rs
  • crates/engine/src/game/ability_rw.rs
  • crates/engine/src/game/ability_scan.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/game/layers.rs
  • crates/engine/src/game/triggers.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • crates/engine/src/parser/oracle_effect/subject.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/types/ability.rs
💤 Files with no reviewable changes (1)
  • client/src/components/combat/BlockerConstraintBadges.tsx
🚧 Files skipped from review as they are similar to previous changes (10)
  • client/src/components/board/tests/BlockAssignmentLines.test.ts
  • crates/engine/src/game/layers.rs
  • crates/engine/src/parser/oracle_effect/imperative.rs
  • client/src/components/combat/tests/combatRequirements.test.tsx
  • crates/engine/src/game/triggers.rs
  • crates/engine/src/parser/oracle_effect/tests.rs
  • crates/engine/src/game/coverage.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/src/game/ability_rw.rs
  • client/src/components/board/ActionButton.tsx

@matthewevans
matthewevans added this pull request to the merge queue Jul 25, 2026
Merged via the queue into main with commit 616d5fd Jul 25, 2026
19 checks passed
@matthewevans
matthewevans deleted the ship/enforce-event-bound-forced-blocks branch July 25, 2026 03:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant