fix(engine): persist effect-resolution sacrifice batches across replacement pauses#6084
Merged
Merged
Conversation
…cement pauses The p03s6 census found effect-resolution sites that match SacrificeOutcome::NeedsReplacementChoice, surface the replacement prompt, and then silently drop the remainder of their work — the rest of the sacrifice loop, the tracked-set publish, per-sacrifice follow-ups, and the terminal EffectResolved never run after the pause resolves. Fix the five true match-but-forget sites by routing every collected effect-resolution sacrifice batch through the existing replacement-safe PendingPlayerScopeSacrificeChoice queue, parameterized by a typed completion (CR 614.1 / CR 616.1): - PendingPlayerScopeSacrificeCompletion gains effect_kind, a typed per-sacrifice follow_up (Exploit, CR 702.110b) with a delivered-ID ledger, publish_fresh_tracked_set, and propagate_parent_context — all serde-defaulted typed data, so the queue re-parks safely on nested pauses and the terminal tail (simultaneous-departure stamp per CR 603.10a, count, tracked set, EffectResolved) runs exactly once. - effects/sacrifice.rs (mandatory-all fast path + targeted branch), effects/exploit.rs, effects/choose_and_sacrifice_rest.rs, and the EffectZoneChoice sacrifice arm in engine_resolution_choices.rs all delegate to perform_collected_player_scope_sacrifices_with_completion; the resolution-choice caller returns the live parked prompt on pause instead of clobbering it. - step_exact_count's old unconditional EffectResolved push (which fired even when the batch paused) is eliminated — the completion owns the terminal event. Sites verified NOT to need fixing: engine_debug.rs (the ordinary post-action pipeline owns its suffix) and the cost/payment sites already persisted via PendingCostMoveResume (#153/#162). No cost-resume variants added — this is the effect-resolution layer's own continuation vocabulary. Witnesses: replacement re-park with exactly-once terminal tail (targeted, effect-zone tracked set, exploit follow-up, choose-and-sacrifice-rest sweep) plus synchronous regressions, in cost_zone_pipeline.
matthewevans
enabled auto-merge
July 17, 2026 08:16
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request refactors the handling of simultaneous sacrifices (including exploit, choose-and-sacrifice-rest, and targeted sacrifice effects) to route them through a unified, replacement-safe player-scope sacrifice queue. This ensures that terminal resolution events, follow-up events (such as exploit triggers), and tracked set publications are correctly preserved and executed across replacement-choice boundaries. The review feedback correctly identifies a violation of Rule R2 (No bool fields) on the PendingPlayerScopeSacrificeCompletion struct, where publish_fresh_tracked_set and propagate_parent_context are introduced as booleans instead of typed enums, and provides actionable suggestions to refactor these fields and their usages.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Comment on lines
+1557
to
+1564
| #[serde(default)] | ||
| pub publish_fresh_tracked_set: bool, | ||
| /// The interactive selection path normally snapshots the completed | ||
| /// sacrifice for a parent-referential continuation. Retain that suffix work | ||
| /// on the same typed batch when the selected sacrifice pauses. | ||
| #[serde(default)] | ||
| pub propagate_parent_context: bool, | ||
| } |
Contributor
There was a problem hiding this comment.
[MEDIUM] Violates Rule R2 (No bool fields). The new fields publish_fresh_tracked_set and propagate_parent_context are defined as bool on PendingPlayerScopeSacrificeCompletion. According to the repository style guide, bool fields should be parameterized with typed enums to better express the design space and prevent boolean proliferation.
#[serde(default)]
pub publish_fresh_tracked_set: TrackedSetPublish,
/// The interactive selection path normally snapshots the completed
/// sacrifice for a parent-referential continuation. Retain that suffix work
/// on the same typed batch when the selected sacrifice pauses.
#[serde(default)]
pub propagate_parent_context: ParentContextPropagation,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
pub enum TrackedSetPublish {
#[default]
None,
PublishFresh,
}
#[derive(Clone, Copy, Debug, PartialEq, Eq, Serialize, Deserialize, Default)]
pub enum ParentContextPropagation {
#[default]
None,
Propagate,
}References
- Rule R2: A bool field never expresses the design space; the project uses typed enums instead. (link)
Comment on lines
23
to
+27
| AutoMayChoice, CastOfferKind, ClauseMinimumSnapshot, DayNight, GameState, LKISnapshot, | ||
| ManaAbilityResume, MayTriggerAutoChoiceKey, PendingContinuation, PendingCopyTokenBatch, | ||
| PendingCostMoveResume, PendingPlayerScopeSacrificeChoice, | ||
| PendingPlayerScopeSacrificeCompletion, PendingRepeatedOptionalPayment, WaitingFor, | ||
| ZoneChangeRecord, | ||
| PendingPlayerScopeSacrificeCompletion, PendingPlayerScopeSacrificeFollowUp, | ||
| PendingRepeatedOptionalPayment, WaitingFor, ZoneChangeRecord, |
Contributor
There was a problem hiding this comment.
[MEDIUM] Import the newly defined TrackedSetPublish and ParentContextPropagation enums to support the refactored typed fields in PendingPlayerScopeSacrificeCompletion.
Suggested change
| AutoMayChoice, CastOfferKind, ClauseMinimumSnapshot, DayNight, GameState, LKISnapshot, | |
| ManaAbilityResume, MayTriggerAutoChoiceKey, PendingContinuation, PendingCopyTokenBatch, | |
| PendingCostMoveResume, PendingPlayerScopeSacrificeChoice, | |
| PendingPlayerScopeSacrificeCompletion, PendingRepeatedOptionalPayment, WaitingFor, | |
| ZoneChangeRecord, | |
| PendingPlayerScopeSacrificeCompletion, PendingPlayerScopeSacrificeFollowUp, | |
| PendingRepeatedOptionalPayment, WaitingFor, ZoneChangeRecord, | |
| AutoMayChoice, CastOfferKind, ClauseMinimumSnapshot, DayNight, GameState, LKISnapshot, | |
| ManaAbilityResume, MayTriggerAutoChoiceKey, ParentContextPropagation, PendingContinuation, | |
| PendingCopyTokenBatch, PendingCostMoveResume, PendingPlayerScopeSacrificeChoice, | |
| PendingPlayerScopeSacrificeCompletion, PendingPlayerScopeSacrificeFollowUp, | |
| PendingRepeatedOptionalPayment, TrackedSetPublish, WaitingFor, ZoneChangeRecord, |
References
- Rule R2: A bool field never expresses the design space; the project uses typed enums instead. (link)
Comment on lines
+5980
to
+5991
| if completion.publish_fresh_tracked_set { | ||
| publish_fresh_tracked_set(state, completion.sacrificed.clone()); | ||
| } | ||
| if completion.propagate_parent_context { | ||
| if let Some(snapshot) = | ||
| parent_referent_context_from_events(state, &events[events_before_sacrifice..]) | ||
| { | ||
| if let Some(cont) = state.pending_continuation.as_mut() { | ||
| cont.chain.set_effect_context_object_recursive(snapshot); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
[MEDIUM] Update the usage of publish_fresh_tracked_set and propagate_parent_context to check against the new typed enums instead of boolean values.
Suggested change
| if completion.publish_fresh_tracked_set { | |
| publish_fresh_tracked_set(state, completion.sacrificed.clone()); | |
| } | |
| if completion.propagate_parent_context { | |
| if let Some(snapshot) = | |
| parent_referent_context_from_events(state, &events[events_before_sacrifice..]) | |
| { | |
| if let Some(cont) = state.pending_continuation.as_mut() { | |
| cont.chain.set_effect_context_object_recursive(snapshot); | |
| } | |
| } | |
| } | |
| if completion.publish_fresh_tracked_set == TrackedSetPublish::PublishFresh { | |
| publish_fresh_tracked_set(state, completion.sacrificed.clone()); | |
| } | |
| if completion.propagate_parent_context == ParentContextPropagation::Propagate { | |
| if let Some(snapshot) = | |
| parent_referent_context_from_events(state, &events[events_before_sacrifice..]) | |
| { | |
| if let Some(cont) = state.pending_continuation.as_mut() { | |
| cont.chain.set_effect_context_object_recursive(snapshot); | |
| } | |
| } | |
| } |
References
- Rule R2: A bool field never expresses the design space; the project uses typed enums instead. (link)
Comment on lines
10
to
13
| ActionResult, CastOfferKind, ChosenDamageSource, CopyChosenSelection, GameState, | ||
| OutsideGameChoiceSource, PayableResource, PendingContinuation, WaitingFor, | ||
| OutsideGameChoiceSource, PayableResource, PendingContinuation, | ||
| PendingPlayerScopeSacrificeCompletion, WaitingFor, | ||
| }; |
Contributor
There was a problem hiding this comment.
[MEDIUM] Import the newly defined TrackedSetPublish and ParentContextPropagation enums to support the refactored typed fields in PendingPlayerScopeSacrificeCompletion.
Suggested change
| ActionResult, CastOfferKind, ChosenDamageSource, CopyChosenSelection, GameState, | |
| OutsideGameChoiceSource, PayableResource, PendingContinuation, WaitingFor, | |
| OutsideGameChoiceSource, PayableResource, PendingContinuation, | |
| PendingPlayerScopeSacrificeCompletion, WaitingFor, | |
| }; | |
| ActionResult, CastOfferKind, ChosenDamageSource, CopyChosenSelection, GameState, | |
| OutsideGameChoiceSource, ParentContextPropagation, PayableResource, PendingContinuation, | |
| PendingPlayerScopeSacrificeCompletion, TrackedSetPublish, WaitingFor, |
References
- Rule R2: A bool field never expresses the design space; the project uses typed enums instead. (link)
Comment on lines
+3875
to
+3880
| let completion = PendingPlayerScopeSacrificeCompletion { | ||
| effect_kind: Some(EffectKind::Sacrifice), | ||
| publish_fresh_tracked_set: state.pending_continuation.is_some(), | ||
| propagate_parent_context: true, | ||
| ..Default::default() | ||
| }; |
Contributor
There was a problem hiding this comment.
[MEDIUM] Update the instantiation of PendingPlayerScopeSacrificeCompletion to use the new typed enums instead of boolean values.
let completion = PendingPlayerScopeSacrificeCompletion {
effect_kind: Some(EffectKind::Sacrifice),
publish_fresh_tracked_set: if state.pending_continuation.is_some() {
TrackedSetPublish::PublishFresh
} else {
TrackedSetPublish::None
},
propagate_parent_context: ParentContextPropagation::Propagate,
..Default::default()
};References
- Rule R2: A bool field never expresses the design space; the project uses typed enums instead. (link)
Parse changes introduced by this PR✓ No card-parse changes detected. |
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.
The p03s6 census found effect-resolution sites that match
SacrificeOutcome::NeedsReplacementChoice, surface the replacement prompt,
and then silently drop the remainder of their work — the rest of the
sacrifice loop, the tracked-set publish, per-sacrifice follow-ups, and the
terminal EffectResolved never run after the pause resolves.
Fix the five true match-but-forget sites by routing every collected
effect-resolution sacrifice batch through the existing replacement-safe
PendingPlayerScopeSacrificeChoice queue, parameterized by a typed
completion (CR 614.1 / CR 616.1):
per-sacrifice follow_up (Exploit, CR 702.110b) with a delivered-ID
ledger, publish_fresh_tracked_set, and propagate_parent_context — all
serde-defaulted typed data, so the queue re-parks safely on nested
pauses and the terminal tail (simultaneous-departure stamp per
CR 603.10a, count, tracked set, EffectResolved) runs exactly once.
effects/exploit.rs, effects/choose_and_sacrifice_rest.rs, and the
EffectZoneChoice sacrifice arm in engine_resolution_choices.rs all
delegate to perform_collected_player_scope_sacrifices_with_completion;
the resolution-choice caller returns the live parked prompt on pause
instead of clobbering it.
even when the batch paused) is eliminated — the completion owns the
terminal event.
Sites verified NOT to need fixing: engine_debug.rs (the ordinary
post-action pipeline owns its suffix) and the cost/payment sites already
persisted via PendingCostMoveResume (#153/#162). No cost-resume variants
added — this is the effect-resolution layer's own continuation vocabulary.
Witnesses: replacement re-park with exactly-once terminal tail (targeted,
effect-zone tracked set, exploit follow-up, choose-and-sacrifice-rest
sweep) plus synchronous regressions, in cost_zone_pipeline.