fix(engine): separate carrier well-formedness from settle readiness - #7067
Conversation
|
Warning Review limit reached
Next review available in: 22 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughChangesSettlement readiness
Estimated code review effort: 3 (Moderate) | ~20 minutes 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 |
`resolving_stack_entry_can_settle` conjoined a structural invariant with its
readiness conditions:
state.resolving_trigger_firing.is_some()
== entry.kind is TriggeredAbility
That equality is true for every well-formed state, so it never gated anything
it was meant to. On a carrier whose firing classification is desynchronized
from its entry kind it evaluates false, the predicate reports "not finished",
and both `pub(super)` settle wrappers become no-ops — disabling the very sweep
that exists to clear such a carrier. CR 608.2c and CR 608.2m make readiness a
question about whether the resolution has finished following its instructions;
whether the carrier is well-formed is a separate question and does not belong
in that predicate.
The conjunct is lifted out into `resolving_carrier_parity_is_coherent`
(CR 603.7 — a firing classification is private to a triggered carrier), which
is reported on the settle path rather than gating it. The `debug_assert!` is
compiled out at the shipped `opt-level = 'z'`, so the violation also gets a
release-visible `tracing::warn!`; the carrier settles either way, since the
resolution it owns has finished regardless and withholding the sweep would only
leave it behind.
The gate loses a conjunct, so per-priority-boundary cost (CR 704.3) strictly
decreases. Parity is computed on the settle path, which runs only when a
resolution actually completes. No new state, field, flag, or enum variant.
Scope note: this is defensive hardening, not a fix for the state reported in
#6892. That capture was recovered and replayed against this base — it decodes
and heals, because `normalize_legacy_completed_resolution_carrier` clears
exactly that shape. The capture also predates the fields involved
(`resolving_trigger_firing` arrived in 8121fd1, the settle gate in
4099566; the capture is 2026-07-23) and carries the firing keys as absent
rather than null. No production read of `resolving_stack_entry` is known to
block priority or legal actions, and in release the next resolution overwrites
a leaked carrier — so the benefit here is that the sweep can do its job on
malformed input, not that a stuck game unlocks.
Four inline tests. The discriminating one asserts a desynchronized triggered
carrier settles at the priority boundary; it was verified red on revert of the
conjunct removal alone, with the three non-vacuity probes staying green
(readiness intact for both pairings, leading `is_some()` conjunct intact, and
all six carrier/firing pairings classified).
Also updates the CR 603.5 prompt-census pin `game/engine.rs:11712 -> :11747`
with a drift-log entry. Pure local line movement: the producer is
sha256-identical (`8a544e878d3e77fb...`), unique in the file, still inside
`begin_pending_trigger_target_selection`, and the set is unchanged at five
producers (total 37, partition 5/7/25).
Refs #6892
26fe17a to
5188955
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
What this is
Defensive hardening of the resolving-carrier settle gate: it separates a well-formedness assertion from a readiness predicate.
resolving_stack_entry_can_settleincluded this conjunct:That is a well-formedness test living inside a readiness predicate. If the carrier/firing pairing is ever incoherent, the gate returns
false, the state reads as "not finished," and the sweep that exists to clear the carrier is disabled by the very condition it is supposed to clear. The malformed carrier is the one input the sweep can never act on.What changed
One file,
crates/engine/src/game/engine.rs(+274 / −6):resolving_carrier_is_triggeredandresolving_carrier_parity_is_coherentas diagnostics, consumed at three sites rather than duplicated.tracing::warn!on the settle path when the pairing is incoherent, sincedebug_assert!is compiled out at the shippedopt-level = 'z'.debug_assert!pointed at readiness, deliberately. Repointing it at the parity predicate would panic in debug on exactly the malformed input this change exists to tolerate.CR annotations (each verified against
docs/MagicCompRules.txtbefore being written): CR 113.3c, CR 603.7, CR 608.2c, CR 608.2m, CR 704.3.Hot path: the gate runs at every priority boundary and now evaluates one fewer conjunct, so per-boundary cost strictly decreases. The parity computation moves to the settle path, which runs only when a resolution actually completes.
Relationship to #6892 — please read
This is not a fix for #6892, and #6892 should not be closed by this PR.
The issue was triaged as an invariant violation:
resolving_stack_entry.kind == TriggeredAbilitywithresolving_trigger_firing == null. That diagnosis is anachronistic. I recovered the reported capture and replayed it againstmain:resolving_trigger_firing: nullresolved_rules_journal: 6 entriesresolving_trigger_firingandTriggerFiringarrived in8121fd1c6a(#6842, 2026-08-01);resolving_stack_entry_can_settlein4099566716(#6933, 2026-08-02). The capture is dated 2026-07-23 — nine days earlier.Replayed through the production restore path on current
main, the capture heals:resolving_stack_entrycomes backNone, becausenormalize_legacy_completed_resolution_carriermatches that shape exactly. The reported stuck state does not reproduce.Severity
Lower than the issue claims, and stated accordingly in every comment and test name here:
resolving_stack_entryblocks priority or legal actions. The gating reads requirewaiting_forto be a resolution-choice prompt, notPriority.begin_resolving_stack_entryassigns both fields unconditionally, so the next resolution overwrites a leaked carrier and restores parity.The honest benefit is "the sweep can do its job on malformed input" — not a softlock fix. The words "softlock", "permanent", and "P0" appear nowhere in this change.
Tests
Four inline
#[cfg(test)]tests.resolving_trigger_firingispub(crate)and both settle wrappers arepub(super), so an external test crate cannot reach them.a_triggered_carrier_missing_its_firing_still_settles_at_the_priority_boundarya_suspended_resolution_still_blocks_settling_for_either_pairingno_carrier_means_nothing_to_settleparity_predicate_classifies_every_carrier_pairingThe two non-vacuity probes are load-bearing: without them the discriminating test is satisfiable by a gate that simply always settles. Negative control was run — reverting the conjunct removal alone turns the first test red.
Approaches considered and rejected
allow_unlabeled_v1_carriersdecode gate — would universalize a fabrication that silently downgradesOrdinary/ReceiptEligiblefirings toLegacyDelayed.UnknownLegacy—validate_firingreturnsErrfor it, so this would turn genuine v1 restores into hard decode errors.Neither reached code.
Verification
cargo fmt --check,cargo clippy -D warnings, and the fullphase-enginesuite all pass at this commit (18557 + 4587 + 12 + 9 tests, 0 failures), run in an isolatedCARGO_TARGET_DIR. The CR 603.5 prompt census pin was re-derived by content hash after rebase and re-verified.No parser impact, no coverage impact, no card-data regeneration.
Summary by CodeRabbit