fix(engine): loop-ring retention across forced windows + observer-negative rulings (combo-fb phases 0-2, chain 1) - #6838
Conversation
github-actions
Bot
added
the
needs-maintainer
|
🤖 AI text below 🤖 Chain link 2 (phases 3–4, draft until this PR merges): #6839 |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds per-schema shortcut iteration limits, validates declarations against those limits, preserves loop-detection samples through forced cascade windows, invalidates them after life changes, and strengthens resource and trigger reachability analysis. ChangesLoop shortcut bounds and loop-detection soundness
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant ShortcutOffer
participant Engine
participant GameState
participant ResourceAnalysis
ShortcutOffer->>Engine: submit shortcut declaration
Engine->>GameState: classify waiting window
GameState-->>Engine: forced-window result
Engine->>ResourceAnalysis: validate iteration bound and loop-cover proofs
ResourceAnalysis-->>Engine: accept or reject declaration
Engine->>GameState: invalidate loop ring when life changed
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
fc4c520 to
201bdb6
Compare
Parse changes introduced by this PR✓ No card-parse changes detected. |
|
🤖 AI text below 🤖 Rebased onto upstream/main |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (2)
crates/engine/tests/integration/loop_shortcut.rs (1)
5013-5018: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLoosen the exact driven-beat assertion to a bounded range.
The comment states the purpose of this assertion is anti-vacuity — proving the row re-derived the crown instead of reading the dump's saved offer.
beats > 0carries that property.beats == 6additionally pins the detector's internal beat schedule for this fixture. Any unrelated change to the number of prompts the fixture emits before the offer re-forms turns this row red with a message that claims a zero-beat read, which is wrong when the observed value is 5 or 7. Keep the count in the panic text and bound it instead.♻️ Proposed change
- assert_eq!( - beats, 6, - "the `c decline` arm re-derives the crown after a measured 6 driven beats — a 0 here \ - would mean the row read the dump's saved offer back instead of re-deriving one, \ - which is what makes both revert-probes above live" - ); + assert!( + (1..=20).contains(&beats), + "the `c decline` arm must RE-DERIVE the crown from live beats (measured: 6). A 0 \ + would mean the row read the dump's saved offer back instead of re-deriving one, \ + which is what makes both revert-probes above live; got {beats}" + );🤖 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/loop_shortcut.rs` around lines 5013 - 5018, Update the beats assertion in the affected integration test to require a positive bounded count rather than exactly 6, preserving the anti-vacuity guarantee while allowing fixture scheduling changes. Keep the observed beats value in the assertion failure message and revise its wording so it accurately describes the zero-beat failure.crates/engine/src/game/engine.rs (1)
441-447: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUpdate the shortcut comment references.
reconcile_terminal_resultruns at lines 336 and 344 insideapply_action_boundary_with_stack_limit, whichapplydelegates to. Update the line numbers and function reference.🤖 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 441 - 447, Update the shortcut comment above the NET-PROGRESS cascade logic to reference both reconcile_terminal_result calls at lines 336 and 344, and identify their containing function as apply_action_boundary_with_stack_limit, which apply delegates to; leave the implementation unchanged.
🤖 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/analysis/loop_check.rs`:
- Around line 633-638: Update the pointer comment above the CR 704.3 partition
to state that forced_cascade_window_class covers thirteen members and eight
non-members, preserving the existing references and explanation.
In `@crates/engine/src/analysis/resource.rs`:
- Around line 2218-2231: The loop-growth relief in the execute-definition check
must validate ledger exclusion against every member of the fodder class, not
only the min_by_key representative. Update
loop_states_cover_modulo_fodder_growth and
execute_ledger_condition_provably_excludes_class to thread the full all_fodder
member set and require exclusion for every member, or equivalently add a
conjunct proving the five uncompared record fields agree across the class before
reusing the representative’s verdict; preserve the existing fail-closed behavior
for None.
- Around line 592-613: Update the life bound calculation in the surrounding
elimination-bound function to combine observed_life_loss and
declared_life_magnitude additively instead of using max, ensuring the bound
fails closed when declarable victim loss includes unattributed life. Recompute
the expectations for cases (j) and (m) in elimination_bounds_conventions to
match the stricter bound.
- Around line 2249-2263: Update the `relieved` condition in the
`obj.abilities.iter().any` scan to apply CR 117.1b relief only when `ability`
has `AbilityKind::Activated`; retain the existing foreign-controller, non-mana,
and `sole_driver` checks. Ensure non-activated kinds, including spell abilities,
continue to be scanned and vetoed.
- Around line 1939-1942: Update the documentation comment describing the
ledger-invariance check near the trigger execute analysis, replacing the invalid
CR 608.2h and CR 608.2i citations with current Comprehensive Rules references
that accurately describe the resolver’s count semantics. Preserve the existing
CR 732.2a citation and the comment’s stated condition and return behavior.
In `@crates/engine/src/game/engine.rs`:
- Around line 2845-2853: Extract the repeated rejection flow from
handle_declare_shortcut into a helper such as reject_shortcut_declaration(state:
&mut GameState, result: &mut ActionResult) that resets priority, assigns the
next living seat, synchronizes result.waiting_for, and returns the appropriate
outcome. Replace all five duplicated blocks, including the sites in the
pin-validation and count-bound branches, with calls to this helper while
preserving the existing fail-closed behavior.
In `@crates/engine/src/types/game_state.rs`:
- Around line 17983-17992: Update invalidate_loop_ring_on_unobserved_life_move
to require lives_before.len() == self.players.len() before positional
comparison, and clear loop_detect_ring when the lengths differ. Preserve the
existing life-change comparison for matching snapshots so snapshots from before
player growth cannot leave the ring intact.
In `@crates/engine/tests/integration/interaction_contract.rs`:
- Around line 2342-2346: Extend the production-pipeline tests around
ShortcutDecisionSchema to use a narrowed max_iterations bound, then verify
Fixed(n) above that bound is rejected and the picker returns
InteractionShortcutCountSpec::Fixed with max equal to the narrowed value. Keep
the existing default-bound and serialization coverage unchanged.
In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 4527-4540: Update both offer-asserting tests in
crates/engine/tests/integration/loop_shortcut.rs at lines 4527-4540 and
5922-5936: bind the bystander id returned by object_growth_with_bystander (using
PRYDWEN_ORACLE and PHASE_LEDGER_ARTIFACT_FILTER_ORACLE respectively), then
before matching runner.state().waiting_for add guards that the object is in
Zone::Battlefield and has exactly one trigger definition. Preserve the existing
offer assertions and negative-test behavior.
---
Nitpick comments:
In `@crates/engine/src/game/engine.rs`:
- Around line 441-447: Update the shortcut comment above the NET-PROGRESS
cascade logic to reference both reconcile_terminal_result calls at lines 336 and
344, and identify their containing function as
apply_action_boundary_with_stack_limit, which apply delegates to; leave the
implementation unchanged.
In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 5013-5018: Update the beats assertion in the affected integration
test to require a positive bounded count rather than exactly 6, preserving the
anti-vacuity guarantee while allowing fixture scheduling changes. Keep the
observed beats value in the assertion failure message and revise its wording so
it accurately describes the zero-beat failure.
🪄 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: ff4b13b2-7ba5-45ba-8f1e-9b4e55fca525
⛔ Files ignored due to path filters (4)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (13)
crates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/combat_damage.rscrates/engine/src/game/engine.rscrates/engine/src/game/interaction.rscrates/engine/src/game/quantity.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/triggers.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/loop_shortcut.rs
| let declared_life_magnitude: i64 = | ||
| slot_magnitude.values().copied().filter(|m| *m > 0).sum(); | ||
|
|
||
| let mut bound = cap; | ||
| let mut narrow = |headroom: i64, magnitude: i64| { | ||
| if magnitude > 0 { | ||
| bound = bound.min(headroom.max(0) / magnitude); | ||
| } | ||
| }; | ||
|
|
||
| for p in &state.players { | ||
| // CR 800.4a: an eliminated seat has left the game and constrains nothing. | ||
| if p.is_eliminated { | ||
| continue; | ||
| } | ||
| // CR 704.5a. A negative life delta is the per-period loss. | ||
| let observed_life_loss = -self.life.get(&p.id).copied().unwrap_or(0); | ||
| let life_magnitude = if declarable_victims.contains(&p.id) { | ||
| observed_life_loss.max(declared_life_magnitude) | ||
| } else { | ||
| observed_life_loss | ||
| }; |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Make the elimination bound fail closed: combine the observed loss and the declared slot magnitude additively.
Line 610 uses observed_life_loss.max(declared_life_magnitude). The doc block at Lines 541-559 states that this is correct only when L_unattributed(p) == 0 for every declarable victim, and that it otherwise overstates the bound and permits an in-proposal CR 704.5a elimination. The doc also states the failure direction: max fails open. Nothing in the signature or the body enforces the 1:1 attribution gate, so the first production caller inherits an unsound bound with only a comment as protection.
Two options keep the repository's fail-closed convention:
- Combine additively (
observed_life_loss + declared_life_magnitude). This can only lower the bound, so it cannot admit an offer the current form rejects. Cases (j) and (m) inelimination_bounds_conventionsthen need their expectations recomputed. - Change the signature to carry per-slot victim attribution, and compute
L_unattributed(p) + S(p)as the doc describes.
If you keep max, encode the gate in the type system instead of prose, for example by taking the attribution map as a required argument so a caller cannot reach the fail-open path.
🛡️ Fail-closed combination
let life_magnitude = if declarable_victims.contains(&p.id) {
- observed_life_loss.max(declared_life_magnitude)
+ // Fail-closed: an untargeted drain and a re-aimable slot can both hit
+ // the same seat in one period, so the two magnitudes ADD.
+ observed_life_loss + declared_life_magnitude
} else {
observed_life_loss
};🤖 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/analysis/resource.rs` around lines 592 - 613, Update the
life bound calculation in the surrounding elimination-bound function to combine
observed_life_loss and declared_life_magnitude additively instead of using max,
ensuring the bound fails closed when declarable victim loss includes
unattributed life. Recompute the expectations for cases (j) and (m) in
elimination_bounds_conventions to match the stricter bound.
Source: Path instructions
| /// CR 732.2a + CR 608.2h + CR 608.2i: does this trigger's `execute` body observe the | ||
| /// growing class ONLY through a battlefield-entry-ledger condition whose filter PROVABLY | ||
| /// cannot count `class_member`? Returns `true` iff so — then the read's value is | ||
| /// invariant across the loop's growth and the observer does not observe the loop. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🌐 Web query:
Magic Comprehensive Rules 608.2h and 608.2i exact text
💡 Result:
The exact text for Magic: The Gathering Comprehensive Rules sections 608.2h and 608.2i is as follows [1][2]: 608.2h: If an effect requires information from the game (such as the number of creatures on the battlefield), the answer is determined only once, when the effect is applied. If the effect requires information from a specific object, including the source of the ability itself, the effect uses the current information of that object if it’s in the public zone it was expected to be in; if it’s no longer in that zone, or if the effect has moved it from a public zone to a hidden zone, the effect uses the object’s last known information. See rule 113.7a. If an ability states that an object does something, it’s the object as it exists—or as it most recently existed—that does it, not the ability. 608.2i: If an effect refers to certain characteristics, it checks only for the value of the specified characteristics, regardless of any related ones an object may also have. (Example: An effect that reads “Destroy all black creatures” destroys a white-and-black creature, but one that reads “Destroy all nonblack creatures” doesn’t.)
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/resolving-spells-and-abilities.html
- 2: https://mtg-rules.vercel.app/chapters/608
🏁 Script executed:
sed -n '1915,1960p;2205,2240p' crates/engine/src/analysis/resource.rsRepository: phase-rs/phase
Length of output: 5679
🌐 Web query:
site:magic.wizards.com Comprehensive Rules 608.2h 608.2i current
💡 Result:
In the Magic: The Gathering Comprehensive Rules, sections 608.2h and 608.2i do not exist. Rule 608 covers the resolution of spells and abilities [1]. The section 608.2, which details the resolution process, currently ends at 608.2g. Section 608.3 exists and pertains to the resolution of permanent spells [2].
Citations:
- 1: https://magic.wizards.com/en/rules
- 2: https://magic.wizards.com/en/news/announcements/comprehensive-rules-changes-6-1-22
Replace the invalid CR citations for ledger invariance.
CR 608.2h and CR 608.2i do not exist in the current Comprehensive Rules; CR 608.2 ends at CR 608.2g. Replace these citations in both comments with applicable rules that describe the resolver’s count semantics.
🤖 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/analysis/resource.rs` around lines 1939 - 1942, Update the
documentation comment describing the ledger-invariance check near the trigger
execute analysis, replacing the invalid CR 608.2h and CR 608.2i citations with
current Comprehensive Rules references that accurately describe the resolver’s
count semantics. Preserve the existing CR 732.2a citation and the comment’s
stated condition and return behavior.
Source: Path instructions
| if let Some(exec) = def.execute.as_ref() { | ||
| // CR 608.2h + CR 608.2i: a ledger read whose filter provably cannot count | ||
| // the growing fodder has a value invariant across the loop's growth, so | ||
| // this def does not observe the loop — skip it rather than veto. | ||
| // Fail-closed on `class_member: None` (the OFFLINE cover passes `None` and | ||
| // is therefore untouched BY this narrowing — note that the CR 117.1b / | ||
| // CR 510.2 scope guards above are NOT class_member-gated and DO reach it). | ||
| if scan::ability_definition_reads_sibling_mutable_for_loop(exec) | ||
| && !class_member.is_some_and(|m| { | ||
| execute_ledger_condition_provably_excludes_class(exec, state, m, obj) | ||
| }) | ||
| { | ||
| return true; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Close the one-representative generalisation gap: test the exclusion against every fodder member, not one.
execute_ledger_condition_provably_excludes_class proves exclusion for a single class_member. The doc at Lines 1968-1998 states that the fodder relation (fodder_content_eq → object_content_eq) does not compare core_types, subtypes, supertypes, colors, or keywords, and that the ledger matcher reads four of those five verdict-bearingly on today's card pool. So two objects can be in the same fodder class while the matcher excludes one and counts the other. Relief then applies to a class whose later members the observer does count. That is the relieving direction the repo forbids.
The information needed to close this is already at the call site: loop_states_cover_modulo_fodder_growth builds all_fodder at Lines 1454-1460 and then discards it, passing only the min_by_key representative at Lines 1479-1486.
Two ways to close it:
- Thread the whole pile. Change
class_member: Option<ObjectId>to a slice of class member ids and require exclusion for every member. The.all()shape in conjunct (c) already generalises. - Add a conjunct requiring the five uncompared record fields to agree across the pile, so the representative's verdict provably carries.
Until one of these lands, the relief rests on an undischarged premise rather than a proof.
🤖 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/analysis/resource.rs` around lines 2218 - 2231, The
loop-growth relief in the execute-definition check must validate ledger
exclusion against every member of the fodder class, not only the min_by_key
representative. Update loop_states_cover_modulo_fodder_growth and
execute_ledger_condition_provably_excludes_class to thread the full all_fodder
member set and require exclusion for every member, or equivalently add a
conjunct proving the five uncompared record fields agree across the class before
reusing the representative’s verdict; preserve the existing fail-closed behavior
for None.
Source: Path instructions
| if obj.abilities.iter().any(|ability| { | ||
| // CR 117.1b + CR 732.2c: no player but the sole driver receives priority | ||
| // inside the taken shortcut, so a FOREIGN-controlled activated ability | ||
| // cannot be activated during the window and cannot read the growing class. | ||
| // CR 605.3a bounds this: a mana ability is activatable outside the priority | ||
| // rule (while another player casts a spell or activates an ability), so it | ||
| // is NOT relieved and keeps vetoing. | ||
| // PER-ABILITY, never per-object: another surface on the same object (a | ||
| // trigger body, block (1)) must keep vetoing. | ||
| // Fail-closed on `sole_driver: None` (the caller proved nothing). | ||
| let relieved = scope.sole_driver.is_some_and(|driver| { | ||
| obj.controller != driver && !crate::game::mana_abilities::is_mana_ability(ability) | ||
| }); | ||
| !relieved && scan::ability_definition_reads_sibling_mutable_for_loop(ability) | ||
| }) { |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Restrict the CR 117.1b relief to AbilityKind::Activated.
The comment justifies the relief with CR 117.1b: a foreign player cannot activate an ability without priority. The condition does not check the ability's kind. Block (2) is deliberately kind-agnostic (S5) — object_growth_r_s5_non_activated_ability_kind_rejects at Lines 6517-6536 pins that an AbilityKind::Spell def in obj.abilities is scanned and vetoes.
So a non-activated ability body on a foreign permanent now gets relieved. CR 117.1b does not bound a static or spell-kind body, because nobody activates it. The relief is therefore unsound for every kind except Activated, in the relieving direction.
Add the kind conjunct so the guard matches the rule it cites.
🛡️ Bound the relief to activated abilities
let relieved = scope.sole_driver.is_some_and(|driver| {
- obj.controller != driver && !crate::game::mana_abilities::is_mana_ability(ability)
+ matches!(ability.kind, crate::types::ability::AbilityKind::Activated)
+ && obj.controller != driver
+ && !crate::game::mana_abilities::is_mana_ability(ability)
});📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if obj.abilities.iter().any(|ability| { | |
| // CR 117.1b + CR 732.2c: no player but the sole driver receives priority | |
| // inside the taken shortcut, so a FOREIGN-controlled activated ability | |
| // cannot be activated during the window and cannot read the growing class. | |
| // CR 605.3a bounds this: a mana ability is activatable outside the priority | |
| // rule (while another player casts a spell or activates an ability), so it | |
| // is NOT relieved and keeps vetoing. | |
| // PER-ABILITY, never per-object: another surface on the same object (a | |
| // trigger body, block (1)) must keep vetoing. | |
| // Fail-closed on `sole_driver: None` (the caller proved nothing). | |
| let relieved = scope.sole_driver.is_some_and(|driver| { | |
| obj.controller != driver && !crate::game::mana_abilities::is_mana_ability(ability) | |
| }); | |
| !relieved && scan::ability_definition_reads_sibling_mutable_for_loop(ability) | |
| }) { | |
| if obj.abilities.iter().any(|ability| { | |
| // CR 117.1b + CR 732.2c: no player but the sole driver receives priority | |
| // inside the taken shortcut, so a FOREIGN-controlled activated ability | |
| // cannot be activated during the window and cannot read the growing class. | |
| // CR 605.3a bounds this: a mana ability is activatable outside the priority | |
| // rule (while another player casts a spell or activates an ability), so it | |
| // is NOT relieved and keeps vetoing. | |
| // PER-ABILITY, never per-object: another surface on the same object (a | |
| // trigger body, block (1)) must keep vetoing. | |
| // Fail-closed on `sole_driver: None` (the caller proved nothing). | |
| let relieved = scope.sole_driver.is_some_and(|driver| { | |
| matches!(ability.kind, crate::types::ability::AbilityKind::Activated) | |
| && obj.controller != driver | |
| && !crate::game::mana_abilities::is_mana_ability(ability) | |
| }); | |
| !relieved && scan::ability_definition_reads_sibling_mutable_for_loop(ability) | |
| }) { |
🤖 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/analysis/resource.rs` around lines 2249 - 2263, Update the
`relieved` condition in the `obj.abilities.iter().any` scan to apply CR 117.1b
relief only when `ability` has `AbilityKind::Activated`; retain the existing
foreign-controller, non-mana, and `sole_driver` checks. Ensure non-activated
kinds, including spell abilities, continue to be scanned and vetoed.
Source: Path instructions
| pub(crate) fn invalidate_loop_ring_on_unobserved_life_move(&mut self, lives_before: &[i32]) { | ||
| if self | ||
| .players | ||
| .iter() | ||
| .zip(lives_before) | ||
| .any(|(p, &before)| p.life != before) | ||
| { | ||
| self.loop_detect_ring.clear(); | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate any code that changes the length of GameState::players.
rg -nP --type=rust -C3 '\bplayers\s*\.\s*(retain|remove|swap_remove|drain|clear|truncate|push|insert)\s*\(' crates/engine/src
# Confirm elimination only flags a seat rather than removing it.
rg -nP --type=rust -C3 '\bis_eliminated\s*=\s*true' crates/engine/srcRepository: phase-rs/phase
Length of output: 17496
🏁 Script executed:
# Find callers of invalidate_loop_ring_on_unobserved_life_move
rg -nP --type=rust 'invalidate_loop_ring_on_unobserved_life_move' crates/engine/src -B3 -A3Repository: phase-rs/phase
Length of output: 3920
🏁 Script executed:
# Check if snapshot is captured during gameplay and how life_before is populated
rg -nP --type=rust 'lives_before|life\s*\[\s*\]' crates/engine/src -B5 -A5 | head -100Repository: phase-rs/phase
Length of output: 4131
🏁 Script executed:
# Check synthesis.rs to see if player addition happens during active gameplay
rg -nP --type=rust -B10 -A10 'state.players.push\(p2\)' crates/engine/src/database/synthesis.rsRepository: phase-rs/phase
Length of output: 1310
🏁 Script executed:
# Verify that combat_damage.rs is only called during actual game play, not initialization
rg -nP --type=rust 'pub.*fn.*apply_combat_damage|mod combat_damage' crates/engine/src/game/combat_damage.rs | head -5Repository: phase-rs/phase
Length of output: 192
🏁 Script executed:
# Check if there are any other places where lives_before snapshot is taken and passed to this function
rg -nP --type=rust 'lives_before\s*=' crates/engine/src -B3 -A10Repository: phase-rs/phase
Length of output: 152
🏁 Script executed:
# Check if apply_combat_damage is the ONLY caller of invalidate_loop_ring_on_unobserved_life_move
# and confirm no other code paths capture stale snapshots
rg -nP --type=rust 'invalidate_loop_ring_on_unobserved_life_move' crates/engine/srcRepository: phase-rs/phase
Length of output: 768
🏁 Script executed:
# Verify the doc comment's claim about seats never being removed/added post-init
# by checking GameState constructors and when seat_order is initialized
rg -nP --type=rust 'fn new.*player|pub.*fn.*new.*GameState' crates/engine/src/types/game_state.rs | head -20Repository: phase-rs/phase
Length of output: 761
🏁 Script executed:
# Check if any test or code could pass a snapshot from one game state version to a different one
rg -nP --type=rust 'lives_before' crates/engine/src -B10 | grep -E '(lives_before|let.*state|snapshot)' | head -30Repository: phase-rs/phase
Length of output: 747
🏁 Script executed:
# Check the full context of the doc comment to see if it actually justifies the zip behavior
sed -n '17970,18000p' crates/engine/src/types/game_state.rsRepository: phase-rs/phase
Length of output: 1820
🏁 Script executed:
# Verify that apply_combat_damage captures the snapshot and immediately calls the function
# in the same scope with no intermediate state mutations
sed -n '1150,1365p' crates/engine/src/game/combat_damage.rs | head -80Repository: phase-rs/phase
Length of output: 4000
🏁 Script executed:
# Check if apply_combat_damage could ever be called in a scenario where players
# were added between initialization and the call
rg -nP --type=rust 'apply_combat_damage' crates/engine/src -B5 | grep -E 'apply_combat_damage|players.push|GameState::new' | head -30Repository: phase-rs/phase
Length of output: 1390
🏁 Script executed:
# Verify: is synthesis.rs ONLY for tests, or is it used in production gameplay?
head -50 crates/engine/src/database/synthesis.rsRepository: phase-rs/phase
Length of output: 3017
Add a length precondition to prevent silent seat misalignment on growth.
The zip iterator silently truncates on a length mismatch. If self.players grows post-initialization and a caller later passes a snapshot taken before that growth, the extra seats are never compared. This is a fail-open outcome: loop_detect_ring remains intact even though a life move occurred on an unobserved seat, violating the CR 510.2 fence (CR 119.3 dip-and-recover detection).
The doc comment claims seats are "never removed", justifying positional alignment. However, synthesis.rs demonstrates that seats are added post-initialization via push(). A defensive guard prevents this fail-open regardless of current caller safety:
pub(crate) fn invalidate_loop_ring_on_unobserved_life_move(&mut self, lives_before: &[i32]) {
+ if self.players.len() != lives_before.len() {
+ // A snapshot that does not cover every seat cannot prove "no life moved".
+ self.loop_detect_ring.clear();
+ return;
+ }
if self
.players
.iter()
.zip(lives_before)
.any(|(p, &before)| p.life != before)
{
self.loop_detect_ring.clear();
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| pub(crate) fn invalidate_loop_ring_on_unobserved_life_move(&mut self, lives_before: &[i32]) { | |
| if self | |
| .players | |
| .iter() | |
| .zip(lives_before) | |
| .any(|(p, &before)| p.life != before) | |
| { | |
| self.loop_detect_ring.clear(); | |
| } | |
| } | |
| pub(crate) fn invalidate_loop_ring_on_unobserved_life_move(&mut self, lives_before: &[i32]) { | |
| if self.players.len() != lives_before.len() { | |
| // A snapshot that does not cover every seat cannot prove "no life moved". | |
| self.loop_detect_ring.clear(); | |
| return; | |
| } | |
| if self | |
| .players | |
| .iter() | |
| .zip(lives_before) | |
| .any(|(p, &before)| p.life != before) | |
| { | |
| self.loop_detect_ring.clear(); | |
| } | |
| } |
🤖 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 17983 - 17992, Update
invalidate_loop_ring_on_unobserved_life_move to require lives_before.len() ==
self.players.len() before positional comparison, and clear loop_detect_ring when
the lengths differ. Preserve the existing life-change comparison for matching
snapshots so snapshots from before player growth cannot leave the ring intact.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head has unresolved soundness and CI blockers.
[HIGH] Foreign-observer relief is not limited to activated abilities. Evidence: crates/engine/src/analysis/resource.rs:2249-2263. Why it matters: the CR 117.1b priority rationale cannot relieve a foreign spell/static-kind ability; the current predicate can skip its mutable-surface read and certify a shortcut that must remain vetoed. Suggested fix: require AbilityKind::Activated before applying the foreign, non-mana relief and add a non-activated regression.
[HIGH] Ledger exclusion is proved only for one fodder representative. Evidence: crates/engine/src/analysis/resource.rs:2225-2228 (with the cover passing a single class member). Why it matters: the code itself documents that the fodder equivalence omits fields which filters can inspect, so another member can match the ledger even when the representative does not; the relief is unsound in the accepting direction. Suggested fix: thread the complete growing-class member set and require exclusion for every member, or prove equality of every filter-observable omitted field.
[MED] New production-fixture regression does not load. Evidence: CI Rust tests shard 1/2 and crates/engine/tests/integration/loop_shortcut.rs:4713-4718; the current four-player dump fails with missing field effect_kind. Why it matters: the central retention regression never reaches the apply pipeline. Suggested fix: regenerate/migrate the fixture against the current serialized GameState contract and retain a load reach guard.
[MED] The branch also fails formatting. Evidence: CI Rust lint reports crates/engine/src/game/triggers.rs:8. Suggested fix: format the committed branch before re-review.
[MED] New comments cite nonexistent CR 608.2h/608.2i. Evidence: crates/engine/src/analysis/resource.rs:2219,2223. Why it matters: the required rules evidence is false. Suggested fix: replace with verified, applicable CR citations or remove the unsupported citation.
The parse-diff is correctly no_changes; all open CodeRabbit findings were checked against this head. Re-request review after these blockers and the current CI are clean.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/interaction.rs`:
- Around line 2454-2468: Update the interaction shortcut projection around
InteractionShortcutCountSpec::Fixed to return InvalidAuthorityState when
schema.max_iterations is zero, before applying clamp. Remove the
release-ineffective debug-only reliance for this case, while preserving the
existing clamping behavior for valid positive values and ensuring invalid offers
are withheld rather than advertised.
In `@crates/engine/src/game/triggers.rs`:
- Around line 2246-2252: Update the cross-file line references in the
documentation comment near the cover-frame guarantees: point
derived_fodder_class to engine.rs:2205 and its loop-detector call to
engine.rs:2475, and point board_covers_modulo_fodder to resource.rs:1376 and its
executable call to resource.rs:1463. Preserve the existing wording and
guarantees.
In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 4399-4424: Add positive reach-guards before every listed negative
assertion in crates/engine/tests/integration/loop_shortcut.rs:4399-4424,
4484-4506, 4542-4552, 4581-4607, 4619-4641, 4659-4689, 5726-5755, 5905-5914, and
5941-5948. Bind each returned observer id and assert its Zone::Battlefield
placement plus the expected parsed trigger or activated-ability shape; guard
both variants or surfaces where specified, add the missing battlefield and
activated-ability checks, and validate the persisted fixture’s opponent
utility-land abilities and Jadar trigger rather than only foreign_permanents >=
3.
🪄 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: bfe374df-85a5-4e50-90cb-b09268775290
⛔ Files ignored due to path filters (4)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (13)
crates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/combat_damage.rscrates/engine/src/game/engine.rscrates/engine/src/game/interaction.rscrates/engine/src/game/quantity.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/triggers.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/loop_shortcut.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/engine/src/analysis/decision_template.rs
- crates/engine/src/game/visibility.rs
- crates/engine/tests/integration/interaction_contract.rs
- crates/engine/src/analysis/loop_check.rs
- crates/engine/src/game/restrictions.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/game/combat_damage.rs
- crates/engine/src/types/game_state.rs
|
Correction to my requested-changes review: I retract only the MED finding that CR 608.2h/608.2i were absent from the local Comprehensive Rules corpus. They are present and that citation finding is non-blocking. The two HIGH soundness blockers at |
|
🤖 AI text below 🤖 Thanks for the review. Pushed a fix for the CI blocker (item 4); notes on item 5 and the remaining findings below. Item 4 — formatting: fixed. The failure was the lint job's first step, The Item 5 — CR 608.2h / 608.2i: these do resolve. Measured against 608.2h is the information-determined-once / last-known-information rule ("If an effect requires information from the game … the answer is determined only once, when the effect is applied"); 608.2i is its explicit look-back-in-time exception. So the citations are not nonexistent. That said, existence is not applicability — your underlying concern may still be right if those two rules are mis-seated at Items 1, 2, 3 — acknowledged. The two HIGH findings (foreign-observer relief not gated on New heads: #6838 → |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — fresh review of current head 471484867adf066fcc3ee84d9932a8ddf5a1d581.
The sole delta since my prior review is the triggers.rs import reflow. It fixes formatting only; the following remain unresolved on this exact head:
[HIGH] Foreign-observer relief is still not limited to AbilityKind::Activated. Evidence: crates/engine/src/analysis/resource.rs:2249-2263. The CR 117.1b rationale cannot exempt foreign spell/static-kind abilities from the mutable-surface scan.
[HIGH] Ledger exclusion is still determined from one fodder representative. Evidence: crates/engine/src/analysis/resource.rs:2225-2228. Class membership deliberately omits filter-observable fields, so a different member may match even if the representative does not.
[MED] The four-player fixture regression remains unremediated. Evidence: crates/engine/tests/integration/loop_shortcut.rs:4713-4718; the previous identical-code CI failed while loading it with missing field effect_kind, and this head changes neither fixture nor migration/loading path.
[MED] An invalid fixed-shortcut authority with max_iterations == 0 is still normalized into an offered one-iteration choice. Evidence: crates/engine/src/game/interaction.rs:2458-2474. debug_assert! disappears in release, while clamp(1, ...) makes the forbidden offer visible; reject it with InvalidAuthorityState before clamping.
[MED] Several observer-negative tests still lack positive parse/observer reach guards. Evidence: crates/engine/tests/integration/loop_shortcut.rs:4399-4424 (and sibling rows identified in the current open review thread). A malformed or absent observer can make the no-offer result pass without exercising the intended firewall; bind and validate each observer’s battlefield/ability shape before asserting the outcome.
Parse-diff remains no_changes. Current Rust required checks are still in progress; do not re-request review until the above is addressed and CI is clean.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — one additional soundness blocker remains on current head 471484867adf066fcc3ee84d9932a8ddf5a1d581.
🔴 Blocker
[MED] The elimination bound admits a known fail-open life-loss case. Evidence: crates/engine/src/analysis/resource.rs:535-559 says the max form is correct only when unattributed loss is zero and otherwise permits an in-proposal elimination; :607-614 nevertheless uses observed_life_loss.max(declared_life_magnitude) without establishing that premise. Why it matters: CR 732.2a says, “It can’t include conditional actions, where the outcome of a game event determines the next action a player takes” (docs/MagicCompRules.txt:6372); permitting the shortcut to contain an unexpected life-zero event violates that constraint. Suggested fix: prove the 1:1 attribution gate before using max, or compute the conservative per-victim unattributed_loss + declared_slot_loss bound and add a case containing both loss sources.
Recommendation: request changes until the bound is fail-closed and the new mixed-loss regression passes.
4714848 to
95165b5
Compare
|
🤖 AI text below 🤖 Correcting the record on my 15:54 comment. That comment said the CI-blocker fix (item 4) was pushed. It was not. The commit existed only in my local worktree and never reached this branch, so head Now actually landed — head is
Post-rebase verification run on the new tip, all green: On the 16:47 blocker (elimination bound / |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — this current head still has unsound observer relief and shortcut bounds.
High
[HIGH] Foreign-observer relief is not limited to activated abilities. crates/engine/src/analysis/resource.rs:2249-2263 computes relieved from foreign control and non-mana status alone. CR 117.1b permits a player to activate an activated ability when they have priority; it does not permit exempting foreign spell or static abilities from this mutable-surface scan. A foreign spell/static-kind ability can still observe the growing class. Require AbilityKind::Activated in the relief condition and add a production-path regression for a non-activated foreign observer.
[HIGH] Ledger relief is proved for only one class representative. resource.rs:1952-1998 documents that fodder membership does not equate several filter-observable fields, while resource.rs:2218-2229 applies the exclusion proof to only the min_by_key class member. That establishes exclusion for one member, not the growing class: a different member can match the ledger filter. Thread and check every growing member, or prove equivalence across every filter-observable field before granting the relief.
Medium
[MED] The life-loss bound is knowingly fail-open for mixed loss. resource.rs:535-559 and 607-614 explicitly state that max is sound only when unattributed loss is zero, yet production code uses observed_life_loss.max(declared_life_magnitude). With untargeted loss plus a declared re-aimable slot, this understates per-period loss and can permit an in-shortcut elimination. Encode the attribution-aware bound, or withhold the optimization until its premise is established; add a mixed-loss regression. CR 732.2a requires a legal predictable sequence and excludes conditional actions.
[MED] A zero iteration bound is normalized into an offered repetition in release. crates/engine/src/game/interaction.rs:2454-2469 has only a debug assertion before clamp(1, ...); release then publishes a one-iteration shortcut despite the producer declaring that no repetition is legal. Reject max_iterations == 0 as invalid authority state rather than normalizing it, and add a production-path regression.
[MED] Negative observer tests lack sufficient reach guards. The no-offer cases at loop_shortcut.rs:4399-4423, 4499-4505, 5906-5913, and 5941-5948 do not first bind and verify that the bystander is present on the battlefield with the expected parsed observer definition. Without those checks, an absent or misparsed observer can make a no-offer assertion pass without exercising the intended veto. Add the observer-presence and expected-shape guards before each negative assertion.
Evidence above was rechecked at 95165b5f8247c6cbd18e8f9ff4d872ec7c69fdc5. CI still has one Rust-test shard pending; that is non-dispositive follow-up, not a substitute for addressing these blockers.
Resolves the review findings on phase-rs#6838. - Elimination bound is fail-closed: `observed_life_loss.max(0) + declared_life_magnitude` replaces `observed.max(declared)`, which admitted an unattributed-loss elimination inside a proposed shortcut (CR 732.2a forbids conditional actions whose outcome determines the next choice). - Foreign-observer relief now requires `AbilityKind::Activated` and an absent `activator_filter` (CR 602.2): a filtered or non-activated ability is not relieved by sole-driver. - Class-member exclusion is universally quantified at both the ETB and ledger sites, with an explicit non-empty guard — `Iterator::all` on an empty set returns true, which previously granted relief to every empty class. - `max_iterations == 0` is rejected rather than clamped; clamping panicked in release builds via `Ord::clamp`'s `min <= max` assert. - Ring invalidation checks snapshot length before `zip`, whose silent truncation hid a moved life total. - Observer-negative rows gain parse reach-guards so a misparse fails loudly instead of passing vacuously. - CR anchors re-seated to the sub-rule each sentence actually claims (510.1c->510.1, 702.154a->702.154b, 608.2i->608.2j, 120.3->120.1). Assisted-by: ClaudeCode:claude-opus-4.8
95165b5 to
77cacc3
Compare
|
🤖 AI text below 🤖 All five findings from your 14:06Z review are fixed on the new head Rebased onto Each fix was verified at the exact coordinate you cited, not merely in the same
On the empty-set edge in the [HIGH] ledger finding: Two-sided non-vacuity for the reach guards: with the observer text corrupted and the One change beyond your findings, disclosed: the rebase surfaced a silent Verification on the rebased head: CI is running on the new head; I am watching it and will report here if anything is red. |
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/parser/oracle_trigger_tests.rs (1)
3418-3424: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winAssert the complete typed filters.
valid_card.is_some()does not prove that the subject is the equipped creature. The debug-string check does not prove that the destruction target is a tapped nonland permanent controlled by the defending player. A parser regression can drop one of these constraints and still pass this test.Match the typed filter structures and assert each required component directly.
Also applies to: 3445-3451
🤖 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/parser/oracle_trigger_tests.rs` around lines 3418 - 3424, Strengthen the assertions around the equipped-creature subject and destruction target to match the complete typed filter structures directly. Verify the equipped-creature filter in def.valid_card, including its required components, and verify the destruction target filter represents a tapped nonland permanent controlled by the defending player rather than relying on presence or debug strings. Apply the same typed-structure assertions to the corresponding block around the second location.Source: Path instructions
🧹 Nitpick comments (1)
crates/engine/tests/integration/loop_shortcut.rs (1)
5218-5227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRelax the exact driven-beat assertion to a lower bound.
assert_eq!(beats, 6, ...)pins an exact beat count produced by a generic drive policy over a real dump. The anti-vacuity property this row needs is onlybeats > 0. Any unrelated change to legal-action enumeration order, prompt sequencing, or the fixture makes this row red for a reason that has nothing to do with crown re-derivation. Keep the measured value in the message for provenance, and assert a lower bound.♻️ Proposed change
- assert_eq!( - beats, 6, - "the `c decline` arm re-derives the crown after a measured 6 driven beats — a 0 here \ - would mean the row read the dump's saved offer back instead of re-deriving one, \ - which is what makes both revert-probes above live" - ); + assert!( + beats > 0, + "the `c decline` arm must re-derive the crown from live beats (measured: 6) — a 0 \ + here would mean the row read the dump's saved offer back instead of re-deriving \ + one, which is what makes both revert-probes above live; got {beats}" + );🤖 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/loop_shortcut.rs` around lines 5218 - 5227, Update the anti-vacuity assertion for beats in the loop-shortcut test from an exact value check to a lower-bound check requiring beats > 0. Preserve the existing measured beats value in the failure message and retain the surrounding crown re-derivation validation.Source: Path instructions
🤖 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.
Outside diff comments:
In `@crates/engine/src/parser/oracle_trigger_tests.rs`:
- Around line 3418-3424: Strengthen the assertions around the equipped-creature
subject and destruction target to match the complete typed filter structures
directly. Verify the equipped-creature filter in def.valid_card, including its
required components, and verify the destruction target filter represents a
tapped nonland permanent controlled by the defending player rather than relying
on presence or debug strings. Apply the same typed-structure assertions to the
corresponding block around the second location.
---
Nitpick comments:
In `@crates/engine/tests/integration/loop_shortcut.rs`:
- Around line 5218-5227: Update the anti-vacuity assertion for beats in the
loop-shortcut test from an exact value check to a lower-bound check requiring
beats > 0. Preserve the existing measured beats value in the failure message and
retain the surrounding crown re-derivation validation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 75e5e01f-3665-4687-82a6-a8b1b72f6d71
⛔ Files ignored due to path filters (4)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (14)
crates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/combat_damage.rscrates/engine/src/game/engine.rscrates/engine/src/game/interaction.rscrates/engine/src/game/quantity.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/triggers.rscrates/engine/src/game/visibility.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/loop_shortcut.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- crates/engine/src/game/visibility.rs
- crates/engine/src/analysis/loop_check.rs
- crates/engine/src/game/quantity.rs
- crates/engine/src/game/triggers.rs
- crates/engine/src/analysis/decision_template.rs
- crates/engine/src/game/combat_damage.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/game/restrictions.rs
- crates/engine/src/analysis/resource.rs
|
Current-head hold for Fresh implementation review confirms that the five prior soundness blockers are repaired on this exact head: foreign-observer relief is limited to unfiltered non-mana activated abilities; ETB and ledger relief quantify over every nonempty growing-class member; the elimination bound charges observed and declared loss additively; a zero shortcut bound is rejected; and the retained-window fixture is migrated to the This is not an approval yet. Rust lint and both Rust test shards remain in progress, while the only |
… windows
A merely-riding observer suppressed the combo shortcut offer: the loop-detection
ring was cleared at two independent sites, so any forced pre-priority window
discarded every sampled frame and detection restarted from empty. Measured on a
real 4p dump: clears were {apply_action: 1152 at len 0, sampler: 16 at len 2} --
the sampler wiped a len-2 ring exactly once per 99-beat period.
- WaitingFor::is_forced_cascade_window() classifies the windows the engine forces
before any player receives priority (CR 117.1) -- trigger ordering, trigger
target selection, optional-effect choice, commander zone choice, the legend
rule (CR 704.5j) and battle protector designation (CR 310.10, a state-based
action). Fail-closed via '_ => false' over the remaining 121 variants.
- Both clear sites now honour the class. Fixing either alone yields zero
retention, so the pair is the unit of change.
- ResourceVector gains the axes the multiplayer gain/drain proof needs;
elimination_bounds now carries an explicit conditional-soundness contract
naming the attribution precondition its max() form requires.
Assisted-by: ClaudeCode:claude-opus-4.8
…-window class to turn-based actions Two coupled changes that land together on purpose: the widening is what makes the fence necessary, so splitting them would leave an intermediate commit in which a retained frame pair can straddle unobserved combat damage. Widening (CR 703.1 / CR 117.3a). is_forced_cascade_window() gains the seven turn-based-action windows the engine forces before any player receives priority: untap choice, bounded untap subset, declare attackers, exert, enlist, declare blockers, cleanup discard. Measured on a real 4-player dump, the ring was force-cleared exactly once per 99-beat turn period at declare-attackers, capping it at 2 frames; the widened class reaches 13. Retention across turns is necessary but NOT yet sufficient for the cross-turn shortcut CR 732.2a contemplates -- loop_states_equal still compares turn_number -- and the docs now say that rather than claiming otherwise. Damage fence (CR 510.2 / CR 119.3). The class doc excluded AssignCombatDamage as life-moving, but that window opens only when a damage-division choice is required. An unblocked attacker deals damage with no window at all, so the exclusion protected nothing once declare-attackers became exempt. The prohibition is now keyed on the damage EVENT: one guard in the shared apply_combat_damage, placed after the prevention riders so it catches Phase C damage, lifelink and CR 615.5 riders alike. The predicate is != rather than <, because CR 119.3 moves life in both directions. Per-batch (not hoisted) capture is what keeps double-strike correct. AssignCombatDamage/AssignBlockerDamage remain non-members, now documented with the CR 510.2 rationale rather than left to inference. CombatTaxPayment is documented as a life-mover (CR 508.1h + CR 107.4f: Phyrexian attack taxes), correcting a false claim that the turn-based members change no life. Tests: the exempt-window cast proof now fails loudly on class drift in both directions, and every member row carries a reach-guard, so a fixture offering no legal answer can no longer pass on an inert zero -- which caught three pre-existing vacuous rows. Two fixtures that could not occur in a real game (a player attacking themselves; a singleton trigger-ordering prompt the engine auto-orders) were corrected rather than left as passing scaffolding. Assisted-by: ClaudeCode:claude-opus-4.8
… a CR 732.2a loop (CR 117.1b / CR 510.2)
The CR 732.2a object-growth firewall vetoed the loop-shortcut offer for two
classes of bystander that provably cannot observe the loop:
* CR 117.1b + CR 732.2c — no player but the sole driver receives priority
inside the taken shortcut, so an OPPONENT-controlled activated ability
cannot be activated during the window. CR 605.3a bounds this: a mana
ability is activatable outside the priority rule, so it is NOT relieved.
The relief is per-ABILITY, never per-object.
* CR 510.2 / CR 506.1 (CR 500.1 for the phase list) — a trigger whose event
is confined to a phase or step the window provably never reaches cannot
fire inside the loop. `TriggerMode::Phase` with a differing `phase`, plus
the combat-damage family gated on `damage_kind == CombatOnly`.
Both proofs come from one new authority, `window_scope_from_cover_frames`,
which populates the already-plumbed `LoopWindowScope` at both suppressing
covers. Every guard sits inside `if let Some(..)`, so `unproven()` reaches
none of them and the 2-arg wrappers stay identity
(`scoped_wrappers_are_identity`, unmodified).
Shape pins, both asserted with paired polarities:
* the `Phase` arm is STRICT inequality. Relieving `p == phase` is a
SOUNDNESS change, not a precision one — CR 117.3a puts beginning-of-phase
abilities on the stack BEFORE the priority at which CR 732.2a lets a
shortcut be proposed, and CR 608.2h reads their information at resolution,
inside the window.
* the damage arm REQUIRES `CombatOnly`; a `damage_kind: Any` trigger fires
on noncombat damage in any phase.
* every unclassified `TriggerMode` falls to `_ => false` and keeps its veto.
Park Heights Pegasus and Smuggler's Share stop suppressing by the
PHASE/STEP mechanism, not by filter-matching: Pegasus's ledger filter is
`Typed{Creature}`, which genuinely does match a Saproling token. The
mis-attributing in-tree note is corrected and the row renamed accordingly.
Assisted-by: ClaudeCode:claude-opus-4.8
…r casts from does not observe a CR 732.2a loop (CR 601.2f) CR 601.2f vs CR 604.1 / CR 613.1: a `StaticMode::ModifyCost` static whose `affected` is `SelfRef`, sitting on a card the loop window provably never casts, cannot modify any cost paid inside the window — so its condition's read of a projected player resource is not an observation of the loop. The proof is `LoopWindowScope::cast_card_ids`, derived at the projected cover's conjunct (5) by the new `window_cast_card_ids`. Two soundness properties, both pinned by tests: * CR 601.2a — the derivation takes every `LoopActionContext::card_id` regardless of `action`, a SUPERSET of the true cast set (only `Recast` actually casts). Over-stating the cast set makes `!ids.contains(..)` false more often, i.e. FEWER relieved defs. Narrowing to `Recast` is a precision upgrade for a successor, with its own paired-polarity row. * an EMPTY `last_loop_action_sequence` yields `None`, never `Some(vec![])`. Empty means NO RECORDED PROOF, not "this window casts nothing"; the latter would relieve every conditioned self-cost static. `None` = scan everything. Extracting the derivation gives the emptiness contract its own directly callable seam, and a second row drives the real 2-arg cover predicate so the conjunct-(5) BINDING is pinned too — measured, degrading that binding to `Some(&[])` re-opens the fail-open while every helper-level row still passes. Evidence includes the real 4-player Dina/Conqueror capture, whose obj 90 Mortality Spear (library-visible conditioned `ModifyCost`/`SelfRef`) is measured to be the ONLY projected-resource-reading fire-time surface on the board, so the flip is attributable to it alone. `scoped_wrappers_are_identity` is unmodified: the guard sits inside an `is_some_and`, so `LoopWindowScope::unproven()` never reaches it. Assisted-by: ClaudeCode:claude-opus-4.8
… growing class does not observe a CR 732.2a loop (CR 608.2i)
The CR 732.2a loop-shortcut firewall's growing-class axis vetoes any fire-time
condition that reads a mutable sibling surface. Block (1)'s `execute` scan
vetoed on the read alone, with no look at WHAT the read counts: a trigger whose
body is gated on `BattlefieldEntriesThisTurn { filter }` where the filter
provably cannot match the growing class reads a value that is INVARIANT across
the loop's growth, so it does not observe the loop (CR 608.2h + CR 608.2i).
Narrowing is four fail-closed conjuncts, all required, in
`execute_ledger_condition_provably_excludes_class`:
(0) `activation_restrictions` must be empty — the firewall is blind to them,
so a restricted ability fails closed rather than being reasoned about.
(a) sole-source: a single-field clone with `condition: None` must NOT still
read a sibling-mutable surface, i.e. the ledger condition is the ONLY
such read in the ability. No visitor (phase-rs#4603's refusal stands).
(b) shape: exactly `QuantityCheck { lhs: Ref(BattlefieldEntriesThisTurn),
rhs: Fixed }` at a single level; everything else `=> false`.
(c) exclusion: `battlefield_entry_matches_filter` must reject BOTH the
synthesized entry record for the live class member AND every real
recorded entry for it. Fail-closed if the member is absent from the
scanned frame.
MEASURED DELTA, with its population predicate: on today's
`data/card-data.json` the relief population is ZERO — no real card is relieved
by this narrowing. The three cards the directive names are relieved by the
foreign-controller / phase-unreachability narrowings in the two preceding
commits, not by this one. 2c ships a SHAPE, as scaffolding, with that null
delta disclosed rather than papered over. Its soundness residual is likewise
measured and disclosed in the predicate's doc: of `BattlefieldEntryRecord`'s
8 fields the fodder relation (`object_content_eq`, 32 compared fields) covers
only `name` + `controller`, and four of the five uncompared fields are read
verdict-bearingly by a live filter on today's pool — so the residual is
REACHABLE, not latent, and must be re-derived if the card pool is regenerated.
Also extracts `battlefield_entry_record_for` in `game/restrictions.rs` so the
read-only firewall can build an entry record without `&mut GameState`;
`record_battlefield_entry` now calls it, giving the 8-field list one authority.
Behaviour-identical (identical field list, `object_id: obj.id` is the one
mechanically-forced substitution for the removed parameter).
Assisted-by: ClaudeCode:claude-opus-4.8
…canonical form
CI "Rust lint (fmt, clippy, parser gate)" failed at its first step,
`cargo fmt --all -- --check`, with a single hunk:
Diff in crates/engine/src/game/triggers.rs:8:
- TriggerDefinitionRef,
- TriggerEntry, TriggerGrantProducerKey, TypeFilter, TypedFilter,
+ TriggerDefinitionRef, TriggerEntry, TriggerGrantProducerKey, TypeFilter, TypedFilter,
The `TriggerDefinitionOccurrenceRef` import added earlier in this chain
pushed the list past a wrap point without being reflowed, leaving a short
line rustfmt joins. Import-list rewrap only — no semantic change.
Assisted-by: ClaudeCode:claude-opus-5
…ract
Review finding [MED] "New production-fixture regression does not load"
(matthewevans, 2026-07-31): CI Rust shard 1/2 failed loading the four-player
dump at crates/engine/tests/integration/loop_shortcut.rs:4713-4718 with
`missing field effect_kind`, so `two_site_retention_survives_a_prompt_and_its_answer`
never reached the apply pipeline.
Measured cause: the committed blob predates the `effect_kind` serialization
contract. Inflated + key-sorted, the blob differs from its migrated twin by
exactly one line, `+ "effect_kind": "LoseLife"`.
The new blob is extracted BYTE-IDENTICALLY from the already-migrated tree
(`git show <migrated-tip>:<path>`) rather than regenerated by a migration
script, and no `serde(default)` shim was added to `effect_kind`. Byte identity
is the load-bearing property: it keeps this file a no-op when the two histories
are folded, instead of a binary conflict.
Discriminating evidence (both runs local, same test binary):
- pre-fix blob => FAILED at loop_shortcut.rs:4717,
`Error("missing field \`effect_kind\`", line: 0, column: 0)`
- post-fix blob => ok, 60.41s
The test's existing post-restore reach guards (`state.loop_detection.samples()`
and the block below it) are retained unchanged, per the review's request to
keep a load reach guard.
Assisted-by: ClaudeCode:claude-opus-5
Resolves the review findings on phase-rs#6838. - Elimination bound is fail-closed: `observed_life_loss.max(0) + declared_life_magnitude` replaces `observed.max(declared)`, which admitted an unattributed-loss elimination inside a proposed shortcut (CR 732.2a forbids conditional actions whose outcome determines the next choice). - Foreign-observer relief now requires `AbilityKind::Activated` and an absent `activator_filter` (CR 602.2): a filtered or non-activated ability is not relieved by sole-driver. - Class-member exclusion is universally quantified at both the ETB and ledger sites, with an explicit non-empty guard — `Iterator::all` on an empty set returns true, which previously granted relief to every empty class. - `max_iterations == 0` is rejected rather than clamped; clamping panicked in release builds via `Ord::clamp`'s `min <= max` assert. - Ring invalidation checks snapshot length before `zip`, whose silent truncation hid a moved life total. - Observer-negative rows gain parse reach-guards so a misparse fails loudly instead of passing vacuously. - CR anchors re-seated to the sub-rule each sentence actually claims (510.1c->510.1, 702.154a->702.154b, 608.2i->608.2j, 120.3->120.1). Assisted-by: ClaudeCode:claude-opus-4.8
77cacc3 to
5b0a9ff
Compare
|
🤖 AI text below 🤖 Head moved since my last comment — for re-review, the current head is The branch has since been rebased onto current CI on Your 14:06Z changes-requested review is recorded against On #6839: understood, and it stays draft. I will rebase it and present a fresh non-draft |
|
🤖 AI text below 🤖 Both gates from your 19:13Z hold are satisfied on the current head 1. Current-head CI — complete and green. Rust lint and both test shards finished after 2. The
Body still reads 3. No longer behind On head identity: your implementation review confirmed the five repairs against Nothing further is pending on my side; #6839 remains draft and I will rebase and present it |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on the rebased current head. git range-diff confirms the prior-reviewed eight-commit series is content-equivalent after rebasing onto current main; the repaired loop-soundness paths, discriminating regressions, current green CI, and current no-change parse artifact all recheck clean.
2 participants
🤖 AI text below 🤖
Summary
Chain 1 of the combo-detector player-feedback epic (phases 0–2): the loop-detection ring survives forced pre-priority windows (with a CR 510.2 damage fence widening the forced-window class to turn-based actions), and three observer classes that provably cannot observe a CR 732.2a loop are ruled out of loop-window vetoes (foreign/phase-unreachable observers, zone-unreachable conditioned self-cost statics, ledger observers whose filter cannot count the growing class).
Files changed
crates/engine/src/analysis/decision_template.rs— window decision plumbingcrates/engine/src/analysis/loop_check.rs— ring retention across forced windowscrates/engine/src/analysis/resource.rs— observer-negative scopingcrates/engine/src/game/combat_damage.rs— CR 510.2 fencecrates/engine/src/game/engine.rs,game/interaction.rs,game/quantity.rs,game/restrictions.rs,game/triggers.rs,game/visibility.rs— forced-window class + observer rulingscrates/engine/src/types/game_state.rs— ring statecrates/engine/tests/fixtures/{dellian_emblem_conqueror,dina_conqueror,tenacity_exquisite_blood,witherbloom_sprout_lumaret}_4p.json.gz— real-game 4p dump fixturescrates/engine/tests/integration/{interaction_contract,loop_shortcut}.rs— drive-level coveragecrates/engine/src/game/triggers.rs— plus a follow-upstyle(engine)commit (471484867) rewrapping the ability import list to rustfmt canonical form (one hunk, no semantic change), fixing thecargo fmt --all -- --checkCI failureTrack
Developer
LLM
Model: claude-fable-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 732.2a (loop observation), CR 510.2 (combat-damage turn-based actions), CR 117.1b (priority windows), CR 601.2f (casting legality), CR 608.2i (resolution instructions / ledger observer scope).
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head. (CI-owned alternative stated below, per author-directed remote-CI reliance for chain submissions.)
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
Branch-tip battery at
04b6bb600(contains this range, rebased on73fd7f6de):cargo check --workspace— 0 errors;cargo clippy --workspace --all-targets -- -D warnings— 0;cargo test -p phase-enginelib — 18124 passed / 0 failed; integration — 4304 passed / 0 failed;cargo test -p phase-ai— 1930 passed / 0 failed.FORGE_TEST_FULL_DB=1 ordering_parity_sweeptwo-arm (merge-base vs tip, identical corpus) —unexplained=18both arms, 18-row card lists byte-identical, 18 over-prompt / 0 under-prompt both — pre-existing on main, delta-0.scripts/ai-perf-gate.shtwo-arm (merge-base vs tip, identical corpus) — 30-row counter tables byte-identical, delta-0 (6 failures pre-existing environment drift, present on both arms).Each phase in this range closed through the full
/engine-implementerpipeline with review-impl run to zero findings at phase close (lane journal; pre-rebase phase-close SHAs413b963f0/3f1b9f7f7phases 0–1, phase-2 close recorded in-journal).Gate A
Deferred to the CI-owned parse-diff check for this submission (author-directed remote-CI reliance for the PR chain). head=471484867 base=02760f588. No
parser//oracle*paths in this range (file list above) — expected parse delta is zero.Anchored on
crates/engine/src/analysis/loop_check.rs— existing loop-ring window authority this range extends (retention rides the same ring, no parallel state)crates/engine/src/game/restrictions.rs— existing ledger/observer filter seam the observer-negative rulings scope (rulings narrow the existing filter; no new observer type)Final review-impl
Final review-impl PASS at each phase close (zero findings; lane journal), re-verified at branch tip
04b6bb600by the full boundary battery above.Claimed parse impact
None.
Scope Expansion
None.
Validation Failures
None.
CI Failures
None. (Rebased onto upstream/main
02760f5882026-07-31; chain rebased as a unit — phase-0-2 remains a strict prefix of phase-3-4. Conflicts: none —git rebase --ontoreplayed all 8 chain commits clean, andgit range-diff 73fd7f6de..22780493c 02760f588..7cf16a49creports 8/8=(patch-identical, zero content drift).)Chain & series context
This PR is chain link 1 of the player-feedback epic. Chain protocol: each successor PR is draft and its diff includes this PR's commits until this PR merges; on merge, the successor rebases, its diff reduces to its own arc, and it flips to non-draft. Chain management (rebases as review lands) is handled by the author.
Predecessor series (combo-detector): #4119 (PR-2 loop-certificate drives) · #4480 (PR-3 mandatory-loop winner) · #4493/#4534 (PR-4a/4b combo-graph extractor + breadth) · #4547 (PR-5 combo-verify CLI) · #4603 (PR-6 ∞ display) · #5672 (PR-7 offer + APNAP response window + declaration UI) · #6238 (drive & collapse accepted shortcuts) · #6259 (review fixes).
Summary by CodeRabbit
New Features
Bug Fixes
Tests