fix(parser): guard an existing player_scope + pin the reason in #6965's fail-closed tests - #7009
Conversation
…e fail-closed tests (#6965) Six findings from the CodeRabbit review of #7003, which merged before these landed. **Correctness.** `bind_recipient_without_recipient_slot` assigned `def.player_scope = Some(scope)` unconditionally while documenting a "Total and FAIL-CLOSED" contract. That contract only covered recipients no `PlayerFilter` can name — not a body that already carries its own iteration scope. Both halves are rewritten from clones of ONE parsed body, so an unguarded stamp would replace a printed per-player fan-out with a single recipient, and with a different one on each half. Refuse when a scope is already present; the caller already turns `false` into `Unimplemented`. **CR citations.** CR 301.5f defines what an ability means by "equipped creature"; it does not define attachment, which is CR 301.5a. Both sites now cite the rule whose body describes the code. CR 109.4 is about which objects have controllers, so it does not describe Infernal Offering's "that player" — a player chosen while applying the effect, CR 608.2d, which the doc already cited. Dropped it there. The targeted-player fail-closed contract now cites CR 115.1, which is the rule that actually makes those conjuncts targets. **Vacuous negative assertions.** Two tests accepted any `Effect::Unimplemented` and so would have passed on a clause that died earlier, or on a dropped effect. Both now assert the gap is named `unbound_subject` and quotes the conjunct that caused it. `recipient_less_body_with_a_targeted_player_conjunct_fails_closed` gains a positive reach guard, since a dead distributor fails closed on everything including what it should bind — the guard carries the full Infernal Offering text, because "that player" without its preceding "Choose an opponent." is itself an unbound subject. The Wand of Orcus test proved only that a trigger reached the stack, which a dropped execute effect would also satisfy while granting no deathtouch; it now inspects the trigger's chain before resolution. **Docs.** `SubjectPhraseAst::affected` claimed one consumer reads it. `sync_subject_into_nested_shuffle_sub` and `inject_subject_target` read it too, via `target.or(affected)`. Narrowed to the invariant actually intended: `ImperativeFallback` is the only consumer treating `None` as a coverage gap; the others treat it as "nothing to rebind". `None` is reachable in all three. Two citations kept deliberately. CR 608.2c stays on the Life at Stake test: it asserts chain order mirroring printed order, which is what that rule governs. CR 109.4 stays on the same test's `ParentObjectTargetController` binding, where the player IS named through a battlefield object's controller. Verified: parser tests 1583 passed; engine lib 18487 passed; integration 4487 passed; clippy -D warnings clean. The new reach guard was watched go red first (it caught a wrong positive example, which is how the antecedent requirement above was found).
|
Warning Review limit reached
Next review available in: 19 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 (4)
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 |
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
1 participant
Follow-up to #7003 (merged), addressing the CodeRabbit review it did not wait for.
Correctness (the one that isn't cosmetic)
bind_recipient_without_recipient_slotassigneddef.player_scope = Some(scope)unconditionally while its doc promised a "Total and FAIL-CLOSED" contract. That contract covered recipients noPlayerFiltercan name — it did not cover a body that already carries its own iteration scope.Both halves are rewritten from clones of one parsed body, so an unguarded stamp would replace a printed per-player fan-out with a single recipient, and with a different one on each half, silently resolving for the wrong set of players. The binder now refuses when a scope is already present; the caller already converts
falseintoUnimplemented, so this falls through to an honest gap.CR citations
Two kept deliberately. CR 608.2c stays on the Life at Stake test — it asserts chain order mirroring printed order, which is exactly what that rule governs. CR 109.4 stays on that test's
ParentObjectTargetControllerbinding, where the player genuinely is named through a battlefield object's controller.Vacuous negative assertions
Two tests accepted any
Effect::Unimplemented, so they would have passed on a clause that died earlier — or, in the Wand case, on a dropped execute effect that also grants no deathtouch. Both now assert the gap is namedunbound_subjectand quotes the conjunct that caused it.recipient_less_body_with_a_targeted_player_conjunct_fails_closedgains a positive reach guard, because a dead distributor fails closed on everything, including what it should bind. Worth noting how that guard was built: my first attempt used a bare"You and that player each sacrifice a creature."and went red — "that player" without its preceding "Choose an opponent." is itself an unbound subject. The guard now carries the full Infernal Offering text.Docs
SubjectPhraseAst::affectedclaimed one consumer reads it.sync_subject_into_nested_shuffle_subandinject_subject_targetread it too viatarget.or(affected). Narrowed to the invariant actually intended:ImperativeFallbackis the only consumer treatingNoneas a coverage gap; the others treat it as "nothing to rebind".Noneis reachable in all three — stated so a future edit doesn't assume otherwise.Verification
cargo clippy -p phase-engine --all-targets -- -D warningsclean