feat(engine): journal CR 603.7 delayed-trigger and CR 611.2a continuous-effect installs - #6649
Merged
Merged
Conversation
…us-effect installs
Adds the modifier-installation half of the CR 733 resolved-command journal:
every delayed triggered ability and every transient continuous effect now
resolves, applies, and journals through a single authority, with a paired
replay applier that installs the exact recorded values.
Two variants, not one parameterized variant
-------------------------------------------
`ResolvedRulesCommand` gains `DelayedTriggerInstall` and
`ContinuousEffectInstall` as siblings. CLAUDE.md's categorical-boundary rule
decides this: the parameterization axis would straddle two CR sections the
engine resolves through entirely separate machinery.
- A delayed triggered ability is CR 603.7. It never touches the CR 613 layer
system; it waits in `delayed_triggers` until its condition occurs, then
goes on the stack as an ordinary triggered ability (CR 603.7b). It draws
NO allocator value.
- A transient continuous effect is CR 611.2a. It never uses the stack; it
applies continuously through the CR 613 layers until its duration ends. It
draws TWO allocator values -- an effect id, and a CR 613.7b timestamp that
orders it within its layer.
Their `expected_*`/`resulting_*` shapes therefore differ in kind, not in a leaf
value: one command carries an allocator receipt to verify, the other has
nothing to verify. Collapsing them would put two unrelated invariants behind
one validator arm and one applier. This is the categorical-boundary failure
CLAUDE.md warns about, not the sibling-cluster smell it warns about -- the two
share no name root, no context label, and no comparator/scope axis.
Authorities
-----------
Delayed triggers had 11 raw `delayed_triggers.push` sites across 10 production
files and no authority; `game::triggers::install_delayed_trigger` is new and
now owns all of them. Transient continuous effects already had exactly one
production insertion authority -- `GameState::add_transient_continuous_effect`
-- so that half is journaling only, no extraction.
Replay installs the whole `DelayedTrigger` / `TransientContinuousEffect`
verbatim: the CR 603.7c bound ability with its targets, the CR 611.2c fixed
affected set, and both allocator draws. Nothing is re-selected, re-read, or
re-drawn. `apply_resolved_continuous_effect` advances `next_continuous_effect_id`
and `next_timestamp` past the installed values, mirroring how the token-birth
family advances `next_object_id`, so a replayed state cannot hand the same id
or timestamp out twice.
Both appliers verify their `expected_installed_count` against live state and
return a typed error before mutating anything; the continuous-effect applier
additionally rejects an id already live.
Scope
-----
Installation only. Both collections also have a removal/expiry half (delayed
triggers: `collect_matching_delayed_triggers` remove + cleanup retain;
continuous effects: the layers.rs duration-prune family), which remains
unjournaled and is not covered here.
matthewevans
enabled auto-merge
July 26, 2026 04:50
Contributor
📝 WalkthroughWalkthroughDelayed-trigger and transient continuous-effect installations now flow through resolved commands, journaling, and replay invariant checks. Existing delayed-trigger creation sites use the centralized installer, while integration tests cover exact replay and fail-closed mismatches. ChangesResolved installation authority
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Resolution
participant GameState
participant Journal
participant Replay
Resolution->>GameState: Install delayed trigger or continuous effect
GameState->>Journal: Record resolved command
Replay->>GameState: Apply recorded command
GameState-->>Replay: Validate preconditions and state updates
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/src/types/game_state.rs (1)
16672-16682: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winConsider a symmetric live-timestamp uniqueness guard.
The duplicate check covers
effect.idbut noteffect.timestamp. Per CR 613.7 the timestamp is what orders an effect within its layer, so a diverged replay that installs an effect sharing a live timestamp yields an ambiguous layer order that no later check catches — the same class of failure the id guard fails closed on. Fails closed before mutation, same as the id check.♻️ Symmetric timestamp guard
if self .transient_continuous_effects .iter() .any(|effect| effect.id == command.effect.id) { return Err( ResolvedContinuousEffectReplayInvariantError::DuplicateEffectId(command.effect.id), ); } + // CR 613.7: the timestamp is the intra-layer ordering key, so two live + // effects sharing one leave the layer order undefined. + if self + .transient_continuous_effects + .iter() + .any(|effect| effect.timestamp == command.effect.timestamp) + { + return Err( + ResolvedContinuousEffectReplayInvariantError::DuplicateTimestamp( + command.effect.timestamp, + ), + ); + }Requires a matching
DuplicateTimestamp(u64)variant onResolvedContinuousEffectReplayInvariantError.🤖 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/game_state.rs` around lines 16672 - 16682, Extend the pre-mutation validation in the transient continuous-effect replay path alongside the existing effect.id check to reject any live effect whose timestamp matches command.effect.timestamp. Add and use the matching DuplicateTimestamp(u64) variant on ResolvedContinuousEffectReplayInvariantError, preserving the fail-closed behavior before state mutation.
🤖 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 2302-2316: The ResolvedRulesCommand::ContinuousEffectInstall
validation currently accepts any lower allocator receipt, allowing replay to
advance allocators incorrectly. Replace the upper-bound checks with strict
equality against the authoritative allocator results for this installation, and
reject allocator arithmetic overflow; retain validation that entry.node matches
command.cause and return InvalidSerializedAuthority for mismatches.
- Around line 247-299: The install-position receipts in
ResolvedDelayedTriggerCommand and the transient-effect installation flow are not
replay-stable when prior delayed triggers fire or transient effects expire.
Remove or replace these expected count precondition checks with authoritative
replay of the corresponding removal/expiry lifecycle transitions, while
preserving exact allocator advancement and installed-value behavior; do not
merely weaken the validation.
---
Nitpick comments:
In `@crates/engine/src/types/game_state.rs`:
- Around line 16672-16682: Extend the pre-mutation validation in the transient
continuous-effect replay path alongside the existing effect.id check to reject
any live effect whose timestamp matches command.effect.timestamp. Add and use
the matching DuplicateTimestamp(u64) variant on
ResolvedContinuousEffectReplayInvariantError, preserving the fail-closed
behavior before state mutation.
🪄 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: ae0acd0d-3f6c-43af-b417-2c31167b513a
📒 Files selected for processing (17)
crates/engine/src/game/blitz.rscrates/engine/src/game/dash.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/encore.rscrates/engine/src/game/effects/exile_resolving_spell.rscrates/engine/src/game/effects/myriad.rscrates/engine/src/game/effects/rebound.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/stack.rscrates/engine/src/game/triggers.rscrates/engine/src/types/game_state.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/cr733_resolved_commands_p2.rscrates/engine/tests/integration/cr733_resolved_draw.rscrates/engine/tests/integration/cr733_resolved_modifier_install.rscrates/engine/tests/integration/main.rs
Comment on lines
+247
to
+299
| /// `expected_installed_count` is the length of `GameState::delayed_triggers` | ||
| /// immediately before the push. Installed triggers are consumed by | ||
| /// `check_delayed_triggers` (CR 603.7b, one firing) and pruned at cleanup, so | ||
| /// the live length at install time is a genuine function of everything the | ||
| /// replayed prefix did. Verifying it fails a replay closed the moment journal | ||
| /// order stops matching execution order. (It is a storage-position check only: | ||
| /// the rules order in which simultaneously firing triggers reach the stack is | ||
| /// chosen by their controller under CR 603.3b, not by this index.) | ||
| /// | ||
| /// No allocator value is drawn: unlike a continuous effect (CR 613.7b) a | ||
| /// delayed triggered ability takes no timestamp, because it does not | ||
| /// participate in the CR 613 layer system until it actually triggers and goes | ||
| /// on the stack as an ordinary triggered ability. | ||
| #[derive(Debug, Clone, PartialEq, Eq, Serialize, Deserialize)] | ||
| pub struct ResolvedDelayedTriggerCommand { | ||
| pub trigger: DelayedTrigger, | ||
| pub expected_installed_count: usize, | ||
| pub cause: RulesExecutionNodeRef, | ||
| } | ||
|
|
||
| /// Typed failure while applying one already-resolved delayed-trigger install. | ||
| #[derive(Debug, Clone, PartialEq, Eq, thiserror::Error)] | ||
| pub enum ResolvedDelayedTriggerReplayInvariantError { | ||
| #[error("delayed-trigger install precondition mismatch: expected {expected} already installed, found {found}")] | ||
| InstalledCountPreconditionMismatch { expected: usize, found: usize }, | ||
| } | ||
|
|
||
| /// One exact CR 611.2a transient continuous-effect installation. | ||
| /// | ||
| /// A continuous effect generated by the resolution of a spell or ability lasts | ||
| /// as long as that spell or ability stated (CR 611.2a) and, per CR 611.2c, the | ||
| /// set of objects it affects is fixed when it begins. Both of those decisions | ||
| /// are already baked into the `TransientContinuousEffect` the authority built, | ||
| /// so the effect is recorded whole rather than as a recipe to re-evaluate. | ||
| /// | ||
| /// Two allocator draws live inside that value and MUST be installed rather than | ||
| /// re-drawn: | ||
| /// - `effect.timestamp`, taken from `GameState::next_timestamp` per CR 613.7b | ||
| /// ("a continuous effect generated by the resolution of a spell or ability | ||
| /// receives a timestamp at the time it's created"). Re-drawing it at replay | ||
| /// would reorder the effect against every other effect in its CR 613 layer. | ||
| /// - `effect.id`, taken from `GameState::next_continuous_effect_id`, which is | ||
| /// the handle later duration/recipient binding addresses the effect by. | ||
| /// | ||
| /// Both are carried as post-draw high-water marks | ||
| /// (`resulting_next_continuous_effect_id` / `resulting_next_timestamp`) so | ||
| /// replay advances the allocators past the installed values exactly the way the | ||
| /// token-birth family advances `next_object_id`, rather than leaving a | ||
| /// replayed state that would hand the same id or timestamp out twice. | ||
| /// | ||
| /// `expected_installed_count` mirrors the delayed-trigger command: the live | ||
| /// length of `GameState::transient_continuous_effects` before the push, which | ||
| /// duration expiry continuously shortens. |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
Journal lifecycle removals/expiries before relying on install-position receipts.
These counts cease to be replay-stable once a delayed trigger fires or a transient effect expires: replay retains the prior install, while live execution removes it. A later install recorded with count 0 then fails closed. Record the corresponding removal/expiry transitions (or replay them through an equivalent authoritative lifecycle path); do not weaken the receipt check.
Based on the PR objective that removal and expiry journaling are out of scope, this breaks the stated replay invariant.
🤖 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 247 - 299, The
install-position receipts in ResolvedDelayedTriggerCommand and the
transient-effect installation flow are not replay-stable when prior delayed
triggers fire or transient effects expire. Remove or replace these expected
count precondition checks with authoritative replay of the corresponding
removal/expiry lifecycle transitions, while preserving exact allocator
advancement and installed-value behavior; do not merely weaken the validation.
Source: Path instructions
Comment on lines
+2302
to
+2316
| ResolvedRulesCommand::ContinuousEffectInstall(command) => { | ||
| // CR 613.7b: the effect's timestamp was drawn when it was | ||
| // created, so it — and the effect id drawn alongside it — must | ||
| // lie strictly below the high-water the draw left behind, or the | ||
| // receipt describes an allocation that never happened. | ||
| if entry.node != command.cause | ||
| || command.effect.id >= command.resulting_next_continuous_effect_id | ||
| || command.effect.timestamp >= command.resulting_next_timestamp | ||
| { | ||
| return Err(ResolvedRulesJournalError::InvalidSerializedAuthority( | ||
| "continuous-effect install command has an impossible allocator receipt, \ | ||
| or an unrelated cause" | ||
| .to_string(), | ||
| )); | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Validate exact allocator receipts, not merely upper bounds.
A forged serialized command can use effect.id = 1 with resulting_next_continuous_effect_id = 100; it passes this check, and replay advances the live allocator to 100. Subsequent effects then diverge from the original execution. Require the recorded post-draw values to equal the authoritative allocator result for this installation, including rejecting overflow.
As per path instructions, journal replay must use strict validation rather than best-effort receipts.
🤖 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 2302 - 2316, The
ResolvedRulesCommand::ContinuousEffectInstall validation currently accepts any
lower allocator receipt, allowing replay to advance allocators incorrectly.
Replace the upper-bound checks with strict equality against the authoritative
allocator results for this installation, and reject allocator arithmetic
overflow; retain validation that entry.node matches command.cause and return
InvalidSerializedAuthority for mismatches.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
This was referenced Jul 27, 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.
Adds the modifier-installation half of the CR 733 resolved-command journal:
every delayed triggered ability and every transient continuous effect now
resolves, applies, and journals through a single authority, with a paired
replay applier that installs the exact recorded values.
Two variants, not one parameterized variant
ResolvedRulesCommandgainsDelayedTriggerInstallandContinuousEffectInstallas siblings. CLAUDE.md's categorical-boundary ruledecides this: the parameterization axis would straddle two CR sections the
engine resolves through entirely separate machinery.
system; it waits in
delayed_triggersuntil its condition occurs, thengoes on the stack as an ordinary triggered ability (CR 603.7b). It draws
NO allocator value.
applies continuously through the CR 613 layers until its duration ends. It
draws TWO allocator values -- an effect id, and a CR 613.7b timestamp that
orders it within its layer.
Their
expected_*/resulting_*shapes therefore differ in kind, not in a leafvalue: one command carries an allocator receipt to verify, the other has
nothing to verify. Collapsing them would put two unrelated invariants behind
one validator arm and one applier. This is the categorical-boundary failure
CLAUDE.md warns about, not the sibling-cluster smell it warns about -- the two
share no name root, no context label, and no comparator/scope axis.
Authorities
Delayed triggers had 11 raw
delayed_triggers.pushsites across 10 productionfiles and no authority;
game::triggers::install_delayed_triggeris new andnow owns all of them. Transient continuous effects already had exactly one
production insertion authority --
GameState::add_transient_continuous_effect-- so that half is journaling only, no extraction.
Replay installs the whole
DelayedTrigger/TransientContinuousEffectverbatim: the CR 603.7c bound ability with its targets, the CR 611.2c fixed
affected set, and both allocator draws. Nothing is re-selected, re-read, or
re-drawn.
apply_resolved_continuous_effectadvancesnext_continuous_effect_idand
next_timestamppast the installed values, mirroring how the token-birthfamily advances
next_object_id, so a replayed state cannot hand the same idor timestamp out twice.
Both appliers verify their
expected_installed_countagainst live state andreturn a typed error before mutating anything; the continuous-effect applier
additionally rejects an id already live.
Scope
Installation only. Both collections also have a removal/expiry half (delayed
triggers:
collect_matching_delayed_triggersremove + cleanup retain;continuous effects: the layers.rs duration-prune family), which remains
unjournaled and is not covered here.
Summary by CodeRabbit
New Features
Bug Fixes
Tests