fix(engine): release trigger carrier on drop and unstick empty attackers - #7051
Merged
matthewevans merged 2 commits intoAug 6, 2026
Merged
Conversation
Two independent stalls reported from one game state, both fixed here.
They share a commit because the CR 603.5 prompt-census pin below is only
correct once BOTH are applied.
CR 603.3d — leaked trigger event batch:
`drop_mid_construction_pending_trigger` released `pending_trigger` and
`pending_trigger_firing` but left `pending_trigger_event_batch` populated.
Because `begin_pending_trigger_target_selection` re-reads that batch as the
event context for whichever trigger is being constructed, and the pause
paths write it straight back, a single no-legal-target drop latched a dead
event into the game state permanently. `resolve_proven_inert_trigger_batch`
gates on `inert_trigger_batch_state_is_settled`, which requires an empty
batch, so batch auto-resolve ("resolve all") stayed disabled for the rest of
the game. The function is now the single authority for releasing all four
in-flight construction cursors, and the four duplicated inline drop blocks
collapse into calls to it.
CR 508.1a — forced empty attack declaration parked the game:
`run_auto_pass_loop`'s `DeclareAttackers` arm only auto-submitted under
`AutoPassMode::UntilTurnBoundary`. With no auto-pass configured, a player
controlling zero creatures was parked on a Declare Attackers prompt whose
entire legal-action set was a single no-op submission, and the loop fell
through to `_ => break`. The arm now also fires when `valid_attacker_ids` is
empty, mirroring the `DeclareBlockers` arm, which already auto-submits when
there is nothing to choose. A phase stop still overrides both.
Also updates the CR 603.5 prompt-census pin `game/engine.rs:11697` ->
`:11712` with a drift-log entry. Pure line movement: the producer is
sha256-identical (`8a544e878d3e77fb...`), still the only line in the file
matching the producer shape outside `#[cfg(test)]`, still inside
`begin_pending_trigger_target_selection`, and the set is unchanged at five
producers (total 37, partition 5/7/25).
Both fixes were verified with negative controls in isolated target dirs:
reverted -> red on the intended assertion -> restored -> green.
matthewevans
enabled auto-merge
August 6, 2026 00:52
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe engine now auto-submits empty attacker declarations when no legal attackers exist and no phase stop is active. Pending-trigger cleanup is centralized and clears ChangesEngine state-flow fixes
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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 `@crates/engine/src/game/engine_trigger_target_tests.rs`:
- Around line 1368-1370: Extend the assertions in the pending-trigger cleanup
test to verify that entry_id is absent from both state.stack and
state.stack_trigger_firings, alongside the existing pending-trigger assertions.
Keep the test focused on confirming removal of the illegal triggered ability
from all relevant stack state.
In `@crates/engine/src/game/engine.rs`:
- Around line 11526-11530: Update the annotation associated with the
mid-construction triggered-ability removal logic, retaining CR 603.3c and CR
603.3d while removing the incorrect CR 608.2c citation. Do not alter the
underlying behavior or other explanatory text.
🪄 Autofix
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: 4753ad7e-5eae-4403-9d40-cf7f8db46bfc
📒 Files selected for processing (3)
crates/engine/src/game/engine.rscrates/engine/src/game/engine_auto_pass_decision_tests.rscrates/engine/src/game/engine_trigger_target_tests.rs
Comment on lines
+1368
to
+1370
| assert!(state.pending_trigger.is_none()); | ||
| assert!(state.pending_trigger_entry.is_none()); | ||
| assert!(state.pending_trigger_firing.is_none()); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert that the illegal stack entry is removed.
Line 1368 only verifies that the pending cursor is cleared. Add assertions that entry_id is absent from state.stack and state.stack_trigger_firings. Otherwise, this regression passes if cleanup leaves the illegal triggered ability on the stack. CR 603.3d requires the ability to be removed from the stack. (media.wizards.com)
🤖 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/engine_trigger_target_tests.rs` around lines 1368 -
1370, Extend the assertions in the pending-trigger cleanup test to verify that
entry_id is absent from both state.stack and state.stack_trigger_firings,
alongside the existing pending-trigger assertions. Keep the test focused on
confirming removal of the illegal triggered ability from all relevant stack
state.
Source: Path instructions
Comment on lines
+11526
to
+11530
| /// CR 603.3c + CR 603.3d + CR 608.2c: Single authority for dropping a | ||
| /// mid-construction triggered ability — an optional modal declined before mode | ||
| /// choice, or CR 603.3d's "if a choice is required when the triggered ability | ||
| /// goes on the stack but no legal choices can be made for it ... the ability is | ||
| /// simply removed from the stack." |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the CR citation.
Line 11526 cites CR 608.2c for a path that removes an ability while it is being put on the stack. CR 608.2c governs following written instructions during resolution. Keep CR 603.3c and CR 603.3d, and remove CR 608.2c from this annotation. (blogs.magicjudges.org)
Based on learnings: “Cite CR 608.2c only when the comment is documenting the resolution of written instructions ‘in order’.”
🤖 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/engine.rs` around lines 11526 - 11530, Update the
annotation associated with the mid-construction triggered-ability removal logic,
retaining CR 603.3c and CR 603.3d while removing the incorrect CR 608.2c
citation. Do not alter the underlying behavior or other explanatory text.
Sources: Path instructions, Learnings
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
`debug_set_summoning_sickness_removes_from_valid_attackers` drove its only eligible attacker sick, emptying `valid_attacker_ids` while parked at the prompt. The CR 508.1a empty-attackers guard then auto-submitted the sole legal (empty) declaration and advanced to priority, so the refreshed `DeclareAttackers` the test exists to inspect no longer existed. Add a second eligible attacker so the refreshed snapshot stays non-empty and the declaration is not forced. In real play this transition is unreachable: declaring attackers is a turn-based action (CR 508.1) with no priority window mid-declaration, so `valid_attacker_ids` cannot go non-empty -> empty while a player is parked at the prompt; emptiness is decided when the prompt is built. Only the debug sandbox can produce it. The paired assertion is also strictly sharper: it now proves the refresh dropped *that* creature while retaining the untouched one, which an emptied set cannot distinguish from a snapshot that simply cleared everything.
matthewevans
deleted the
ship/fix-engine-trigger-carrier-and-empty-attackers
branch
August 6, 2026 02:19
lgray
pushed a commit
to lgray/phase
that referenced
this pull request
Aug 6, 2026
…#6826 cost (phase-rs#7053) phase-rs#7044 refreshed the baseline onto the phase-rs#6826 regression itself (layers_full_eval 3495 -> 15877). Once phase-rs#7049 fixed that regression the gate carried ~3x slack -- real cost 5464 against a 15877 baseline under a ~6% band -- so it could no longer catch a new decision-cost regression. Re-measured on a clean worktree against an immutable card-data snapshot (scoped card_data_hash 670a4a14, covering 46/46 scenario deck cards): counter | old | new | threshold layers_full_eval | 15877 | 5464 | 5801 state_clone_for_legality | 19342 | 11099 | 11717 restriction_static_mode_gate_scans | 155747 | 110891 | 116499 sba_battlefield_snapshot_builds | 27462 | 19626 | 20671 scripts/validate-ai-perf-reproducibility.sh: PASSED (margin+band). 25/25 band runs clean, "0 OVER-MARGIN of 29 counters", and worst_current == baseline on every counter across 125 cold processes. CI budget: T_run_max 90s * 2.5 + T_build ~630s = ~14.3 min < 25 min. (The script reported T_build=1s from a warm cache; the 630s figure is the cold isolated server-release build measured separately, which is the conservative ceiling the check intends.) git_sha stamps 7f5f0b4, where the 25-run validation ran. Re-confirmed against f26f4e3 (current main, including phase-rs#7051 and phase-rs#7017): 0 FAIL, 29 PASS, +0 on every counter. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This was referenced Aug 7, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Two independent stalls reported from one game state, both fixed here.
They share a commit because the CR 603.5 prompt-census pin below is only
correct once BOTH are applied.
CR 603.3d — leaked trigger event batch:
drop_mid_construction_pending_triggerreleasedpending_triggerandpending_trigger_firingbut leftpending_trigger_event_batchpopulated.Because
begin_pending_trigger_target_selectionre-reads that batch as theevent context for whichever trigger is being constructed, and the pause
paths write it straight back, a single no-legal-target drop latched a dead
event into the game state permanently.
resolve_proven_inert_trigger_batchgates on
inert_trigger_batch_state_is_settled, which requires an emptybatch, so batch auto-resolve ("resolve all") stayed disabled for the rest of
the game. The function is now the single authority for releasing all four
in-flight construction cursors, and the four duplicated inline drop blocks
collapse into calls to it.
CR 508.1a — forced empty attack declaration parked the game:
run_auto_pass_loop'sDeclareAttackersarm only auto-submitted underAutoPassMode::UntilTurnBoundary. With no auto-pass configured, a playercontrolling zero creatures was parked on a Declare Attackers prompt whose
entire legal-action set was a single no-op submission, and the loop fell
through to
_ => break. The arm now also fires whenvalid_attacker_idsisempty, mirroring the
DeclareBlockersarm, which already auto-submits whenthere is nothing to choose. A phase stop still overrides both.
Also updates the CR 603.5 prompt-census pin
game/engine.rs:11697->:11712with a drift-log entry. Pure line movement: the producer issha256-identical (
8a544e878d3e77fb...), still the only line in the filematching the producer shape outside
#[cfg(test)], still insidebegin_pending_trigger_target_selection, and the set is unchanged at fiveproducers (total 37, partition 5/7/25).
Both fixes were verified with negative controls in isolated target dirs:
reverted -> red on the intended assertion -> restored -> green.
Summary by CodeRabbit
Bug Fixes
Tests