cr733(p2): journal logical-zone trigger collection occurrences#6536
Conversation
📝 WalkthroughWalkthroughAdds resolved trigger collection commands to the rules journal, applies them during normal execution and replay, routes zone-change trigger handling through the journal, and adds integration coverage for serialization, ordering, no-ops, causes, and ChangeZone behavior. ChangesResolved trigger collection journaling
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant ChangeZoneResolver
participant TriggerHelpers
participant GameState
participant RulesJournal
ChangeZoneResolver->>TriggerHelpers: resolve trigger collection
TriggerHelpers->>GameState: apply contexts or occurrences
TriggerHelpers->>RulesJournal: record TriggerCollection command
RulesJournal-->>TriggerHelpers: command ordinal
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/tests/integration/cr733_resolved_trigger_collection.rs (1)
1-228: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffNo production-pipeline test for the
ConsumeBeforePrioritypath.
real_change_zone_resolver_journals_its_collected_trigger_occurrencesnicely drivesDeferPendingthrough the realChangeZoneresolver with a positive reach-guard.ConsumeBeforePriority(the other half of this PR's wiring, replacing the direct extend inmark_logical_zone_events_consumed_before_priority) is only exercised via hand-built commands passed straight toapply_resolved_trigger_collection, never through the real engine flow that callsmark_logical_zone_events_consumed_before_priority. Consider adding a scenario-driven test that reaches that call site.As per path instructions, "Test adequacy is the highest-frequency contributor finding... a parser AST shape test does NOT prove runtime semantics" for
crates/engine/tests/**.🤖 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/tests/integration/cr733_resolved_trigger_collection.rs` around lines 1 - 228, Add a scenario-driven integration test that exercises the production flow invoking mark_logical_zone_events_consumed_before_priority and verifies it journals a non-empty ConsumeBeforePriority ResolvedTriggerCollection. Use a real game event/resolution path rather than directly calling apply_resolved_trigger_collection, and assert the collected occurrence is recorded in the journal with the expected cause and state effect.
🤖 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/types/resolved_commands.rs`:
- Around line 1644-1650: Update the ResolvedRulesCommand::TriggerCollection arm
in validate_resolved_command to reject empty ResolvedTriggerCollection payloads
by checking that command.contexts is non-empty before validating the cause.
Return the same invalid-command error pattern used by the sibling arms for no-op
payloads, while preserving the existing unrelated-cause validation for non-empty
collections.
- Around line 288-303: Update validate_resolved_command to reject empty
ResolvedTriggerCollection payloads for both DeferPending.contexts and
ConsumeBeforePriority.occurrences, matching the existing validation behavior for
other resolved-command variants while continuing to accept non-empty
collections.
---
Nitpick comments:
In `@crates/engine/tests/integration/cr733_resolved_trigger_collection.rs`:
- Around line 1-228: Add a scenario-driven integration test that exercises the
production flow invoking mark_logical_zone_events_consumed_before_priority and
verifies it journals a non-empty ConsumeBeforePriority
ResolvedTriggerCollection. Use a real game event/resolution path rather than
directly calling apply_resolved_trigger_collection, and assert the collected
occurrence is recorded in the journal with the expected cause and state effect.
🪄 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: 5faa924a-1af2-4d2a-8366-ef62ed0e1f59
📒 Files selected for processing (6)
crates/engine/src/game/triggers.rscrates/engine/src/types/mod.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rscrates/engine/tests/integration/cr733_resolved_trigger_collection.rscrates/engine/tests/integration/main.rs
| /// Exact trigger occurrences collected at one logical trigger/LKI boundary. | ||
| /// | ||
| /// CR 603.2 + CR 603.3b: collected trigger contexts retain their already | ||
| /// determined firing and placement order. CR 603.10 + CR 603.10a: final | ||
| /// logical zone-change settlement uses the recorded pre-event authority. | ||
| /// CR 603.2c: consumed event occurrences prevent the generic priority scan | ||
| /// from collecting the same occurrence a second time. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub enum ResolvedTriggerCollection { | ||
| DeferPending { | ||
| contexts: Vec<PendingTriggerContext>, | ||
| }, | ||
| ConsumeBeforePriority { | ||
| occurrences: Vec<ConsumedTriggerEventOccurrence>, | ||
| }, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
fd -i MagicCompRules.txt
rg -n "^603\.2c" -A3 docs/MagicCompRules.txt
rg -n "^603\.3b" -A3 docs/MagicCompRules.txt
rg -n "^603\.10a?\b" -A3 docs/MagicCompRules.txtRepository: phase-rs/phase
Length of output: 509
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Top-level files:\n'
git ls-files | sed -n '1,120p' | head -n 120
printf '\nPossible rules text files:\n'
fd -i 'MagicCompRules.txt|CompRules|ComprehensiveRules|rules' . -t fRepository: phase-rs/phase
Length of output: 5198
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'Relevant files in crates/engine/src/types:\n'
fd -i 'resolved_commands.rs|triggers.rs' crates/engine/src/types crates/engine/src -t f
printf '\nOutline resolved_commands.rs:\n'
ast-grep outline crates/engine/src/types/resolved_commands.rs --view expanded
printf '\nOutline triggers.rs:\n'
ast-grep outline crates/engine/src/triggers.rs --view expandedRepository: phase-rs/phase
Length of output: 13854
🏁 Script executed:
#!/bin/bash
set -euo pipefail
git ls-files | rg -n 'MagicCompRules|CompRules|ComprehensiveRules|(^|/)docs/'Repository: phase-rs/phase
Length of output: 270
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf 'resolved_commands.rs excerpt:\n'
sed -n '280,340p' crates/engine/src/types/resolved_commands.rs
printf '\nvalidate_resolved_command excerpt:\n'
sed -n '1628,1665p' crates/engine/src/types/resolved_commands.rs
printf '\nrecord_trigger_collection excerpt:\n'
sed -n '1188,1206p' crates/engine/src/types/resolved_commands.rsRepository: phase-rs/phase
Length of output: 5058
🌐 Web query:
Magic: The Gathering Comprehensive Rules 603.2c exact text
💡 Result:
The exact text of section 603.2c of the Magic: The Gathering Comprehensive Rules is: 603.2c An ability triggers only once each time its trigger event occurs. However, it can trigger repeatedly if one event contains multiple occurrences. [1][2]
Citations:
- 1: https://mtg-rules.vercel.app/chapters/603
- 2: https://ancestral.vision/spells-abilities-and-effects/handling-triggered-abilities.html
🌐 Web query:
site:magic.wizards.com Comprehensive Rules 603.2c 603.3b 603.10a trigger occurrence stack placement look-back
💡 Result:
In the Magic: The Gathering Comprehensive Rules, sections 603.2c, 603.3b, and 603.10a describe the mechanics of how triggered abilities occur, are placed on the stack, and interact with the game state. Rule 603.2c states that an ability triggers each time its trigger event occurs, even if the event happens during the resolution of another spell or ability. Triggered abilities are then handled by the game system regardless of when the triggering event takes place. Rule 603.3b details that when a triggered ability is put onto the stack, the controller of that ability is the player who controlled the source of the ability at the time it triggered. If the source is no longer on the battlefield, the game uses the last known information (the controller's status) to determine ownership on the stack. Rule 603.10a explains the "look-back" or state-dependent nature of triggers. It clarifies that if an ability's trigger event depends on the game state (such as a creature leaving the battlefield or an object changing zones), the game checks the state of the game immediately before or after the event to verify if the trigger conditions were met. This ensures that triggers like "when a creature dies" correctly identify the object that left the battlefield even though it is no longer in the zone where the trigger occurred. These rules collectively ensure that triggered abilities are reliably accounted for, assigned to the correct controller, and triggered based on the game state at the appropriate moment, maintaining consistency even during complex interactions where objects move between zones or leave play.
Citations:
- 1: https://magic.wizards.com/en/news/feature/kaladesh-update-bulletin-2016-09-30
- 2: https://magic.wizards.com/en/news/feature/stack-and-its-tricks-2017-11-30
- 3: https://magic.wizards.com/en/news/feature/dynamics-turn-2006-11-04
- 4: https://magic.wizards.com/en/news/announcements/comprehensive-rules-changes-2020-09-22
- 5: https://magic.wizards.com/en/news/feature/sequencing-2015-02-16
Reject empty TriggerCollection payloads
validate_resolved_command should reject zero-length collections like the other resolved-command variants; otherwise a malformed journal entry can pass replay validation even though production never records one.
🤖 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/resolved_commands.rs` around lines 288 - 303, Update
validate_resolved_command to reject empty ResolvedTriggerCollection payloads for
both DeferPending.contexts and ConsumeBeforePriority.occurrences, matching the
existing validation behavior for other resolved-command variants while
continuing to accept non-empty collections.
Source: Path instructions
| ResolvedRulesCommand::TriggerCollection(command) => { | ||
| if entry.node != command.cause { | ||
| return Err(ResolvedRulesJournalError::InvalidSerializedAuthority( | ||
| "trigger-collection command has an unrelated cause".to_string(), | ||
| )); | ||
| } | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
validate_resolved_command's TriggerCollection arm doesn't reject empty collections, unlike every sibling variant.
Every other arm here rejects a no-op payload (player_edit_is_empty, command.expected_old == command.new, object_counter_edit_is_empty, information_command_is_invalid, ledger_edit_is_invalid). Production code (resolve_and_apply_trigger_collection in triggers.rs) short-circuits empty ResolvedTriggerCollection before ever recording it, but record_trigger_collection is pub with no such guard, and this validator doesn't enforce it either — so a deserialized/tampered journal with an empty TriggerCollection { contexts: vec![] } entry silently passes validate_serialized_authority, breaking the invariant this file otherwise enforces for every other command.
🛡️ Proposed fix
+impl ResolvedTriggerCollection {
+ fn is_empty(&self) -> bool {
+ match self {
+ Self::DeferPending { contexts } => contexts.is_empty(),
+ Self::ConsumeBeforePriority { occurrences } => occurrences.is_empty(),
+ }
+ }
+}
+
ResolvedRulesCommand::TriggerCollection(command) => {
- if entry.node != command.cause {
+ if entry.node != command.cause || command.collection.is_empty() {
return Err(ResolvedRulesJournalError::InvalidSerializedAuthority(
"trigger-collection command has an unrelated cause".to_string(),
));
}
}🤖 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/resolved_commands.rs` around lines 1644 - 1650,
Update the ResolvedRulesCommand::TriggerCollection arm in
validate_resolved_command to reject empty ResolvedTriggerCollection payloads by
checking that command.contexts is non-empty before validating the cause. Return
the same invalid-command error pattern used by the sibling arms for no-op
payloads, while preserving the existing unrelated-cause validation for non-empty
collections.
Parse changes introduced by this PR✓ No card-parse changes detected. |
…-rs#6536) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
1 participant
Summary by CodeRabbit
New Features
Bug Fixes