Skip to content

fix(parser): bind "that token" inside a reflexive "when you do" body (CR 603.12)#6621

Merged
matthewevans merged 3 commits into
phase-rs:mainfrom
lgray:fix/bbfu9-reflexive-seed-token-chain
Jul 25, 2026
Merged

fix(parser): bind "that token" inside a reflexive "when you do" body (CR 603.12)#6621
matthewevans merged 3 commits into
phase-rs:mainfrom
lgray:fix/bbfu9-reflexive-seed-token-chain

Conversation

@lgray

@lgray lgray commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Summary

chain_prior_referent_is_created_token bailed return false on the first prior clause carrying a
condition, before it ever reached the is_token_creating_effect check. In a reflexive body the
token-creating clause is exactly the clause that carries the gate — strip_if_you_do_conditional
stamps WhenYouDo / EffectOutcome{OptionalEffectPerformed} onto it — so a later "that token"
anaphor in the same body never saw the token and misbound. Iroh, Tea Master placed its +1/+1
counter on an unbound ParentTarget instead of the Ally it had just created; Summoner's Sending
bound TrackedSet{0}.

The referent walk now continues past a prior clause whose condition is an affirmative reflexive gate,
and the consuming clause is re-linked SequentialSiblingContinuationStep so the resolver's
condition-false descent cannot resolve it against state.last_created_token_ids — a game-lifetime
ledger that would otherwise hand it a token from an earlier resolution (CR 609.3: no legal
antecedent).

Built as building blocks, not special cases:

  • AbilityCondition::is_affirmative_reflexive_gate — exhaustive 53-arm match, no wildcard, so a
    future variant is a compile error rather than a silent reclassification. Its true set is exactly
    the image of parse_affirmative_reflexive_connector.
  • AbilityDefinition::is_self_gated_reflexive — a clone that carries the gate itself is honest
    wherever it lands.
  • oracle_ir::ast::sub_link_after_boundary and lower::instruction_spine_is_continuation extracted
    so the seeder and the re-link share one authority instead of two drifting copies.
  • The repeat-process clone guard asks its question directly: it builds the def the assembler would
    push (via the shared repeat_process_clone_shape), runs the re-link over it, and reads the
    resulting link back, rather than stacking structural proxies.

Files changed

  • crates/engine/src/parser/oracle_effect/mod.rs — seeder carve-out; repeat_process_clone_shape
    extracted as the single clone-shape authority
  • crates/engine/src/parser/oracle_effect/lower.rsrelink_gated_token_referent_consumers,
    gated_instruction_reaches, instruction_spine_is_continuation,
    clone_would_transplant_gated_referent
  • crates/engine/src/parser/oracle_effect/assembly.rs — re-link wired into chain assembly
  • crates/engine/src/parser/oracle_ir/ast.rssub_link_after_boundary shared authority
  • crates/engine/src/types/ability.rsAbilityCondition::is_affirmative_reflexive_gate,
    AbilityDefinition::is_self_gated_reflexive
  • crates/engine/tests/integration/reflexive_body_token_referent.rs — new, 21 durable tests
  • crates/engine/tests/integration/main.rsmod line

Note on #6533 (SiblingCondition::ReplicatedOrBranch)

attach_repeat_process_keywords is refactored here into the shared repeat_process_clone_shape,
because clone_would_transplant_gated_referent decides whether a binding is safe by building this
exact def and asking the re-link what it would do with it — if the two shapes diverged, the guard
would be answering a question about a def the assembler never pushes.

That loop is also where #6533 set sibling_condition = SiblingCondition::ReplicatedOrBranch, so the
assignment is folded into the shared helper (with the CR 608.2c rationale recorded there) rather
than left at the push site. Verified by revert-probe rather than asserted: dropping that one line from
the helper takes the suite from 29/29 → 27/29, failing exactly
mutable_pupa_perpetual_keyword_mirror::kathril_reaches_matching_counter_and_tail_past_false_earlier_gates
and oracle_ir::snapshot_tests::kathril_aspect_warper#6533's own behavioural + AST tests. The
separate perpetual-grant site is untouched, and all 21 tests here stay green in the drop arm, so
ReplicatedOrBranch is orthogonal to this PR's guard rather than interacting with it.

Track

Developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: max (planning / plan-review), high (implementation / impl-review)

Implementation method (required)

Method: /engine-implementer

CR references

  • CR 111.1 — tokens
  • CR 603.12 — reflexive triggered abilities
  • CR 608.2c — instructions carried out in the order written
  • CR 609.3 — an effect does only as much as it can; no legal antecedent
  • CR 701.23 — Search
  • CR 701.24 — Shuffle

All six re-grepped against docs/MagicCompRules.txt; 0 unverified.

Verification

  • Required checks ran clean, or the exact CI-owned alternative is stated below.
  • Gate A output below is for the current committed head.
  • Final review-impl below is clean for the current committed head.
  • Both anchors cite existing analogous code at the same seam.

All re-run on this head after rebasing onto b3bd4f910 (#6607 touches three of the four files this
PR changes). The rebase applied with zero conflicts, which is precisely why the full sweep was re-run
rather than carried forward — the post-rebase hazard is semantic drift a 3-way merge accepts.

  • cargo nextest run -p engine21710 passed / 0 failed, 8 skipped
  • cargo clippy --workspace --all-targets --features engine/proptest -- -D warnings0 warnings
  • cargo fmt --all --check — exit 0
  • scripts/check-parser-combinators.sh b3bd4f910Gate G PASS + Gate A PASS; no string-dispatch
    lines added
  • combo/loop detector suite (-E 'test(combo) or test(loop_shortcut) or test(loop_counter_growth) or test(chicago_loop)') — 111 passed / 0 failed
  • reflexive_body_token_referent durable suite — 21 passed / 0 failed
  • upstream drift tripwire (-E 'test(strategic_betrayal) or test(kathril) or test(mutable_pupa)') —
    21 passed / 0 failed. fix(engine): a player-directed "exile a creature they control" is a choice, not a target (Strategic Betrayal) #6607's own suite plus the two Kathril suites that fix(parser): distribute Mutable Pupa's perpetual keyword-mirror trigger per keyword #6533 added, both of
    which run through the repeat_process_clone_shape helper this PR extracts.
  • cargo combo-verify BASE vs POST — rows byte-identical
    (13 confirmed / 4 gated / 37 deferred / 0 failed of 54). Non-vacuous: the two regenerated
    card-data.json exports are confirmed to DIFFER, so the row identity is not a stale-binary artifact.

Every one of the 21 tests carries an executed revert-probe that flips, and every negative
assertion carries a reach-guard (verified by breaking its premise: T5's guard fires when
select_targets_and_resolve is removed, T4's when base_controller is dropped). Fixture fidelity:
all 40 embedded card texts diffed byte-verbatim against data/mtgjson/AtomicCards.json, 0 mismatches.

Gate A

Gate A PASS head=044574f3579e1128422ae220b91bfe4c3da9a7b1 base=b3bd4f910720b7141331dee9f18f81aa539dad5e

Anchored on

  • crates/engine/src/parser/oracle_effect/lower.rs:347gate_reflexive_rider_on_declined_optional_target,
    the existing post-lowering whole-definition pass that gates a reflexive rider on a declined optional
    antecedent (also annotated CR 608.2c). Same seam, same shape as relink_gated_token_referent_consumers.
  • crates/engine/src/parser/oracle_effect/lower.rs:296patch_choose_from_zone_counter_continuation_target,
    the existing post-lowering pass that rebinds a continuation's target when the head exposes the only
    legal referent. Same seam, same class-level ("general across the whole class") justification.

Claimed parse impact

20 cards, 25 chain nodes, every one SequentialSiblingContinuationStep, and exactly 2
PutCounter.target moves. No node added or removed on any card.

Two measurements, stated separately because they have different denominators and artifacts:

  • The 20-card set — re-measured on the current head against the shipped card-data.json
    (35477 entries), both arms regenerated from the identical feature set
    (--profile tool --features cli,combo-verify) so the commit is the only variable.
    Non-vacuity control: the two exports are confirmed to DIFFER, so neither arm ran a stale binary.
  • The 25-node / 2-target-move decomposition — measured at the AST level over all 34348
    AtomicCards.json entries. Post-rebase it is carried by T18 (invariant: 0 SequentialSibling
    nodes read LastCreated) and T21 (per-card PutCounter multisets), both green on this head.

Akoum Stonewaker · Blight Herder · Boulder Jockey · Cadric, Soul Kindler · Dalek Intensive Care ·
Felhide Spiritbinder · Flameshadow Conjuring · God-Pharaoh's Gift · Gyrus, Waker of Corpses ·
Inalla, Archmage Ritualist · Iroh, Tea Master · Kavaron Harrier · Krenko, Baron of Tin Street ·
Rebellion of the Flamekin · Saheeli, Radiant Creator · Séance · Summoner's Sending ·
Timothar, Baron of Bats · Ultron, Artificial Malevolence · Vile Redeemer

The two bolded cards are the PutCounter.target moves — the bug being fixed. The other 18 are the
sub_link re-tag only, which changes which parent the resolver descends from without changing what
resolves. T18 freezes this exact set as a corpus-complete stale-bind invariant.

Scope Expansion

None.

Validation Failures

None.

CI Failures

None.


Anchored on b3bd4f910 (#6607). 799b34220 (#6620, Devour) landed while the final sweep was
running; it touches triggers.rs / oracle_keyword.rs / types/keywords.rs / synthesis.rs /
ability_scan.rs and mtgish-import, none of which this PR touches — zero file overlap, so the
verification above stands on the stated base rather than being silently carried across it.

Summary by CodeRabbit

  • Bug Fixes
    • Fixed “Last Created” token binding so clauses after affirmative reflexive gates resolve to the correct gated continuation, not stale antecedents.
    • Prevented conditional decline branches from reusing tokens created on other paths.
    • Improved Repeat/replication handling to ensure cloned counter-placement stays aligned with the right gated publisher path.
    • Refined clause continuation and boundary handling for more accurate ability resolution.
  • Documentation
    • Clarified how sub-clause links and reflexive gate conditions affect token resolution.
  • Tests
    • Added a comprehensive integration test suite covering reflexive “Last Created” relinking and replication scenarios.

…(CR 603.12)

`chain_prior_referent_is_created_token` bailed `return false` on the first
prior clause carrying a `condition`, before it ever reached the
`is_token_creating_effect` check. In a reflexive body the token-creating
clause is exactly the clause that carries the gate — `strip_if_you_do_conditional`
stamps `WhenYouDo` / `EffectOutcome{OptionalEffectPerformed}` onto it — so a
later "that token" anaphor in the same body never saw the token and misbound.

Iroh, Tea Master placed its +1/+1 counter on an unbound `ParentTarget`
instead of the Ally it had just created; Summoner's Sending bound
`TrackedSet{0}`.

The referent walk now continues past a prior clause whose condition is an
affirmative reflexive gate, and the consuming clause is re-linked
`SequentialSibling` -> `ContinuationStep` so the resolver's condition-false
descent cannot resolve it against `state.last_created_token_ids` — a
game-lifetime ledger that would otherwise hand it a token from an earlier
resolution (CR 609.3: no legal antecedent).

Building blocks, not special cases:
- `AbilityCondition::is_affirmative_reflexive_gate` — exhaustive 53-arm match,
  no wildcard, so a future variant is a compile error rather than a silent
  reclassification. Its `true` set is exactly the image of
  `parse_affirmative_reflexive_connector`.
- `AbilityDefinition::is_self_gated_reflexive` — a clone that carries the gate
  itself is honest wherever it lands.
- `oracle_ir::ast::sub_link_after_boundary` and
  `lower::instruction_spine_is_continuation` extracted so the seeder and the
  re-link share one authority instead of two drifting copies.
- The repeat-process clone guard asks its question directly: it builds the def
  the assembler would push, runs the re-link over it, and reads the resulting
  link back, rather than stacking structural proxies.

Blast radius, measured over all 34348 AtomicCards entries: 20 cards, 25 chain
nodes, every one `SequentialSibling` -> `ContinuationStep`, and exactly 2
`PutCounter.target` moves (the two cards above). No node added or removed on
any card.

Verification: nextest -p engine 21635 passed / 0 failed; clippy --workspace
--all-targets --features engine/proptest -D warnings 0 warnings;
check-parser-combinators.sh Gate G + Gate A PASS; combo gate 98 passed / 0
failed; cargo combo-verify rows byte-identical BASE vs POST (13 confirmed /
4 gated / 37 deferred / 0 failed of 54) with the two card-data exports
confirmed to differ, so the identity is not vacuous.

21 durable regression tests, each with an executed revert-probe that flips.

Assisted-by: ClaudeCode:claude-opus-5
@lgray
lgray requested a review from matthewevans as a code owner July 25, 2026 05:44
@github-actions github-actions Bot added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 25, 2026
@coderabbitai

coderabbitai Bot commented Jul 25, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 4834120c-23bf-424c-b978-d4daa05e0cc4

📥 Commits

Reviewing files that changed from the base of the PR and between 044574f and 8680f05.

📒 Files selected for processing (2)
  • crates/engine/src/parser/oracle_effect/lower.rs
  • crates/engine/tests/integration/reflexive_body_token_referent.rs
🚧 Files skipped from review as they are similar to previous changes (2)
  • crates/engine/src/parser/oracle_effect/lower.rs
  • crates/engine/tests/integration/reflexive_body_token_referent.rs

📝 Walkthrough

Walkthrough

Changes

The parser now classifies reflexive gates, aligns clause-link decisions, relinks LastCreated consumers on valid continuation paths, filters hazardous repeat-process clones, and adds integration coverage for gate outcomes, AST invariants, and replication behavior.

Reflexive token referent handling

Layer / File(s) Summary
Gate predicates and clause-link contracts
crates/engine/src/types/ability.rs, crates/engine/src/parser/oracle_ir/ast.rs
Adds reflexive-gate predicates, clarifies SubAbilityLink, and centralizes clause-boundary-to-link mapping.
Gated referent lowering pass
crates/engine/src/parser/oracle_effect/lower.rs
Detects affirmative gated publishers and LastCreated consumers, validates continuation spines, relinks eligible consumers, and probes clone hazards.
Assembly and referent prediction
crates/engine/src/parser/oracle_effect/assembly.rs, crates/engine/src/parser/oracle_effect/mod.rs
Runs the relinking pass, filters repeat-process clones that would transplant gated referents, and updates token-referent prediction for affirmative gates.
Runtime and corpus validation
crates/engine/tests/integration/main.rs, crates/engine/tests/integration/reflexive_body_token_referent.rs
Adds gate-true, gate-false, AST-invariant, clone, and replication-corpus integration tests.

Estimated code review effort: 4 (Complex) | ~60 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OracleText
  participant Assembly
  participant Lowering
  participant AbilityDefinitions
  OracleText->>Assembly: parse and assemble ability definitions
  Assembly->>Lowering: relink gated token referent consumers
  Lowering->>AbilityDefinitions: inspect gates, links, and LastCreated reads
  Lowering->>AbilityDefinitions: retag eligible consumers as ContinuationStep
  Assembly->>AbilityDefinitions: filter hazardous repeat-process clones
Loading

Suggested labels: bug

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5
✅ Passed checks (5 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title accurately summarizes the main parser fix for binding “that token” in reflexive “when you do” bodies.
Docstring Coverage ✅ Passed Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/parser/oracle_effect/lower.rs`:
- Around line 2672-2680: Update filter_reads to use an exhaustive match over
every TargetFilter variant instead of the wildcard fallback, recursively
inspecting all composite or wrapper variants for nested LastCreated filters.
Preserve true for LastCreated and recursive any semantics for collections, with
the compiler enforcing handling when new variants are added.
- Around line 2672-2680: Update filter_reads in
crates/engine/src/parser/oracle_effect/lower.rs:2672-2680 to exhaustively match
TargetFilter and recurse through every composite variant that can contain
LastCreated instead of defaulting to false; also update the
ability_reads_last_created definition traversal at
crates/engine/src/parser/oracle_effect/lower.rs:2698-2705 to recurse through
def.mode_abilities alongside sub_ability and else_ability, matching
normalize_parsed_replacement_flags.

In `@crates/engine/tests/integration/reflexive_body_token_referent.rs`:
- Around line 143-153: Update the documentation above fund to state that it
conditionally grants mana only, and remove the claim that it grants energy or
controls Saheeli’s energy gate. Keep the energy-seeding invariant attributed to
drive_ship, which is the authority for T16’s {E}{E}{E} condition.
🪄 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: 69b736eb-26d6-4ce8-bd6c-44032675c7fd

📥 Commits

Reviewing files that changed from the base of the PR and between 799b342 and 044574f.

📒 Files selected for processing (7)
  • crates/engine/src/parser/oracle_effect/assembly.rs
  • crates/engine/src/parser/oracle_effect/lower.rs
  • crates/engine/src/parser/oracle_effect/mod.rs
  • crates/engine/src/parser/oracle_ir/ast.rs
  • crates/engine/src/types/ability.rs
  • crates/engine/tests/integration/main.rs
  • crates/engine/tests/integration/reflexive_body_token_referent.rs

Comment thread crates/engine/src/parser/oracle_effect/lower.rs
Comment thread crates/engine/tests/integration/reflexive_body_token_referent.rs Outdated
@github-actions

github-actions Bot commented Jul 25, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main 6346c267d473)

🟡 Modified fields (2 signatures)

  • 1 card · 🔄 ability/PutCounter · changed field target: parent targetlast created
    • Affected (first 3): Iroh, Tea Master
  • 1 card · 🔄 ability/PutCounter · changed field target: tracked set #0last created
    • Affected (first 3): Summoner's Sending

@matthewevans matthewevans self-assigned this Jul 25, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved after maintainer repair and exact-head review.

Evidence: the recursive TargetFilter walk now covers Not, TrackedSetFiltered, and ChosenDamageSource with explicit terminal arms; mode_abilities are traversed; wrapper/modal regressions exercise the re-link; the fixture comment now matches the mana/energy setup. cargo fmt --all, diff check, security review, and pre-commit parser gates passed. No direct builds/tests/clippy were run.

@matthewevans matthewevans added enhancement New feature or request and removed needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) labels Jul 25, 2026
@matthewevans
matthewevans enabled auto-merge July 25, 2026 06:12
@matthewevans matthewevans removed their assignment Jul 25, 2026
@github-actions github-actions Bot added the needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps) label Jul 25, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 25, 2026
Merged via the queue into phase-rs:main with commit be6883b Jul 25, 2026
14 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request needs-maintainer AI-contribution PR requires human triage (Non-dev track or unresolved gaps)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants