fix(parser): stop failing open on an unparseable subject, bind targeting compound subjects (#6965) - #7003
Merged
Merged
Conversation
`parse_subject_application` returning `None` was replaced with
`SubjectApplication { affected: TargetFilter::Any, .. }` at both
subject-predicate sites that re-derive a subject. `TargetFilter::Any`
matches unconditionally (`game/filter.rs`), so a parse FAILURE produced a
BOARD-WIDE effect — the grant landed on every permanent the controller
had, lands and artifacts included, while coverage still reported the card
as supported.
Measured on the emitted AST (`oracle-gen`, 35,657 faces): 49 sites over 48
cards applied a modification through a fabricated `Any` filter. All 49
originate at those two sites — removing the default takes them to 0.
Fail closed by construction, not by patch:
* `SubjectPhraseAst.affected` becomes `Option<TargetFilter>`, so "the
subject grammar could not bind this phrase" is a state the type holds
and the fail-open is unrepresentable. Sibling of
`EntersUnderSpec::UnboundAnaphor`.
* The one consumer that applies the subject filter (the
`ImperativeFallback` arm of `lower_subject_predicate_ast`) emits
`Effect::unimplemented("unbound_subject", <whole printed clause>)`.
The `Continuous`/`Become`/`Restriction` arms never read it, so failing
closed there would have been inert over-firing — measured at 195 extra
cards made red for no correctness gain.
* The decision, and the options not taken, are recorded on
`subject::UNBOUND_SUBJECT_GAP`.
Then parse the construction the issue names. CR 611.2c: one effect naming
several subjects determines each part's object set independently, so a
compound subject is the UNION of its conjuncts.
`parse_conjoined_subject_application` splits at a word-boundary `and` and
parses each conjunct with the ordinary single-subject grammar, recursing
on the right for N-ary lists; `merge_or_filters` flattens. This replaces
the single compound arm hardcoded to the literal phrase "you and
permanents you control", which it reproduces byte-for-byte. It runs last,
so it can only convert an unbound subject into a bound union.
A conjunct must be non-targeting and *unionable*. Two rejection classes,
both found by measurement:
* a non-discriminating filter (`Any`, or a default `TypedFilter`) —
Model of Unity's unmodelled "who voted for a choice you voted for"
collapsed to one, and `Or[Controller, <default>]` let every player
scry: the same fail-open wearing an `Or`;
* an event-context anaphor, which resolves through the target/binding
channel — `Or[TriggeringSource, Typed(Zombie, You)]` granted deathtouch
to the Zombies and not to the equipped creature, a half-applied grant
that still reports as supported.
Failing closed also surfaced three player subjects the grammar should
always have bound, masked until now by the fabricated filter: "you may"
(controller's own permission grant — not `is_optional`, the permission is
itself the opt-in), "they each" (distributive emphasis on an already
plural pronoun), and "that opponent" (the "that player" anaphor with the
noun narrowed).
Coverage 92.1% -> 91.9%: 88 cards move from supported to unsupported,
which is what they are. The 21 sites that legitimately carry
`affected: Any` — the CR 305.1 play/plot permissions built in
`oracle_static/restriction.rs` (Omniscience, Future Sight, Bolas's
Citadel, Theater of Horrors, Fblthp, ...) — are byte-identical, and no
new `Any` site is created anywhere in the corpus.
Life at Stake — "You and target creature's controller each secretly choose a
number 0 or greater." — became an honest `Unimplemented("unbound_subject")`
when the fail-open closed: `parse_conjoined_subject_application`'s CR 611.2c
union declines a TARGETING conjunct, because unioning it into one filter would
lose the target binding.
The union is the wrong shape for this class. `try_parse_compound_subject_each`
already distributes a shared predicate across two recipients as a `sub_ability`
chain, which keeps each recipient separately bound. Two things stopped it from
covering the class.
* **The conjunct names its player through an object.** New second-subject axis
`parse_possessive_actor_each_second_subject` delegates the conjunct to the
single-subject grammar (`parse_subject_application`) — the established
authority for "target <filter>'s controller/owner", which resolves the actor
to `ParentTargetController`/`ParentTargetOwner` (CR 109.4) while preserving
the announced object as the ability's target (CR 115.1). It is gated on BOTH
halves being present, so it never fabricates a target slot. The distributor
then emits that target as a leading `Effect::TargetOnly` head — the same
slot-only declaration the single-subject grammar emits — so the recipient
reference (and the card's later "exile that creature" anaphor) has a slot to
resolve against (CR 601.2c). The prefix parsers now return a
`CompoundSubjectPrefix` rather than a widening tuple, and the shared
"you"/"~" first-subject alt is factored into one combinator.
* **`Effect::Choose` has no recipient slot.** `rewrite_recipient_on_link` bound
recipients by writing a `TargetFilter` field, and returned `false` for every
effect family without one. Such an effect's acting player is the resolving
ability's controller, so the recipient belongs on the ABILITY:
`bind_recipient_without_recipient_slot` stamps `player_scope`, the same lift
the single-subject grammar already performs for a slot-less predicate ("its
controller investigates" — `player_scope_from_parent_target_subject`, reused
here rather than duplicated).
That binding is total and FAIL-CLOSED. "you" needs no scope (CR 109.5 — the
printed controller already is the unscoped acting player); a parent-target
actor and a resolution-chosen player map to existing `PlayerFilter` variants;
everything else returns `false`. In particular a TARGETED player ("you and
target opponent each flip a coin" — Mana Clash; "… each secretly choose 1, 2,
or 3" — Expert-Level Safe) stays `Unimplemented`, because no `PlayerFilter`
names one: `PlayerFilter::Opponent` would make EVERY opponent act in a
multiplayer game. A regression test pins that, and fails against the
over-broad mapping.
The chunk-splitter guard (`remainder_trimmed_starts_with_compound_subject_each`)
delegates to the same axis combinator, so the two sites cannot drift.
Measured on the emitted AST (`oracle-gen`, 35,657 faces): 32028 → 32030 fully
implemented, zero cards newly broken. The two are Life at Stake and Infernal
Offering ("You and that player each sacrifice a creature" — `Effect::Sacrifice`
is likewise slot-less, and its conjunct is the opponent the preceding "Choose an
opponent." picked).
The new integration test drives the real parse → cast → resolution pipeline and
asserts the only thing that matters at runtime: the number prompts go to P0 then
P1, not twice to the caster. Pre-fix it observes `[]`.
…he fail-open (#6965) Both tests asserted shapes that were only reachable because an unparseable subject fell open to a filter matching unconditionally, which is the defect this issue removes. Each said so in its own doc comment. Keen Duelist pinned `RevealTop` as the trigger root and gapped only the lose clause — but the comment named the binding it depended on, `RevealTop { player: Any }`. "you and target opponent each reveal" has a TARGETED player as its second conjunct, and no PlayerFilter names one; PlayerFilter::Opponent would make every opponent reveal in multiplayer. So the subject now fails closed and the whole trigger is an honest unbound_subject gap. The lose clause sits behind it, so the test no longer routes through lose_node (Parker Luck still does). Angel of Destiny pinned `GainLife { player: Controller }` and conceded the gap in the same breath: the damaged player never gained life. That is a half-applied effect the caster benefits from, counted as SUPPORTED in coverage — a silent misparse rather than a visible gap. It reached GainLife at all only because the unbindable subject fell open. Neither original intent is lost. Both existed to stop an unresolved subject from being laundered into a concrete recipient, and an Unimplemented gap satisfies that more completely than a Controller default did. Both now assert the gap names the SUBJECT as the unbound part and quotes the conjunct that caused it, so a clause failing elsewhere no longer passes as fail-closed coverage. Both carry a forward-red note: binding a targeted player, or "that player" as the damage-event player, will red them, which is the prompt to assert the real shape. Integration suite: 4487 passed, 0 failed.
matthewevans
enabled auto-merge
August 4, 2026 22:42
Contributor
📝 WalkthroughWalkthroughThe parser now supports possessive actor compound subjects, represents unbound subjects explicitly, rejects unsafe conjuncts, and lowers unresolved subjects to ChangesCompound subject parsing
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant CompoundSubjectParser
participant PossessiveActorParser
participant EffectBuilder
OracleText->>CompoundSubjectParser: parse compound subject
CompoundSubjectParser->>PossessiveActorParser: parse target filter's controller or owner
PossessiveActorParser-->>CompoundSubjectParser: return filters and declared target
CompoundSubjectParser->>EffectBuilder: build distributed effect
EffectBuilder-->>EffectBuilder: prepend Effect::TargetOnly
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/parser/oracle_effect/subject.rs (1)
2676-2700: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winKeep the
"you may"subject out of continuous effects.
try_parse_additive_type_continuous_clausereturns early only on the exact player subject"you", so accepting"you may"inparse_subject_applicationlets the same player subject reachbuild_continuous_clause. Exclude"you may"at the continuous clause guard, or route permission grants through a separate subject path that cannot classify imperative one-shots as P/T or keyword modifications.🤖 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_effect/subject.rs` around lines 2676 - 2700, The newly added `"you may"` case in parse_subject_application must not flow into continuous-effect handling. Update try_parse_additive_type_continuous_clause or its guard to explicitly reject the exact `"you may"` subject while preserving support for ordinary `"you"` and other player-subject forms; do not alter the permission-grant SubjectApplication semantics.crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs (1)
315-356: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftExercise fail-closed effects through the runtime pipeline.
Both tests inspect lowered AST nodes only. Neither test resolves the affected ability. A later execution-path regression can partially apply an
unbound_subjecteffect while both tests remain green.
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs#L315-L356: trigger Angel of Destiny through combat damage and resolve it. Assert that the unsupported clause grants life to neither player.crates/engine/tests/integration/parker_luck.rs#L139-L166: trigger Keen Duelist at upkeep, select the opponent, and resolve it. Assert that the unsupported clause performs neither reveal nor life loss.As per path instructions, “a parser AST shape test does NOT prove runtime 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/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs` around lines 315 - 356, The tests currently inspect lowered AST nodes instead of exercising runtime behavior. In crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:315-356, update the Angel of Destiny test to trigger the ability through combat damage and resolve it, asserting the unsupported life-gain clause grants life to neither player. In crates/engine/tests/integration/parker_luck.rs:139-166, update the Keen Duelist test to trigger at upkeep, select the opponent, and resolve it, asserting the unsupported clause performs neither reveal nor life loss.Source: Path instructions
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/subject.rs (1)
9411-9416: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueState that "perpetually" is outside the Comprehensive Rules.
The comment calls
perpetuallythe "Alchemy permanence marker". A recorded preference for this repository asks thatperpetuallybe identified explicitly as a digital-only Alchemy extension outside the Comprehensive Rules, so a reader does not look for a CR section that defines it. Add that qualifier.📝 Proposed comment tweak
/// Fixture is By Elspeth's Command mode 2, VERBATIM. `"It perpetually"` is /// the real stranded-adverb shape: `find_predicate_start` splits at the verb - /// `gets`, leaving the Alchemy permanence marker on the subject side, which - /// no subject arm binds. Before the fix this clause emitted a static with + /// `gets`, leaving `perpetually` on the subject side, which no subject arm + /// binds. `perpetually` is a digital-only Alchemy extension and is not + /// defined anywhere in the Comprehensive Rules, so no CR reference applies + /// to it. Before the fix this clause emitted a static with /// `affected: TargetFilter::Any` — the grant landed on every permanent.Based on learnings: treat "perpetually" as a digital-only Alchemy extension outside the Comprehensive Rules; explicitly identify it as such in the comment.
🤖 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_effect/subject.rs` around lines 9411 - 9416, Update the fixture comment near the “perpetually” reference to identify it explicitly as a digital-only Alchemy extension outside the Comprehensive Rules, while preserving the existing explanation of its subject-side parsing behavior.Source: Learnings
🤖 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/parser/oracle_effect/mod.rs`:
- Around line 18661-18676: Update bind_recipient_without_recipient_slot to
return false when def.player_scope is already set, before assigning the scope
derived from filter. Preserve the existing OriginalController handling, scope
lookup, and assignment only for definitions without an existing player scope so
rewrite_recipient_chain fails closed instead of overwriting iteration scope.
- Around line 18323-18346: Update parse_possessive_actor_each_second_subject and
its caller to preserve the original-case subject span and thread the enclosing
ParseContext through parsing instead of using ParseContext::default(). Parse the
possessive controller/owner target with a tentative context, return or commit
the resulting context as required, and ensure subtype-bearing targets retain
their original casing while preserving relative scope.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 617-626: Update the test around parse_effect_chain to assert the
exact fail-closed result, requiring Effect::Unimplemented with name
"unbound_subject" rather than any unimplemented effect. Add a positive reach
guard covering the shared compound-subject parsing path so the test confirms
that targeted-player subjects reach the intended unbound-subject handling before
asserting player_scope is None.
- Around line 416-426: Correct the CR citations in the documented tests: remove
CR 608.2c from the comments around the compound-choice cases unless they
explicitly test resolving instructions in written order, and remove CR 109.4
from the comments around “that player” or “target opponent” cases. Replace each
removed citation with a verified rule directly describing the tested target,
controller, choice, or action behavior, while preserving citations such as CR
115.1 where they accurately describe targets.
In `@crates/engine/src/parser/oracle_ir/ast.rs`:
- Around line 196-201: Update the documentation near the subject field to remove
the claim that ImperativeFallback is the only consumer reading it. State instead
that lower_subject_predicate_ast’s ImperativeFallback arm is the only consumer
treating None as a coverage gap, while sync_subject_into_nested_shuffle_sub and
inject_subject_target treat None as nothing to rebind.
In `@crates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs`:
- Line 24: Replace the incorrect CR 301.5f citations in the test comments near
the compound-subject attachment assertions with citations matching the
implemented attachment relation: use CR 301.5 and CR 301.5a, or CR 301.5b when
the attachment is caused by an ability. Update both cited locations while
preserving the test behavior.
- Around line 99-126: Before calling advance_until_stack_empty in the Wand of
Orcus trigger test, inspect the triggered stack item's execute chain and assert
it contains Effect::Unimplemented with name "unbound_subject" plus the expected
diagnostic. Keep the existing stack_names reach guard and runtime no-Deathtouch
assertions, ensuring the negative assertions remain protected by proof that this
specific unbound-subject path was reached.
- Around line 167-182: Extend the integration test around runner and the
existing Lazotep Plating scenario with a production-path player-targeting
assertion. Have P1 attempt to target P0 while Plating is active and assert the
action is rejected because P0 has player hexproof. Keep the existing permanent
assertions and ensure the new check verifies the compound subject preserves the
“you” player binding.
---
Outside diff comments:
In `@crates/engine/src/parser/oracle_effect/subject.rs`:
- Around line 2676-2700: The newly added `"you may"` case in
parse_subject_application must not flow into continuous-effect handling. Update
try_parse_additive_type_continuous_clause or its guard to explicitly reject the
exact `"you may"` subject while preserving support for ordinary `"you"` and
other player-subject forms; do not alter the permission-grant SubjectApplication
semantics.
In
`@crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs`:
- Around line 315-356: The tests currently inspect lowered AST nodes instead of
exercising runtime behavior. In
crates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rs:315-356,
update the Angel of Destiny test to trigger the ability through combat damage
and resolve it, asserting the unsupported life-gain clause grants life to
neither player. In crates/engine/tests/integration/parker_luck.rs:139-166,
update the Keen Duelist test to trigger at upkeep, select the opponent, and
resolve it, asserting the unsupported clause performs neither reveal nor life
loss.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/subject.rs`:
- Around line 9411-9416: Update the fixture comment near the “perpetually”
reference to identify it explicitly as a digital-only Alchemy extension outside
the Comprehensive Rules, while preserving the existing explanation of its
subject-side parsing behavior.
🪄 Autofix
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: d74acc83-1aa1-431c-8f0a-eec3cb426fdf
⛔ Files ignored due to path filters (1)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__karn_legacy_reforged_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (10)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/subject.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/tests/integration/issue_6381_benevolent_offering_repeat_opponent.rscrates/engine/tests/integration/life_at_stake_both_choosers_6965.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/parker_luck.rscrates/engine/tests/integration/wand_of_orcus_compound_subject_6965.rs
Comment on lines
+18323
to
+18346
| pub(super) fn parse_possessive_actor_each_second_subject( | ||
| rest: &str, | ||
| ) -> Option<(usize, TargetFilter, TargetFilter)> { | ||
| let (remaining, (first_filter, second_filter)) = ( | ||
| alt(( | ||
| value( | ||
| TargetFilter::OriginalController, | ||
| tag::<_, _, OracleError<'_>>("you and "), | ||
| ), | ||
| value(TargetFilter::SelfRef, tag("~ and ")), | ||
| )), | ||
| let (remaining, subject) = terminated( | ||
| take_until::<_, _, OracleError<'_>>(" each "), | ||
| tag::<_, _, OracleError<'_>>(" each "), | ||
| ) | ||
| .parse(rest) | ||
| .ok()?; | ||
| let application = | ||
| subject::parse_subject_application(subject.trim(), &mut ParseContext::default())?; | ||
| if !matches!( | ||
| application.affected, | ||
| TargetFilter::ParentTargetController | TargetFilter::ParentTargetOwner | ||
| ) { | ||
| return None; | ||
| } | ||
| let declared_target = application.target?; | ||
| Some(( | ||
| rest.len() - remaining.len(), | ||
| application.affected, | ||
| declared_target, | ||
| )) | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the possessive-suffix subject arm is ctx-dependent, and that it is the
# only producer of ParentTargetController/Owner WITH a declared target.
set -euo pipefail
rg -n -C12 --type=rust "ParentTargetController," crates/engine/src/parser/oracle_effect/subject.rs \
| rg -n -C12 'parse_target_with_ctx|target: Some'
# All producers of an application with affected=ParentTargetController|Owner and a target.
ast-grep run --lang rust \
--pattern 'SubjectApplication {
affected: $A,
target: Some($T),
$$$
}' \
crates/engine/src/parser
# Does parse_target build Subtype filters from the passed-in casing?
ast-grep run --lang rust --pattern 'TypeFilter::Subtype($X)' crates/engine/src/parser/oracle_target.rsRepository: phase-rs/phase
Length of output: 5996
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== oracle_effect mod relevant functions and call sites =="
sed -n '17330,18455p' crates/engine/src/parser/oracle_effect/mod.rs | cat -n | sed -n '1,130p'
sed -n '18455,18525p' crates/engine/src/parser/oracle_effect/mod.rs | cat -n
echo "== subject possessive arm =="
sed -n '2225,2285p' crates/engine/src/parser/oracle_effect/subject.rs | cat -n
echo "== target Subtype capitalization behavior =="
sed -n '3875,3905p' crates/engine/src/parser/oracle_target.rs | cat -n
echo "== parse_subject_application implementation and callers =="
rg -n -C8 --type=rust "fn parse_subject_application|parse_subject_application\\(" crates/engine/src/parser/oracle_effectRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== exact function body for possessive second subject =="
sed -n '18280,18360p' crates/engine/src/parser/oracle_effect/mod.rs | cat -n
echo "== lowercased producer call sites =="
sed -n '18360,18445p' crates/engine/src/parser/oracle_effect/mod.rs | cat -n
echo "== TextPair Subtype capitalization behavior =="
rg -n -C15 --type=rust "fn capitalize_first|capitalize_first\\(" crates/engine/src/parser/oracle_effect target.rs || true
echo "== parse_target_context signature and relative_player_scope usage =="
rg -n -C6 'fn parse_target_with_ctx|relative_player_scope|controller: Some\\(ctx|ControllerRef::ChosenPlayer' crates/engine/src/parser/oracle_effect/subject.rsRepository: phase-rs/phase
Length of output: 10755
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== capitalized_first definitions/usages =="
rg -n -C8 --type=rust "capitalize_first|TypeFilter::Subtype\(capitalize_first" crates/engine/src/parser
echo "== parse_target_with_ctx relative_player_scope controller suffix code =="
rg -n -C5 --type=rust 'fn parse_target_with_ctx|relative_player_scope|parse_target_with_ctx\(|target that player controls|that player controls' crates/engine/src/parser/oracle_effect/oracle_target.rs crates/engine/src/parser/oracle_effect/subject.rs
echo "== try_parse_compound_subject_each exact producer path =="
sed -n '18525,18595p' crates/engine/src/parser/oracle_effect/mod.rs | cat -nRepository: phase-rs/phase
Length of output: 45364
Thread the original case and context through the possessive second-subject parser.
Lines 18301-18304 pass subject.trim() and a fresh ParseContext::default() into parse_subject_application. The possessive-controller/owner arm parses the inner target with parse_target_with_ctx(subject, ctx), so “that player controls” loses the enclosing relative scope and falls back to ControllerRef::You. The caller and consumers also receive lowercased text, so a future subtype-bearing form would build TypeFilter::Subtype("zombie") for an Oracle subtype like “Zombie”. Keep the original-case span (for subtypes) and use a tentative/returned context for ParseContext state.
🤖 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_effect/mod.rs` around lines 18323 - 18346,
Update parse_possessive_actor_each_second_subject and its caller to preserve the
original-case subject span and thread the enclosing ParseContext through parsing
instead of using ParseContext::default(). Parse the possessive controller/owner
target with a tentative context, return or commit the resulting context as
required, and ensure subtype-bearing targets retain their original casing while
preserving relative scope.
Comment on lines
+18661
to
+18676
| fn bind_recipient_without_recipient_slot( | ||
| def: &mut AbilityDefinition, | ||
| filter: &TargetFilter, | ||
| ) -> bool { | ||
| // CR 109.5: "you" — the printed controller already IS the acting player of | ||
| // an unscoped ability, so this half needs no scope. Stamping one would be a | ||
| // redundant single-player fan-out. | ||
| if matches!(filter, TargetFilter::OriginalController) { | ||
| return true; | ||
| } | ||
| let Some(scope) = distribution_recipient_player_scope(filter) else { | ||
| return false; | ||
| }; | ||
| def.player_scope = Some(scope); | ||
| true | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Guard def.player_scope before overwriting it.
bind_recipient_without_recipient_slot assigns def.player_scope = Some(scope) without checking the existing value. The function documents a "Total and FAIL-CLOSED" contract, but that contract only covers recipients no PlayerFilter can name. It does not cover a body that already carries its own iteration scope.
try_parse_compound_subject_each calls rewrite_recipient_chain twice on clones of one parsed body (Lines 18505-18512). If any link of that body already has a player_scope — for example a body that itself reads "each opponent s" — then half A and half B each overwrite that scope with a different recipient. The printed per-player iteration is lost and the effect resolves for the wrong set of players.
Return false when a scope is already present, so the distribution falls through to Effect::Unimplemented instead of silently rebinding.
🛡️ Proposed fail-closed guard
if matches!(filter, TargetFilter::OriginalController) {
return true;
}
let Some(scope) = distribution_recipient_player_scope(filter) else {
return false;
};
+ // CR 109.4: the body already declares its own per-player iteration
+ // ("each opponent <verb>s"). Overwriting it would silently replace the
+ // printed player set with this one recipient, so fail closed instead.
+ if def.player_scope.is_some() {
+ return false;
+ }
def.player_scope = Some(scope);
true📝 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.
Suggested change
| fn bind_recipient_without_recipient_slot( | |
| def: &mut AbilityDefinition, | |
| filter: &TargetFilter, | |
| ) -> bool { | |
| // CR 109.5: "you" — the printed controller already IS the acting player of | |
| // an unscoped ability, so this half needs no scope. Stamping one would be a | |
| // redundant single-player fan-out. | |
| if matches!(filter, TargetFilter::OriginalController) { | |
| return true; | |
| } | |
| let Some(scope) = distribution_recipient_player_scope(filter) else { | |
| return false; | |
| }; | |
| def.player_scope = Some(scope); | |
| true | |
| } | |
| fn bind_recipient_without_recipient_slot( | |
| def: &mut AbilityDefinition, | |
| filter: &TargetFilter, | |
| ) -> bool { | |
| // CR 109.5: "you" — the printed controller already IS the acting player of | |
| // an unscoped ability, so this half needs no scope. Stamping one would be a | |
| // redundant single-player fan-out. | |
| if matches!(filter, TargetFilter::OriginalController) { | |
| return true; | |
| } | |
| let Some(scope) = distribution_recipient_player_scope(filter) else { | |
| return false; | |
| }; | |
| // CR 109.4: the body already declares its own per-player iteration | |
| // ("each opponent <verb>s"). Overwriting it would silently replace the | |
| // printed player set with this one recipient, so fail closed instead. | |
| if def.player_scope.is_some() { | |
| return false; | |
| } | |
| def.player_scope = Some(scope); | |
| true | |
| } |
🤖 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_effect/mod.rs` around lines 18661 - 18676,
Update bind_recipient_without_recipient_slot to return false when
def.player_scope is already set, before assigning the scope derived from filter.
Preserve the existing OriginalController handling, scope lookup, and assignment
only for definitions without an existing player scope so rewrite_recipient_chain
fails closed instead of overwriting iteration scope.
Comment on lines
+416
to
+426
| /// CR 109.4 + CR 115.1 + CR 608.2c + CR 608.2d: Life at Stake — "You and target | ||
| /// creature's controller each secretly choose a number 0 or greater." | ||
| /// | ||
| /// The compound subject's second conjunct names its player THROUGH an announced | ||
| /// object target, so the parse must produce three things, not one: | ||
| /// 1. a `TargetOnly { creature }` head declaring the CR 115.1 target slot the | ||
| /// possessive reference (and the later "exile that creature" anaphor) read; | ||
| /// 2. a `Choose { NumberRange }` whose chooser is the printed controller | ||
| /// ("you", CR 109.5 — the unscoped resolver default); | ||
| /// 3. a SECOND `Choose { NumberRange }` bound to a DISTINCT chooser via | ||
| /// `player_scope: ParentObjectTargetController` (CR 109.4). |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Correct the CR citations.
At Lines 416-426 and 515-519, remove CR 608.2c unless the test documents resolution in written order. At Lines 562-566 and 605-610, remove CR 109.4 because it defines controllers of objects, not that player or target opponent. Use a verified rule that directly describes the tested target, controller, choice, or action behavior.
CR 109.4 applies to objects on the stack or battlefield. CR 115.1 defines targets. CR 608.2c only covers following instructions in written order. (media.wizards.com)
As per path instructions, rules-touching code must use a verified CR citation whose text describes the code. Based on learnings, cite CR 608.2c only for written instructions resolved in order.
Also applies to: 515-519, 562-566, 605-610
🤖 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_effect/tests.rs` around lines 416 - 426,
Correct the CR citations in the documented tests: remove CR 608.2c from the
comments around the compound-choice cases unless they explicitly test resolving
instructions in written order, and remove CR 109.4 from the comments around
“that player” or “target opponent” cases. Replace each removed citation with a
verified rule directly describing the tested target, controller, choice, or
action behavior, while preserving citations such as CR 115.1 where they
accurately describe targets.
Sources: Path instructions, Learnings
Comment on lines
+617
to
+626
| let ability = parse_effect_chain(text, AbilityKind::Spell); | ||
| assert!( | ||
| matches!(&*ability.effect, Effect::Unimplemented { .. }), | ||
| "{text:?} must fail closed, got {:#?}", | ||
| ability.effect | ||
| ); | ||
| assert_eq!( | ||
| ability.player_scope, None, | ||
| "{text:?} must not fabricate a fan-out scope" | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the intended fail-closed reason.
This test accepts any Effect::Unimplemented. It can pass if parsing fails before the targeted-player compound subject is reached. Assert the exact unbound_subject result and add a positive reach guard for the shared compound-subject path.
As per path instructions, negative parser assertions need a positive reach guard. The PR objective requires unbound subjects to lower as Effect::Unimplemented { name: "unbound_subject" }.
🤖 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_effect/tests.rs` around lines 617 - 626,
Update the test around parse_effect_chain to assert the exact fail-closed
result, requiring Effect::Unimplemented with name "unbound_subject" rather than
any unimplemented effect. Add a positive reach guard covering the shared
compound-subject parsing path so the test confirms that targeted-player subjects
reach the intended unbound-subject handling before asserting player_scope is
None.
Source: Path instructions
Comment on lines
+196
to
+201
| /// unrepresentable: every consumer must say what it does with `None`, and | ||
| /// the one consumer that actually reads this field | ||
| /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm, the only | ||
| /// predicate kind that applies the subject filter) fails closed to | ||
| /// `Effect::unimplemented`. Same shape, same reason, as | ||
| /// [`EntersUnderSpec::UnboundAnaphor`]. |
Contributor
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
Correct the "one consumer" claim in the doc.
The doc states that lower_subject_predicate_ast's ImperativeFallback arm is the one consumer that reads this field. Two other functions in crates/engine/src/parser/oracle_effect/mod.rs also read it: sync_subject_into_nested_shuffle_sub and inject_subject_target. Both use subject.target ... .or(subject.affected) and now early-return on None.
The invariant the doc wants to state is narrower: ImperativeFallback is the only consumer that treats None as a coverage GAP; the other two treat None as "nothing to rebind". State that instead, so a future edit does not assume None is unreachable in those helpers.
📝 Proposed doc correction
- /// unrepresentable: every consumer must say what it does with `None`, and
- /// the one consumer that actually reads this field
- /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm, the only
- /// predicate kind that applies the subject filter) fails closed to
- /// `Effect::unimplemented`. Same shape, same reason, as
+ /// unrepresentable: every consumer must say what it does with `None`. The
+ /// only consumer that applies this filter as a subject
+ /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm) fails closed
+ /// to `Effect::unimplemented`; the rebinding helpers
+ /// (`inject_subject_target`, `sync_subject_into_nested_shuffle_sub`) read it
+ /// only as a fallback after `target` and no-op on `None`. Same shape, same
+ /// reason, as
/// [`EntersUnderSpec::UnboundAnaphor`].📝 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.
Suggested change
| /// unrepresentable: every consumer must say what it does with `None`, and | |
| /// the one consumer that actually reads this field | |
| /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm, the only | |
| /// predicate kind that applies the subject filter) fails closed to | |
| /// `Effect::unimplemented`. Same shape, same reason, as | |
| /// [`EntersUnderSpec::UnboundAnaphor`]. | |
| /// unrepresentable: every consumer must say what it does with `None`. The | |
| /// only consumer that applies this filter as a subject | |
| /// (`lower_subject_predicate_ast`'s `ImperativeFallback` arm) fails closed | |
| /// to `Effect::unimplemented`; the rebinding helpers | |
| /// (`inject_subject_target`, `sync_subject_into_nested_shuffle_sub`) read it | |
| /// only as a fallback after `target` and no-op on `None`. Same shape, same | |
| /// reason, as | |
| /// [`EntersUnderSpec::UnboundAnaphor`]. |
🤖 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_ir/ast.rs` around lines 196 - 201, Update the
documentation near the subject field to remove the claim that ImperativeFallback
is the only consumer reading it. State instead that
lower_subject_predicate_ast’s ImperativeFallback arm is the only consumer
treating None as a coverage gap, while sync_subject_into_nested_shuffle_sub and
inject_subject_target treat None as nothing to rebind.
| //! | ||
| //! CR 611.2c: one continuous effect naming several subjects determines the set | ||
| //! each part applies to independently — i.e. the UNION of the named subjects. | ||
| //! CR 301.5f: an Equipment attaches to a creature. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Replace the incorrect CR 301.5f citations.
CR 301.5f defines “equipped creature.” It does not define Equipment attachment. Line 24 and Line 79 should cite CR 301.5 and CR 301.5a for the attachment relation, or CR 301.5b for attachment by an ability. (media.wizards.com)
As per path instructions, rules-touching code must use a CR citation whose rule body describes the code.
Also applies to: 79-80
🤖 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/wand_of_orcus_compound_subject_6965.rs` at
line 24, Replace the incorrect CR 301.5f citations in the test comments near the
compound-subject attachment assertions with citations matching the implemented
attachment relation: use CR 301.5 and CR 301.5a, or CR 301.5b when the
attachment is caused by an ability. Update both cited locations while preserving
the test behavior.
Source: Path instructions
Comment on lines
+99
to
+126
| // Reach-guard: the trigger really did fire and go on the stack. Without it | ||
| // the assertions below would pass vacuously on a card that never triggered. | ||
| assert_eq!( | ||
| runner.stack_names(), | ||
| vec!["Wand of Orcus".to_string()], | ||
| "the attack trigger must be on the stack, or nothing below is exercised" | ||
| ); | ||
|
|
||
| runner.advance_until_stack_empty(); | ||
| runner.state_mut().layers_dirty.mark_full(); | ||
| evaluate_layers(runner.state_mut()); | ||
|
|
||
| // The printed subject ("it and Zombies you control") carries an anaphor | ||
| // conjunct the union cannot bind, so the whole clause fails closed. Nothing | ||
| // is granted — most importantly, NOT everything. | ||
| for (id, label) in [ | ||
| (host, "the equipped creature"), | ||
| (zombie, "a Zombie you control"), | ||
| (bear, "an unrelated creature you control"), | ||
| (land, "a LAND you control"), | ||
| ] { | ||
| assert!( | ||
| !keywords(&runner, id).contains(&Keyword::Deathtouch), | ||
| "{label} must not gain deathtouch: the printed subject could not be \ | ||
| bound, so the clause is an honest gap (issue #6965 — it used to \ | ||
| become TargetFilter::Any and grant to every permanent)" | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Assert the expected unbound-subject state before resolution.
stack_names() proves that the trigger was created. It does not prove that its execute chain is Effect::Unimplemented { name: "unbound_subject", .. }. A regression that drops the execute effect can produce no deathtouch and keep this test green. Inspect the trigger definition before resolution, assert the expected gap and diagnostic, then retain the runtime assertions.
As per path instructions, a negative assertion needs a reach guard that proves the tested path was reached.
🤖 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/wand_of_orcus_compound_subject_6965.rs`
around lines 99 - 126, Before calling advance_until_stack_empty in the Wand of
Orcus trigger test, inspect the triggered stack item's execute chain and assert
it contains Effect::Unimplemented with name "unbound_subject" plus the expected
diagnostic. Keep the existing stack_names reach guard and runtime no-Deathtouch
assertions, ensuring the negative assertions remain protected by proof that this
specific unbound-subject path was reached.
Source: Path instructions
Comment on lines
+167
to
+182
| // CR 611.2c: both named subjects are covered. | ||
| assert!( | ||
| keywords(&runner, ally).contains(&Keyword::Hexproof), | ||
| "a creature you control is inside \"permanents you control\" and must \ | ||
| gain hexproof" | ||
| ); | ||
| assert!( | ||
| keywords(&runner, ally_land).contains(&Keyword::Hexproof), | ||
| "a LAND you control is a permanent you control and must gain hexproof" | ||
| ); | ||
| // The negative arm is non-vacuous: the two positives above prove the grant | ||
| // resolved at all. | ||
| assert!( | ||
| !keywords(&runner, foe).contains(&Keyword::Hexproof), | ||
| "an opponent's permanent is excluded by \"you control\"" | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Test the player half of the compound subject.
Both asserted subjects are permanents controlled by P0. They only cover permanents you control. If you is dropped or player_scope binds the wrong player, this test still passes. Add a production-path player-targeting assertion that proves P1 cannot target P0 while Lazotep Plating is active. Hexproof has distinct player and permanent semantics. (media.wizards.com)
As per path instructions, integration tests must exercise the relevant runtime behavior.
🤖 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/wand_of_orcus_compound_subject_6965.rs`
around lines 167 - 182, Extend the integration test around runner and the
existing Lazotep Plating scenario with a production-path player-targeting
assertion. Have P1 attempt to target P0 while Plating is active and assert the
action is rejected because P0 has player hexproof. Keep the existing permanent
assertions and ensure the new check verifies the compound subject preserves the
“you” player binding.
Source: Path instructions
|
Generated for head Parse changes introduced by this PR · 199 card(s), 121 signature(s) (baseline: main
|
Member
Author
|
All eight review findings are addressed in #7009, opened because this PR merged out of the queue before the review landed. Six accepted:
Two declined, with reasons:
One note on process, since it validated the reach-guard finding: my first attempt at the positive reach guard used a bare |
lgray
pushed a commit
to lgray/phase
that referenced
this pull request
Aug 5, 2026
…e fail-closed tests (phase-rs#6965) (phase-rs#7009) Six findings from the CodeRabbit review of phase-rs#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). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
4 tasks
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.
Closes #6965.
The defect
An unparseable subject fell open to
TargetFilter::Any— a filter that matches unconditionally. The card then "parsed successfully" and counted as SUPPORTED in coverage while doing something the Oracle text never says. That is worse than an honest gap, because a gap is visible and a fail-open is not.Three commits
1. Stop failing open on an unparseable subject. Removes the
Anyfallback; an unbindable subject now yields an honestEffect::Unimplemented { name: "unbound_subject" }.2. Bind both choosers of a targeting compound subject. The shape "You and <subject> each <verb>" has 20 corpus members in
AtomicCards.json(34,868 cards); 14 have a targeting second conjunct. Two seams, both general:parse_possessive_actor_each_second_subject) built from nomtake_until/tag, delegating the conjunct to the existingsubject::parse_subject_application— already the authority for"target X's controller". Gated on both halves, so it never fabricates a target. The"target <filter>'s controller/owner each"axis is general over any object noun and any body.Effect::Choosehas no recipient field, sobind_recipient_without_recipient_slotstampsAbilityDefinition.player_scope, reusing the existingplayer_scope_from_parent_target_subjectlift. Total and fail-closed: anything it cannot name returns false.Newly supported: Life at Stake, Infernal Offering.
Mana ClashandExpert-Level Safedeliberately stay fail-closed — their conjunct istarget opponent, and noPlayerFilternames a targeted player (PlayerFilter::Opponentwould make every opponent act in multiplayer).3. Repin two guards that were asserting the fail-open. Keen Duelist and Angel of Destiny both pinned shapes only reachable through the
Anyfallback, and both said so in their own doc comments (RevealTop { player: Any }; "the damaged player still doesn't gain life"). Their intent — never launder an unresolved subject into a concrete recipient — is preserved and strengthened: anUnimplementedgap satisfies it more completely than the old defaults did. Both now assert the gap names the subject as the unbound part and quotes the conjunct that caused it, so a clause failing elsewhere can't pass as fail-closed coverage.Coverage
Measured with two real
oracle-genruns in-worktree (99 MB emitted ASTs diffed by card), fix vs. both seams neutered:Heads-up on the ratchet: commit 1 removes the fail-open globally, which was previously measured at roughly −88 supported cards (92.1% → 91.9%). Those cards were never actually working — they were matching unconditionally. This may need a ratchet waiver or a baseline refresh; flagging rather than adjusting anything.
Verification
cargo fmt --allclean;cargo clippy -p phase-engine --all-targets -- -D warningsexit 0Opponent → PlayerFilter::Opponentmapping and confirming it redsdocs/MagicCompRules.txt: CR 109.4, CR 109.5, CR 115.1, CR 601.2c, CR 608.2c, CR 608.2d, CR 611.2cDeliberately not in scope
"That player" as the damage/reveal-event player.
Effect::GainLifeandEffect::RevealTopdo have recipient slots, so adding arms torewrite_recipient_on_linklooks like a two-line extension — but Angel of Destiny's second conjunct would bind toTargetFilter::ScopedPlayer, which in aDamageDonetrigger with noplayer_scopefalls back to the controller. The damaged player would not gain life and the caster would gain twice: a silent misparse, strictly worse than the honest gap. Needs a real binding for the event player first.Simultaneous hidden choice. "Secretly choose" implies neither player sees the other's number.
Effect::Chooseraises oneWaitingFor::NamedChoiceat a time, so P1 answers after seeing that P0 answered (never what). No simultaneous-hidden-choice primitive exists in the engine; adding one is its own unit.Summary by CodeRabbit
Bug Fixes
Tests