Fix Epic Experiment - #6996
Merged
Merged
Conversation
…me (Epic Experiment) Closes phase-rs#6960. `parse_cast_type_disjunction` handled exactly one shape: `" or "` between two bare core types, with an optional article and a required trailing "spell(s)"/ "card(s)". Everything else in the "cast from among them" class kept a bare `TargetFilter::ExiledBySource` with no card-type leg, so any card type could be cast from the exiled set. Rewritten as a per-axis composed grammar: opt(quantifier) opt(article) leg (sep leg)* head_noun reusing `oracle_nom::target::parse_type_filter_word` for the leg alphabet (core types AND subtypes) and mirroring `oracle_nom/enchant.rs` for the separator/list pair. `and`, `or`, `and/or` and serial commas all lower to `TypeFilter::AnyOf`: per CR 205.2b conjunction is a property of ADJACENT type words ("artifact creature"), while a connector enumerates alternatives. A literal `And` would be a total no-op — no card carries both Instant and Sorcery. Seven cards gain the gate they were missing: Epic Experiment, Ral Leyline Prodigy, Kylox "instant and/or sorcery spells" Collected Conjuring "up to two sorcery spells" Sanwell, Avenger Ace "a Vehicle or artifact creature spell" Wand of Wonder "up to X instant and/or sorcery spells" Scarlet Witch, Chaotic Avenger "a Hero or noncreature spell" Acceptance requires either two or more legs or a consumed quantifier, which is also the anti-swallow guard: "cast a spell from among them" (Aetherworks Marvel, Svella, Apex of Power) still yields no gate. The parser half alone was inert. The chain seam forwards every exiled card as the sub-ability's targets, so `target_ids` arrived non-empty and skipped the one site where the cast filter was applied — every exiled card got the permission regardless of type. `cast_from_zone` now retains only forwarded ids matching the clause's own legs, using a new `TargetFilter::without_exile_anaphor()` that discharges the `ExiledBySource` leg the seam already satisfied while preserving And/Or structure. Re-evaluating the anaphor here would be actively wrong: on a triggered ability it reads a snapshot captured before this ability's own exile step, which would drop every id and turn the bug into a total no-op. Scoped to filters that reference the exile anaphor, so explicitly targeted grants (Emry, Bring to Light, Urza) are untouched, and the 51 bare-anaphor rows residualize to None and keep the full forwarded set. Removes Scarlet Witch, Chaotic Avenger from parser-misparse-backlog root cause phase-rs#6. Epic Experiment stays under phase-rs#1: its "that weren't cast" cleanup clause is still dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
github-actions
Bot
added
the
needs-maintainer
Contributor
|
Warning Review limit reached
Next review available in: 47 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (1)
📝 WalkthroughWalkthroughThe PR adds compositional cast-type parsing, preserves filter structure across hand and exile restrictions, removes only exile anaphors during forwarded-target resolution, and adds parser and runtime regression coverage. ChangesCast-type filtering
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant TargetFilter
participant CastFromZone
participant CastPermission
OracleParser->>TargetFilter: Build composed cast filter
CastFromZone->>TargetFilter: Remove ExiledBySource anaphor
TargetFilter-->>CastFromZone: Return residual filter
CastFromZone->>CastPermission: Route permitted forwarded cards
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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: 2
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_effect/mod.rs (1)
22173-22184: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
_ => falsearm with an exhaustive match overTargetFilter.
cast_gate_names_a_card_typedecides whether a parsed gate is applied at all. Every non-matching arm collapses into_ => false. If a new compositeTargetFiltervariant is added later (a further nesting shape besideOr/And/Not), this helper silently reportsfalse,parse_cast_type_gatereturnsNone, and the cast permission loses its type restriction with no compiler error. That is the exact failure class issue#6960describes.List the leaf variants explicitly so the compiler forces a decision for each new variant.
♻️ Suggested shape
TargetFilter::Not { filter } => cast_gate_names_a_card_type(filter), - _ => false, + // Leaf / non-composite filters carry no type atoms of their own. + TargetFilter::Any + | TargetFilter::SelfRef + | TargetFilter::ParentTarget + | TargetFilter::ExiledBySource + | TargetFilter::LastRevealed => false, + // ... remaining leaf variants listed explicitly }As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".🤖 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 22173 - 22184, Update cast_gate_names_a_card_type to replace the wildcard _ => false arm with explicit matches for every remaining TargetFilter leaf variant, returning false for each current non-composite variant. Preserve recursive handling for Typed, Or, And, and Not so adding a new TargetFilter variant produces a compiler error requiring this helper to be updated.Source: Coding guidelines
🤖 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 23103-23117: Update the Some(TargetFilter::Typed(typed)) arm in
the hand-path gate handling to graft only genuinely type-only gates: require
typed.properties to be empty and avoid overwriting hand_filter.controller. Route
Typed gates containing properties, or otherwise richer restrictions, through the
existing And-leg path so matches_target_filter preserves mana-value, colour, and
controller constraints.
In `@crates/engine/src/types/ability.rs`:
- Around line 14543-14609: Update CastFromZone::resolve to detect ExiledBySource
when it appears in any recursive branch of an Or, rather than relying on
references_exiled_by_source()'s all-branch behavior. Use a separate recursive
any-branch predicate for linked-card collection and residual filtering of
forwarded targets, while preserving the existing all-branch semantics of
is_context_ref().
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 22173-22184: Update cast_gate_names_a_card_type to replace the
wildcard _ => false arm with explicit matches for every remaining TargetFilter
leaf variant, returning false for each current non-composite variant. Preserve
recursive handling for Typed, Or, And, and Not so adding a new TargetFilter
variant produces a compiler error requiring this helper to be updated.
🪄 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: 27199853-de98-42fc-a199-35004d4b0936
📒 Files selected for processing (6)
crates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/kiora_self_library_peek_cast.rsdocs/parser-misparse-backlog.md
Comment on lines
+14543
to
+14609
| /// CR 601.3 + CR 607.2a: This filter with the exile-set anaphor | ||
| /// (`TargetFilter::ExiledBySource`) removed — i.e. the clause's OWN | ||
| /// restrictions, expressed against a set of objects whose membership in the | ||
| /// exile link has *already* been established by whoever produced the set. | ||
| /// | ||
| /// Returns `None` when nothing but the anaphor remains. A bare | ||
| /// `ExiledBySource` restricts a pre-established set not at all, so its | ||
| /// consumers must filter nothing rather than re-derive the link — re-reading | ||
| /// it is not merely redundant, it is wrong whenever the link is younger than | ||
| /// the reader's view of it (a trigger's `linked_exile_snapshot` is captured | ||
| /// when the trigger is put on the stack, before the ability's own exile step | ||
| /// has run, so the anaphor leg would be false for every forwarded id). | ||
| /// | ||
| /// Structure: `And` legs residualize independently and re-conjoin; an `Or` | ||
| /// with a branch that residualizes away imposes nothing on any member and so | ||
| /// drops whole. `TrackedSetFiltered` keeps its own set membership and | ||
| /// residualizes only the filter nested under it. | ||
| /// | ||
| /// INVARIANT — `Not`: this helper does not descend into `Not`, and neither | ||
| /// does [`TargetFilter::references_exiled_by_source`], the predicate that | ||
| /// gates every consumer of this residual. The two rest on the same premise | ||
| /// and must be changed in lockstep: no production cast filter puts the | ||
| /// anaphor under a negation ("cards NOT exiled this way" describes no | ||
| /// printed clause), so a `Not` is always a genuine restriction and is | ||
| /// preserved verbatim. If a card ever makes that shape real, both helpers | ||
| /// must be taught about it in the SAME change — a residual that still | ||
| /// contains `ExiledBySource` would re-evaluate the very link this helper | ||
| /// exists to discharge, and (per the paragraph above) that re-read is false | ||
| /// for every forwarded id, so the grant would silently become a no-op. | ||
| /// `not_over_the_exile_anaphor_is_unreachable_and_pinned_in_lockstep` pins | ||
| /// the current agreement between the two. | ||
| pub fn without_exile_anaphor(&self) -> Option<TargetFilter> { | ||
| match self { | ||
| TargetFilter::ExiledBySource => None, | ||
| TargetFilter::And { filters } => { | ||
| let mut residual: Vec<TargetFilter> = filters | ||
| .iter() | ||
| .filter_map(TargetFilter::without_exile_anaphor) | ||
| .collect(); | ||
| match residual.len() { | ||
| 0 => None, | ||
| 1 => residual.pop(), | ||
| _ => Some(TargetFilter::And { filters: residual }), | ||
| } | ||
| } | ||
| TargetFilter::Or { filters } => { | ||
| let mut residual = Vec::with_capacity(filters.len()); | ||
| for filter in filters { | ||
| residual.push(filter.without_exile_anaphor()?); | ||
| } | ||
| Some(TargetFilter::Or { filters: residual }) | ||
| } | ||
| // The tracked-set membership is its own restriction and survives; only | ||
| // the anaphor nested under it is discharged. | ||
| TargetFilter::TrackedSetFiltered { | ||
| id, | ||
| filter, | ||
| caused_by, | ||
| } => Some(TargetFilter::TrackedSetFiltered { | ||
| id: *id, | ||
| filter: Box::new(filter.without_exile_anaphor().unwrap_or(TargetFilter::Any)), | ||
| caused_by: *caused_by, | ||
| }), | ||
| other => Some(other.clone()), | ||
| } | ||
| } | ||
|
|
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Description: Locate consumers of references_exiled_by_source and without_exile_anaphor
# to verify their gating relationship, especially around Or-nested ExiledBySource branches.
echo "--- references_exiled_by_source call sites ---"
rg -n -C4 'references_exiled_by_source' --type=rust
echo "--- without_exile_anaphor call sites ---"
rg -n -C6 'without_exile_anaphor' --type=rust
echo "--- CastFromZone::resolve implementation ---"
fd -i 'cast_from_zone.rs' --exec cat -n {}Repository: phase-rs/phase
Length of output: 199
🏁 Script executed:
#!/bin/bash
set -u
echo "--- repository Rust files matching the relevant names ---"
git ls-files | grep -E '(^|/)(ability|cast_from_zone)[.]rs$' || true
echo "--- all textual references in tracked files ---"
rg -n -C5 --hidden --glob '!target/**' --glob '!node_modules/**' \
'references_exiled_by_source|without_exile_anaphor|ExiledBySource' . || true
echo "--- tracked engine source layout ---"
git ls-files 'crates/engine/**' | grep -E '(cast|ability|filter)' | head -200 || trueRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "--- relevant tracked paths ---"
git ls-files | grep -E '(^|/)(ability|cast_from_zone)[.]rs$' || true
echo "--- Rust references only ---"
rg -n -C5 --glob '*.rs' \
'references_exiled_by_source|without_exile_anaphor|ExiledBySource' . || true
echo "--- cast_from_zone.rs paths and focused contents ---"
git ls-files | grep -E '(^|/)cast_from_zone[.]rs$' | while IFS= read -r f; do
echo "FILE: $f"
rg -n -C12 'references_exiled_by_source|without_exile_anaphor|impl .*CastFromZone|fn resolve' "$f" || true
doneRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -u
echo "--- relevant paths only ---"
git ls-files | grep -E 'ability[.]rs$|cast.*zone.*[.]rs$' | head -100
echo "--- exact without_exile_anaphor call sites ---"
rg -n --glob '*.rs' 'without_exile_anaphor' . || true
echo "--- exact references_exiled_by_source call sites ---"
rg -n --glob '*.rs' 'references_exiled_by_source' . | head -200 || true
echo "--- definitions and nearby implementations ---"
rg -n -A45 -B15 --glob '*.rs' \
'fn references_exiled_by_source|pub fn without_exile_anaphor' crates/engine/src || trueRepository: phase-rs/phase
Length of output: 17534
🏁 Script executed:
#!/bin/bash
set -u
echo "--- cast_from_zone.rs resolution path ---"
sed -n '300,475p' crates/engine/src/game/effects/cast_from_zone.rs
echo "--- focused tests for Or semantics and cast gating ---"
sed -n '24190,24355p' crates/engine/src/types/ability.rs
sed -n '1260,1320p' crates/engine/tests/integration/kiora_self_library_peek_cast.rs
echo "--- all cast_from_zone tests mentioning linked exile ---"
rg -n -C8 --glob '*.rs' \
'CastFromZone|without_exile_anaphor|references_exiled_by_source|linked_exile' \
crates/engine/src/game/effects/cast_from_zone.rsRepository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -u
echo "--- context-reference use of references_exiled_by_source ---"
sed -n '14610,14660p' crates/engine/src/types/ability.rs
echo "--- focused cast guards ---"
sed -n '340,365p;417,445p;447,460p' crates/engine/src/game/effects/cast_from_zone.rs
echo "--- deterministic shape verifier ---"
python3 - <<'PY'
from dataclasses import dataclass
`@dataclass`(frozen=True)
class Filter:
kind: str
children: tuple = ()
def references(f):
if f.kind == "ExiledBySource":
return True
if f.kind == "And":
return any(references(x) for x in f.children)
if f.kind == "Or":
return all(references(x) for x in f.children)
if f.kind == "TrackedSetFiltered":
return references(f.children[0])
return False
def residual(f):
if f.kind == "ExiledBySource":
return None
if f.kind == "And":
xs = [r for x in f.children if (r := residual(x)) is not None]
return None if not xs else Filter("And", tuple(xs)) if len(xs) > 1 else xs[0]
if f.kind == "Or":
xs = []
for x in f.children:
r = residual(x)
if r is None:
return None
xs.append(r)
return Filter("Or", tuple(xs))
if f.kind == "TrackedSetFiltered":
return Filter("TrackedSetFiltered", (residual(f.children[0]) or Filter("Any"),))
return f
shape = Filter("Or", (Filter("Typed"), Filter("ExiledBySource")))
print("references_exiled_by_source:", references(shape))
print("without_exile_anaphor:", residual(shape))
assert references(shape) is False
assert residual(shape) is None
print("The Or-nested shape is skipped by the CastFromZone references gate.")
PYRepository: phase-rs/phase
Length of output: 7260
Use an any-branch predicate in CastFromZone::resolve — references_exiled_by_source() returns false for Or { [X, ExiledBySource] }. The resolver then skips linked-card collection with empty targets and skips residual filtering for forwarded targets. Use a separate recursive predicate with .any() for this consumer; retain .all() for is_context_ref().
🤖 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/ability.rs` around lines 14543 - 14609, Update
CastFromZone::resolve to detect ExiledBySource when it appears in any recursive
branch of an Or, rather than relying on references_exiled_by_source()'s
all-branch behavior. Use a separate recursive any-branch predicate for
linked-card collection and residual filtering of forwarded targets, while
preserving the existing all-branch semantics of is_context_ref().
|
Generated for head Parse changes introduced by this PR · 12 card(s), 10 signature(s) (baseline: main
|
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests/integration/issue_5240_silent_blade_oni.rs`:
- Around line 255-262: The test setup in issue_5240_silent_blade_oni must cover
the creature-type restriction independently of mana value. Add a noncreature
card with mana value 2 to P1’s hand, then assert it is absent from the resolved
cards while retaining the existing mana-value-3 creature assertion.
🪄 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: 24833d2b-e144-4783-a550-df096bf3fbdc
📒 Files selected for processing (5)
crates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_5240_silent_blade_oni.rscrates/engine/tests/integration/kiora_self_library_peek_cast.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/engine/src/game/effects/cast_from_zone.rs
- crates/engine/src/types/ability.rs
- crates/engine/src/parser/oracle_effect/mod.rs
- crates/engine/tests/integration/kiora_self_library_peek_cast.rs
Member
|
Maintainer hold for head |
Member
|
Maintainer hold for head |
matthewevans
approved these changes
Aug 4, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved for merge queue at 92a628cf3b006b28b0a2f082eb2b6ec64155ea73.
The hand-bound typed-leg fix now selects the InZone { Hand } binding explicitly, the runtime Silent-Blade Oni path discriminates both creature type and mana value, and the current-head parse-diff reports the expected 12 cards / 10 signatures. Required CI is green.
4 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 5, 2026
…ated The rebase onto upstream `b654513cb` (phase-rs#6996, phase-rs#6999, phase-rs#6998, phase-rs#7001, phase-rs#6997, phase-rs#6946) resolved the census row's conflict to upstream's three `effects/mod.rs` literals, which are correct for the upstream tree but not for this branch replayed on top of it. Re-derived from the row's own failure output — never predicted by arithmetic — and re-pinned: `:6065/:6142/:9324 ⇒ :6175/:6252/:9456` The shift is NOT uniform (`+110/+110/+132`), and the asymmetry is the measurement: C1's `upfront_optional_gate` authority is one 110-line hunk above all three producers, and `resolve_chain_body` takes a further `+22` from two hunks inside itself and above its own gate (the `optional_for` coupling note, and adoption A replacing the inline conjunct chain with the authority call plus its `debug_assert!`). The file's whole-file delta is also `+132`, so nothing lands below the third producer, and `6065+110`, `6142+110`, `9324+132` equal the observed coordinates exactly. Identity re-established rather than assumed. Each producer at its new coordinate is sha256-identical to `b654513cb:effects/mod.rs` at its old one and to the pre-rebase tip `117baa6a1` at `:6109/:6186/:9183`, and each is still inside the enclosing function this row NAMES — `drive_sequential_repeated_optional_payment`, `resolve_repeated_optional_payment_choice`, `resolve_chain_body` — which is stronger evidence than the coordinate. The diff instrument discriminates: in the new tree the three old coordinates hold a `may_trigger_auto_choice` lookup, a blank line, and a bare `//`. `engine.rs:11549` was re-derived too, not carried over, and is UNMOVED: upstream's six commits net ZERO above it (`:11420` in both the old base `dcb8f3808` and the new base `b654513cb`), so this branch's own `+95`/`+34` still land it on `:11549`, byte-identical and still inside `begin_pending_trigger_target_selection`. `scoped_library_search.rs:452` is unmoved as well. Two entries holding still while three move is the set-preservation evidence: the row's first two asserts fired GREEN on the run that caught this, so the total stays 37 and the partition stays 5/7/25 — no producer was gained or lost. Assisted-by: ClaudeCode:claude-opus-5
matthewevans
pushed a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 5, 2026
phase-rs#7005) * fix(engine): announce the entries a forced-window answer places on the stack CR 732.2a's ring sampler had exactly one site: `pass_priority_once_with_pipeline`, which fires only at an active-player `Priority` settle. Any stack entry that resolves ACROSS a forced pre-priority window — a CR 608.2b `TriggerTargetSelection`, a CR 603.5 `OptionalEffectChoice`, a CR 603.3b `OrderTriggers` — was therefore never present in two consecutive retained frames, so `certified_period_touch`'s announced set ("entries in a frame's stack absent from the previous frame's") could not see it and `bounded_cycle_pin_slots_for_window` could not publish its choice. The shortcut then described a sequence with unpublished per-iteration choices in it. This adds the SECOND sampling site, in `apply_action`, keyed on the forced-window flag captured BEFORE the reducer consumed it. Its conjuncts are the settle sampler's, plus a non-shrinking-stack guard measured against the pre-action depth. Consequences carried in this commit rather than left to be discovered: * the structural pin `arc_as_ptr_beat_identity_is_the_sample_not_one_of_its_halves` moves 2 -> 3 `as *const` reads (the shared per-beat `before` plus an `after` read in each arm that can advance the ring), so a re-basing onto a field address still flips it; * two doc comments claiming `victim_slot` is "empty on every trajectory that offers today" are FALSIFIED by the widening and are replaced, not softened — a `Targets` declaration is announced now, so `worst_seat_life_loss` reaches `elimination_bounds` in production; * B5f rows that consequence two-sided on the user's own MODE1 capture (tracked here as `f4_user_mode1_no_offer_4p.json.gz`, 860,451 B, derived `jq -c '{gameState}' | gzip -9 -n` from the 20.5 MB envelope): with P1 seeded at 7 and 6 the offer FIRES with `max_iterations == 1`; at 5 and 4 the drive reaches the same beat, raises nothing, and the typed verdict is `NoNarrowedLegalCount`. The arms are ONE life point apart, which is what makes the row about the divisor rather than about the board. Landing FIRST of the five commits is load-bearing: relief without the widening turns a silent no-offer into a treadmill that offers and commits nothing. Assisted-by: ClaudeCode:claude-opus-5 * refactor(engine): one authority for a loop-shortcut period boundary `drive_one_shortcut_cycle` delimits a committed repetition two ways: board recurrence, and — for the certification basis that consults no board predicate at all — the published `frames_per_period` count. The frame-count arm existed at exactly one beat kind, the active-player settle, because that was the ring's only sampling site. With the answer-beat sampler in place that premise is gone: a period whose extra frames are recorded while a player answers a forced pre-priority window would never reach `k`, so `frames_per_period` becomes unreachable on precisely the boards the widening was for, and such a drive can only end at its runaway beat cap having committed nothing. The injector arm therefore advances the same counter, under the same `Arc`-identity frame detector the settle arm uses. Both arms now ask ONE function. `published_period_elapsed(frames_this_cycle, frames_per_period)` carries the two properties neither call site can state: `None` NEVER elapses (an offer that published no signature must not have one invented for it, because ending a cycle early commits a fraction of the published delta — the conditional action CR 732.2a forbids), and the comparison is `>=` rather than `==` (one beat may retain more than one frame, and an `==` would drive past its own boundary). Two regression rows, one on each surface: * `published_period_elapsed_is_total_over_the_axes_that_delimit_a_cycle` asserts the whole truth table, including the `k - 1` and `k + 1` arms that discriminate the off-by-one and the `>=`/`==` choice — the anti-vacuity control the structural row cannot supply; * `the_period_delimiter_has_one_authority_and_both_frame_recording_arms_ask_it` censuses `drive_one_shortcut_cycle`'s extent with the tree's own comment-excluding extractor: exactly two delimiter calls, exactly two counter advances, and ZERO inlined raw comparisons, with a proven-live instrument on both sides of the zero census. Also restores `drive_one_shortcut_cycle`'s doc block, which the delimiter extraction had silently re-attached to the new function, and corrects its "the single `record_loop_detect_sample` call site" sentence — there are two sampling sites now. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): discharge a loop's replacement obligations against the live board Conjunct (6) classifies each announced stack entry on its CARRYING FRAME — a retained ring sample, and therefore a board from the past. It then discharged the resulting `FreeUnlessReplacements` obligation against that same frame, which answers the wrong question: a shortcut is a claim about the FUTURE, and every remaining repetition resolves under the board that exists NOW. A replacement definition that entered the battlefield after the sample was taken is invisible to the frame-side check, so the described sequence could contain exactly the CR 616.1 resolution-time choice CR 732.2a forbids. The gate now discharges a second time against `state`, guarded by `!ptr::eq(*frame, state)` — a de-duplication, not an exemption: when the pair is carried by `current` itself the first call already ran on that very board. Two rows, each with its own paired positive: * `n3_a_replacement_installed_after_the_frame_was_captured_refuses_certification` builds a ring whose frames are all cloned BEFORE the definition is installed, so the def exists on the live board and nowhere else, and runs four arms — no def (certifies), live-only OPTIONAL (refused, the arm this change exists for), live-only MANDATORY (certifies, which keys the previous arm to optionality rather than to "a definition exists"), and present-everywhere OPTIONAL (refused, proving the frame-side discharge still does its own job so this is an ADDED refusal, not a relocated one). `announced_from_retained_sample` runs on every arm as the reach-guard that the pair is carried by a frame that is not `current`. * `n3_b_a_live_carried_pair_is_still_discharged_by_the_first_call` exhibits the short-circuited shape and shows the optional definition is still refused there. CR anchors corrected in the same change, because they are about this seam. CR 614.1a is "effects that use the word instead" — a sub-rule cited for its parent's job. The prompt-cause authority in `replacement.rs` classifies EVERY applicable replacement, including skips, enters-with, turned-face-up and virtual candidates that carry no `ReplacementDefinition` at all, so its anchor is the definitional head CR 614.1; and what makes an optional replacement disqualify a shortcut is CR 732.2a's ban on conditional actions, not CR 614.1a. Both `replacement.rs` sites and four r9 sites now read `CR 732.2a + CR 614.1`, in that order. CR 616.1 stays on the two-or-more ORDERING branch, where it belongs. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): one authority for whether a "may" is already answered, and answer the shortcut's gate with it Three places answered "does this ability open ONE up-front optional gate, and to whom, under which key?" — production's own branch in `resolve_chain_body`, the loop-shortcut mint's guard (b), and `analysis::resource::auto_may_answer_for`. The latter two asked the same four predicates and OMITTED two conjuncts the first has: `optional_for` and the CR 608.2d feasibility probe. Measured, that made them return a different answer on two ability shapes, and each was defended only incidentally — the fan-out case by a disjunct in `resolution_prompt.rs` answering a different question, and the infeasible case by membership of a fail-closed list from which three variants have already been promoted out. `effects::upfront_optional_gate` is now the assembler, and production's own branch IS that function rather than a fourth copy of it. `stored_may_answer` is its consumer half. `OptionalFeasibility::{Known, Probe}` exists because a naive adoption would have made production probe TWICE: `resolve_chain_body` has already run `optional_effect_is_infeasible` for the `CastFromZone` decline early-return that precedes the gate, and that arm clones the whole `GameState` per bound object to run a dry-run cast. Adoption A hands its answer over as `Known`; every other caller passes `Probe`, which the authority evaluates LAST so the clone-bearing arm is reached only for `optional AND NOT optional_for AND NOT repeat` entries. It is deliberately not charged against `PROBE_BUDGET`: that counter bounds CR 732.2a certification asks at the verdict door, and mixing wall-clock cost into it would re-base every metered row's pinned spend. Guard (b) adopting the authority is a BEHAVIOUR CHANGE and it is rules-correct in the fail-closed direction. It now withholds the `MayChoice` slot for an `optional_for` ability — CR 608.2d + CR 101.4 make that an APNAP cascade of up to one window per living player, and one published slot standing for N prompts is the cardinality defect group (c) already argues against — and for an infeasible optional, which opens no window at all, so a slot for it is a pin the gate can never spend. Direction: strictly FEWER offers, never more. N0 rides on the same authority: gate (6) now takes relief from a stored auto-choice as well as from a published pin, and the two bases are disjoint by construction because guard (b) publishes a slot only for a may with NO stored answer. Reading an auto-answered may's slotless mint as "unspecified" was the defect. Only `Accept` is relieved — a stored `Decline` is equally prompt-free but produces the OPPOSITE board, so its optional-cleared residual would describe events the shortcut never proposes. Rows, each with its own paired positive: * `a5_a_stored_accept_relieves_gate_six_and_a_stored_decline_does_not` — one board, one key, one value different; the arm the user's MODE1 capture rides on. * `f2a_the_upfront_gate_authority_answers_the_two_shapes_the_third_copy_omitted` — every arm seeds a stored `Accept` under exactly the key the old copy built, so an omitted conjunct is a WRONG answer rather than an absent one. Includes the feasibility control (the same `RemoveCounter` ability on a board ONE counter different) and the pair that proves `Known` overrides the probe instead of re-running it. * `f2b_guard_b_withholds_a_pin_the_cr_603_5_gate_can_never_spend` — deliberately UNSEEDED, so guard (b)'s store conjunct is vacuously true on every arm and the only thing that can move `may` is the axis under test. A seeded variant is rejected: it cannot fail for the reason the row exists. * `f2c_the_cr_603_5_conjunct_set_has_one_production_assembler` — MEASURED per-predicate production call-site counts 2/2/2/1/2, plus the stronger statement that ZERO production consumers live outside `game/effects/`, with a proven-live instrument on the zero census. The three surviving non-authority sites select a DRIVER rather than opening an up-front window, so folding them in would be wrong, not cleaner — and the guarantee is stated honestly: an inline re-derivation from `ability.repeat_for` is NOT caught, and no census over these five tokens can catch it. `cargo clippy -p phase-engine --all-targets -- -D warnings` is clean under the shipped enum, with zero `is never constructed` (both variants are constructed on production paths, and F2a exercises `Probe` to opposite outcomes). Assisted-by: ClaudeCode:claude-opus-5 * test(engine): re-derive every row whose premise was the sampler's blind spot, and pin both user captures The answer-beat sampling site (C4) widened what a CR 732.2a offer can publish, and several rows had encoded the OLD blind spot as if it were a property of the board. Each is re-derived from measurement rather than relaxed, and both of the user's own captures are tracked and driven end to end. THE FIX BAR. `a1_the_users_accept_committed_nothing_board_now_commits_on_every_axis` drives the user's MODE2 capture — the board where the offer fired, the declaration was accepted, and the drive then committed nothing and re-offered. It now publishes all three per-iteration choices and the accepted `Fixed(n)` grant commits EXACTLY n repetitions of the offer's own published per-cycle signature on life and library, with counters and tokens non-zero at n=1 and exactly 3x at n=3. Revert-probe run: with the answer-beat site ablated every axis collapses to 0, reproducing the captured symptom. `m1_...` is its one-field-apart sibling on MODE1 (a STORED CR 603.5 answer, so guard (b) withholds Sue's slot and the auto-answer relief discharges gate (6) instead). `--lib` rows re-keyed to production's own walk. `newest_item4_window` consumes `game::engine::candidate_windows`, so R21(b-placement-B)'s window IS production's rather than a hard-coded `len - 2` that silently asserted `span == 1`; its reach-guard now states what it needs (no denied answer; a gate that ASKED must have completed) with the load-bearing exemption equality byte-identical. R16(ii-b) searches its real construction requirement (`meter.spent > 0`, never `denied`, which would assert itself) and ships its own revert-probe as a `RaisedTwiceLinks` positive control. The CR 603.5 prompt census is re-pinned from the failure's own left side, every producer sha256-identical at its new coordinate and still inside the function the row names, and its authority count is made comment-insensitive so prose cannot trip a call-site pin. Attribution repairs: `r2` is renamed `r2a` to state what its body now asserts; r1 keeps its over-charge follow-up pointer instead of claiming discharge; the `r5_declare_is_accepted` citation is replaced with the per-caller measurement that actually exists; the r28 empty-schema arm DISCLOSES that its path is now reached by staging rather than naturally; a pre-existing `CR 614.1a` comment that described no rule is corrected to CR 614.1. `ai1_the_bounded_declare_candidate_withdraws_when_the_offer_publishes_a_pin` pins the generator's `Fixed` candidate to the published pin set in both directions on one board, and is deliberately not `#[ignore]`d. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): replace the four notes C4 falsified, and recover the counter assertion a false premise cost Five ACCEPT-WITH-FIXES findings, plus one sibling swept by the same defect mechanism. `crates/engine/src/` is COMMENT-ONLY this round — proved by `git diff -U0 crates/engine/src/` having no non-comment +/- line — so the only executable change is in the F4 test file. F1. Four docs still asserted the pre-C4 premise that `record_loop_detect_sample` has ONE call site, and this branch's own policy is to REPLACE a falsified note rather than soften it. The measured truth is TWO production sites, both after `run_post_action_pipeline` (CR 603.3): the settle sampler in `pass_priority_once_with_pipeline` and the forced-window answer site in `apply_action`. Rewritten at the fn doc and the `loop_detect_ring` field doc (`game_state.rs`), at `frames_per_period` (`resource.rs` — the justification C2 had already repaired in code), and at `ring_delta_signature`, whose homogeneity argument now rests on the `Priority{active_player}` conjunct the two sites SHARE rather than on there being one site. F1e (swept sibling). `drive_one_shortcut_cycle`'s "the frame counter is advanced here and nowhere else" was falsified on this same branch by the forced-window ANSWER arm's own advance. Both arms key the advance on the ring's back allocation changing, which is what keeps drive and mint one-to-one. F2. The second site's comment claimed "the same conjuncts as the settle sampler, PLUS the window flag". Measured, the sets are the same size one member apart: `answering_forced_window` REPLACES `resolved_this_beat`, and the settle site's `else { ring.clear() }` has no counterpart here. The comment now says that, names the consequence (an answer that resolves nothing but leaves the stack non-shrinking records a duplicate frame), and says why it is acceptable — `ring_delta_signature` refuses a zero smallest-period delta, and mint/drive are symmetric because `inject_pinned_answer`'s arms all dispatch `apply_action`. It also documents the latent ordering asymmetry: this site records BEFORE `state.waiting_for = wf` while the settle sampler records after `sync_waiting_for`, and `GameState::eq` compares both `waiting_for` and `priority_player` while `normalize_for_loop` neutralizes neither. Latent, not live: a `debug_assert_eq!` census reported 0 failures across 18,486 lib and 4,487 integration rows, with a `debug_assert!(false)` positive control that fired on the A1 board. Deliberately NOT reordered — that would be a behavioural change for a non-live defect. F3. The fix bar's life and library equalities had no anti-vacuity guard, so an all-zero certificate would satisfy `moved == rate * n` on a board that never moved. Added, in the existing `assert_axis_scales` idiom. F4. The counter assertion had been weakened on a false premise. Measured, the published vector is `counters {(Plus1Plus1, Creature): 2}` — non-zero and state-readable; only `tokens_created: 0` is event-fed. The real obstacle was the accessor: `commit_axes` reads ONE object's counters against an AGGREGATE key. Re-cut against `ResourceVector::snapshot`/`delta`, the accessor the certificate is minted from, plus a "nothing unpublished may move" arm. The aggregate moves 2 at n=1 and 6 at n=3, i.e. exactly 2n. The token axis keeps the scaling arm alone, now for its real measured reason. F5. `has_frozen_window`'s residual was declared as "four authored-ring rows". Measured: two call sites, and NEITHER is an authored ring — both drive the real tracked dumps. Overstated in count, understated in kind. The disclosure now names both rows and the loud floor that lets them keep a hard-coded `span == 1`. The CR 603.5 prompt census went red on F2's line drift and was re-pinned by its own protocol, not by matching the tree: `engine.rs:11515 => :11549`, +34 which is engine.rs's entire (comment-only) delta above the producer, the line sha256-identical at the new coordinate and still inside `begin_pending_trigger_target_selection`; total 37 and partition 5/7/25 unchanged. Every new assertion and guard is proven to flip. Ablating the answer-beat sampling site fails the library equality at `left: 0 / right: -1` (the prior probe's signature) and, with the seat loop bypassed so control reaches it, the recovered counter equality at `left: 0 / right: 2`. Zeroing each published rate in turn fires each guard alone. All five are typed assertion failures, not harness crashes. lib 18487 passed / 0 failed, integration 4487 passed / 0 failed / 2 ignored, clippy --workspace --all-targets -D warnings exit 0. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): re-derive the CR 603.5 producer pins the rebase invalidated The rebase onto upstream `b654513cb` (phase-rs#6996, phase-rs#6999, phase-rs#6998, phase-rs#7001, phase-rs#6997, phase-rs#6946) resolved the census row's conflict to upstream's three `effects/mod.rs` literals, which are correct for the upstream tree but not for this branch replayed on top of it. Re-derived from the row's own failure output — never predicted by arithmetic — and re-pinned: `:6065/:6142/:9324 ⇒ :6175/:6252/:9456` The shift is NOT uniform (`+110/+110/+132`), and the asymmetry is the measurement: C1's `upfront_optional_gate` authority is one 110-line hunk above all three producers, and `resolve_chain_body` takes a further `+22` from two hunks inside itself and above its own gate (the `optional_for` coupling note, and adoption A replacing the inline conjunct chain with the authority call plus its `debug_assert!`). The file's whole-file delta is also `+132`, so nothing lands below the third producer, and `6065+110`, `6142+110`, `9324+132` equal the observed coordinates exactly. Identity re-established rather than assumed. Each producer at its new coordinate is sha256-identical to `b654513cb:effects/mod.rs` at its old one and to the pre-rebase tip `117baa6a1` at `:6109/:6186/:9183`, and each is still inside the enclosing function this row NAMES — `drive_sequential_repeated_optional_payment`, `resolve_repeated_optional_payment_choice`, `resolve_chain_body` — which is stronger evidence than the coordinate. The diff instrument discriminates: in the new tree the three old coordinates hold a `may_trigger_auto_choice` lookup, a blank line, and a bare `//`. `engine.rs:11549` was re-derived too, not carried over, and is UNMOVED: upstream's six commits net ZERO above it (`:11420` in both the old base `dcb8f3808` and the new base `b654513cb`), so this branch's own `+95`/`+34` still land it on `:11549`, byte-identical and still inside `begin_pending_trigger_target_selection`. `scoped_library_search.rs:452` is unmoved as well. Two entries holding still while three move is the set-preservation evidence: the row's first two asserts fired GREEN on the run that caught this, so the total stays 37 and the partition stays 5/7/25 — no producer was gained or lost. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): replace every doc claim this branch's own rows falsified, and re-measure the span==1 residual The final review at 7841e1e found three survivors of the F1 falsified-doc class plus one unproven mechanism. A mechanical sweep of the same class found two more the review did not name, both in the test file the previous round never searched. Comment-only; no behaviour. r1's doc said the offer "publishes ONE point and commits ZERO cycles (see r1b and r2)". All three clauses are dead: r1b's own assert_eq! pins THREE points [Sue MayChoice, Reed MayChoice, Torch Targets]; r2a commits exactly n at n=1 and n=3; and `r2` names a row this branch renamed (fn-name diff b654513..HEAD: exactly one name disappeared, r2_an_accepted_declaration_commits_zero_cycles_because_reeds_may_is_unannounced, and zero references to it survive). r1's second paragraph was falsified too and went unreported: the in-tree form is the ADDITIVE one (resource.rs observed_life_loss.max(0) + declared_life_magnitude), not the MAX form, and victim_slot is NON-EMPTY on this board, so the two forms do not coincide. r1b's OWN doc block was the sharpest instance and neither round had caught it — it said "403 and 401 are never announced ... publishes exactly ONE point" while the same function's body asserts three and its message says all four sources are announced. The U6 header's "F4 publishes ONE point, not three" and the Fixed-gate bullet's "F4 publishes one point" are corrected with the reason preserved: the AI still declines, but on the emptiness gate, never on the count. resource.rs:10713 is brought into line with the two siblings this branch already replaced at resource.rs:1062-70 and engine.rs:2257-64, reusing their wording. has_frozen_window's span==1 residual was justified by an unproven mechanism ("both fail LOUD on a half period"). A half period is non-degenerate, so those guards do not fire, and both rows' assertions are span-independent — they would PASS. Re-measured here rather than transcribed: at the beat drive_dump_until(gz, 80, has_frozen_window) selects, dina beat=6 ring=2 and dellian beat=5 ring=2, and candidate_windows yields exactly one candidate (idx=0, span=1, len=2) on each, so &live[len-2..] is the whole ring and span==1 is exact. The residual is restated as "exact today, silent if the sampling rate grows this ring past two". inject_pinned_answer's "arms all dispatch apply_action" (two sites) is corrected for precision: four arms, three dispatch, the fourth Err()s before any frame advance. The mint/drive symmetry conclusion survives and is now stated in the stronger form the code supports. The engine.rs edit is deliberately line-neutral (3 for 3) so the CR 603.5 census pin at engine.rs:11549 does not move; verified still on the OptionalEffectChoice producer and the census row green. --lib: 18501 passed; 0 failed; 6 ignored. --test integration: 4513 passed; 0 failed; 2 ignored. clippy --workspace --all-targets -D warnings: exit 0. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): correct the CR 608.2d announcer citation, derive r2a's rates from the certificate, and make an unresolvable point source fatal CodeRabbit left four inline findings on phase-rs#7005. Three hold; the fourth's premise does not, and the difference is a measurement rather than an argument. F-A - `UpfrontOptionalGate::prompt_player` cited CR 117.3a ("The active player receives priority at the beginning of most steps and phases..."), which is priority timing. The field names the player who ANNOUNCES the choice, which is CR 608.2d ("...the player announces these while applying the effect."). Both quotes verified against the rules text before the edit. `optional_prompt_player`'s own doc carried the identical wrong citation and is fixed with it. Both edits are line-for-line so the CR 603.5 prompt census keeps its exact pins. F-B - CodeRabbit asked for memoization "if the fixtures produce optional, non-repeat, non-`optional_for` `CastFromZone` entries". They do not. Instrumenting the clone-bearing arm and both `state.clone()` sites, with a thread-local marking the `Probe` path, over a full `--test integration` run (reproduced bit-for-bit across two runs): 16402 raw mint calls => 4573 `Probe`-mode feasibility calls => 0 arm entries and 0 clones on that path. The 59 arm entries and 50 clones that do occur are production's own `Known` path, which pays them once per resolution. The zero's positive control is in-band: `P` and `K` are two labels from the same statement, and `K` returned 59. The memo itself already exists for the other caller - `PeriodVerdicts` is a `(FrameIx, ObjectId)`-keyed compute-on-miss memo whose `published` field IS `entry_publishes_pin_slots`. Recorded the measured number at the seam; added no memoization for a cost of zero. F-C - `(libs_before[0] - libs_after[0]) as i64` subtracted two `usize` before the cast, so the zero-commit regression the row exists to catch aborted on an arithmetic overflow instead of printing the row's own diagnostic. Demonstrated both ways: the old form under the underflow condition panics `attempt to subtract with overflow` with the diagnostic suppressed; the new form fails as `assertion left == right` and prints it. The row also asserted `(i64::from(n), i64::from(n))` - two literals - while its message claimed the published per-cycle delta. Both rates now come from `certificate.per_cycle.delta`, negated because the axes are measured as losses, with an anti-vacuity guard so the equality cannot degenerate to `0 == 0 * n`, and seat ids read positionally so a rate belongs to the seat whose movement is measured. F-D - `published_point_names` synthesised `obj<id>` when a point's source was absent from `state.objects`. Every caller compares that string to the SUE/REED/TORCH constants, so an unresolvable source read as "not that card" and silently satisfied m1's negative owner-firewall assertion. Now a panic, matching the treatment the adjacent `other =>` arm already gave the same class of failure. The new guard is proven able to fire: `published_point_names_panics_when_a_points_source_is_absent` deletes the first published point's source and requires the panic, and reports `should panic ... FAILED` when the synthetic fallback is restored. Gates at this tip: lib 18501 passed / 0 failed / 6 ignored; integration 4514 passed / 0 failed / 2 ignored (4513 + the new row); clippy --workspace --all-targets -D warnings exit 0. The CR 603.5 prompt census and `a1_the_users_accept_committed_nothing_board_now_commits_on_every_axis` are both green. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): synchronize the forced-window state before recording the loop sample `apply_action`'s answer-beat sampler called `record_loop_detect_sample` BEFORE installing the pipeline's returned `wf`, while the settle sampler in `pass_priority_once_with_pipeline` records AFTER its `sync_waiting_for`. A frame minted at the answer site therefore snapshotted the PRE-pipeline `waiting_for`/`priority_player` pair and a settle frame the synced one. That is a detection hazard, not cosmetics: `impl PartialEq for GameState` compares both fields and `normalize_for_loop` neutralizes neither, so a heterogeneous ring breaks `ring_delta_signature`'s turn-position conjunct. Route `wf` through `game::public_state::sync_waiting_for` — the canonical synchronizer, which also recomputes `priority_player` — before the record, and drop the raw assignment below it. Blast radius is the ring only: `apply_action_boundary` already re-syncs the returned `wf` before the result leaves the engine, so the settled state is unchanged. The edit is line-neutral so the CR 603.5 prompt census keeps its line-exact `engine.rs:11549` pin (verified byte-identical by sha256, still inside `begin_pending_trigger_target_selection`). Mechanism verified at source: `is_forced_cascade_window` is a `matches!` over 13 non-`Priority` variants with a fail-closed fall-through, and the sampler's gate reads the returned `wf` while the snapshot preserved `state.waiting_for`. Evidence — instrumented probe on the pre-fix tree, `--test-threads=1`, full lib + integration; one unit = one emitted probe line = one `record_loop_detect_sample` invocation at that site. Settle site: 996 samples, 0 that the sync changed on either field. Answer site: 285 samples, 0 stale on either field. An always-true comparison of the same shape emitted on the same line is true on 996/996 and 285/285, so the zeros are a verdict rather than a dead instrument. The defect is consequently structural and latent, and this replaces a coincidence with a guarantee. New production-fixture row on the tracked `dina_conqueror_4p` dump, driven through production `apply()`: the newest answer-beat frame is `Priority{active_player}`, its `priority_player` is that seat, and the published `LoopCertificate` is exact under an exhaustive destructure. Revert-probes, measured: clobbering `priority_player` after the sync fails arm 2 (`PlayerId(3)` vs `PlayerId(0)`); clobbering the window fails arm 1 (`GameOver` vs `Priority`), with arm 2 passing first, so the arms are separately live. A pure revert of the reorder PASSES, which is the honest statement that no current fixture reaches the divergence. Gate at this tree: fmt 0, clippy --workspace --all-targets -D warnings 0, lib 18501 passed / 0 failed / 6 ignored, integration 4515 passed / 0 failed / 2 ignored (4514 -> 4515 is exactly the one new row). Assisted-by: ClaudeCode:claude-opus-5
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
2 participants
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.
Summary
Closes #6960.
parse_cast_type_disjunctionhandled exactly one shape —" or "between two bare core types, with an optional article and a required trailingspell(s)/card(s). Everything else in the "cast from among them" class kept a bareTargetFilter::ExiledBySourcewith no card-type leg, so any card type could be cast from the exiled set.Rewritten as a per-axis composed grammar:
reusing
oracle_nom::target::parse_type_filter_wordfor the whole leg alphabet (core types and subtypes, soVehicleneeds no new code) and mirroringoracle_nom/enchant.rsfor the separator/list pair.and,or,and/orand serial commas all lower toTypeFilter::AnyOf: per CR 205.2b conjunction is a property of adjacent type words ("artifact creature"), while a connector enumerates alternatives. A literalAndwould be a total no-op — no card in the corpus carries both Instant and Sorcery — which is the trap the issue warned about.Acceptance requires either ≥2 legs or a consumed quantifier. That is also the anti-swallow guard:
"cast a spell from among them"yields zero legs (the head-noun guard empties the list), so Aetherworks Marvel, Svella and Apex of Power stay correctly bare.The parser half alone was inert. The chain seam (
effects/mod.rs:10788) forwards every exiled card as the sub-ability's targets, sotarget_idsarrived non-empty and skipped the only site where the cast filter was applied — every exiled card got the permission regardless of type.cast_from_zonenow retains only forwarded ids matching the clause's own legs, via a newTargetFilter::without_exile_anaphor()that discharges theExiledBySourceleg the seam already satisfied while preservingAnd/Orstructure. Re-evaluating the anaphor there would be actively wrong: on a triggered ability it reads alinked_exile_snapshotcaptured before this ability's own exile step, so every id would be dropped and the grant would become a total no-op.Files changed
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/game/effects/cast_from_zone.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/kiora_self_library_peek_cast.rsdocs/parser-misparse-backlog.mdTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 107.3i,CR 109.2b,CR 112.1,CR 118.9,CR 205.2b,CR 205.3g,CR 406.1,CR 601.2,CR 601.2c,CR 601.3,CR 603.4,CR 607.2a,CR 608.2c,CR 608.2d,CR 608.2gEvery number was grep-verified against
docs/MagicCompRules.txtbefore it was written. CR 601.3 ("a player can begin to cast a spell only if a rule or effect allows it") is the authorizing rule for treating the card-type restriction as part of the cast-legality predicate; CR 205.2b authorizes theAnyOflowering; CR 607.2a authorizes discharging the linked exile anaphor rather than re-evaluating it.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.
cargo fmt --all --check— cleancargo clippy --all-targets -- -D warnings— clean, 0 warningscargo test -p phase-engine— 18474 lib + 4494 integration + 12 + 9 passed, 0 failed./scripts/check-parser-combinators.sh— Gate G PASS, Gate A PASS, no newallow-noncombinator./scripts/check-test-card-data-load.sh— PASS./scripts/check-engine-authorities.sh— Gate B PASS, Gate D PASS, draw-replacement baseline frozen./scripts/gen-card-data.sh— regenerated; all seven cards carry the gate, controls unchangedNot run locally, left to CI: the pre-push hook's frontend stages (
pnpm lint,pnpm run type-check) andcoverage-regression-check.shagainst the remote preview endpoint. This change touches no frontend file. The push used--no-verify, matching the repository's ownship-commitsconvention; every Rust stage of that hook was run directly and is listed above.Gate A
Gate A PASS head=37297cc658bec080a19b64b5631bfbbd9a0d4cba base=02760f58841457847f535a6f54aa480ae8e22af5
Anchored on
crates/engine/src/parser/oracle_nom/enchant.rs:115/:131— the existing separator + separated-list pair this grammar mirrors. Same file family, same combinator shape (space-leading separator arms closed by a space-leading tail), and the reason the leg had to be aseparated_list1rather than amany1(terminated(word, tag(" "))):enchant.rs's own leg does not eat its trailing space.crates/engine/src/parser/oracle_nom/target.rs:262—parse_type_filter_word, the single alphabet authority for type words. The new grammar delegates its entire leg alphabet here rather than re-enumerating core types inline as the old helper did, which is what makes the subtype leg (Vehicle,Hero) work with zero new code. The head-noun guard is now expressed asverify(parse_type_filter_word, |tf| matches!(tf, TypeFilter::Card))so there is one alphabet and one boundary guard, not two.crates/engine/src/parser/oracle_effect/mod.rs—parse_cast_type_gate, the existing composition site whose doc comment already establishes that the type half must ride on the casttargetfilter becauseCastPermissionConstrainthas no card-type arm. This change makes that composition reachable for the shapes it could not previously parse.crates/engine/src/game/effects/cast_from_zone.rs— the pre-existing no-target fallback that populatestarget_idsfrom live exile links and applies the whole filter to them. The new retain is its forwarded-ids counterpart, and the comment notes the two are idempotent with respect to each other.Final review-impl
Final review-impl PASS head=37297cc658bec080a19b64b5631bfbbd9a0d4cba
Stated precisely: the final read-only review returned no code findings at this head. Its single LOW finding is the parse-diff disclosure now recorded under "Claimed parse impact" below. Earlier rounds each found a real defect that was fixed and re-verified — see Validation Failures.
Claimed parse impact
Seven cards gain the card-type gate they were missing:
And[Typed{AnyOf[Instant,Sorcery]}, ExiledBySource]And[Typed{[Sorcery]}, ExiledBySource]And[Or[Typed{Subtype Vehicle}, Typed{[Artifact,Creature]}], ExiledBySource]And[Typed{AnyOf[…]}+InZone{Exile}, ExiledBySource]And[Or[Hero+InZone Exile, [Card,Non(Creature)]+InZone Exile], ExiledBySource]Scarlet Witch is not in the issue's list — it was found by census and arrives from widening the match arm to accept
TargetFilter::Or.Shape-only, semantically equivalent — no behaviour change. The composed grammar is now tried before
parse_type_phraseonand/serial-comma lists, and it emitsTyped{AnyOf[…]}whereparse_type_phrase'sTYPE_SEPARATORSloop emittedOr{Typed,…}. These rows will appear in the CI parse-diff and are called out here so the list is not mistaken for unintended blast radius:Scholar of the Lost Trove, Bilbo Thief in the Night, Counterpoint, Victor Timely, Bösium Strip, The Great Work.
The equivalence is total on every consuming seam —
cast_filter_has_typed_leaf,ensure_exile_zone_on_cast_targetandadd_cast_target_propsall recurse intoOr, andcast_target_is_hand_originflips no driver because every affected card is graveyard- or zone-less rather than hand-origin.Reached but deliberately NOT claimed: Spawnsire of Ulamog moves
Any→Typed{Subtype Eldrazi}, a real type-axis improvement, but"from among cards you own outside the game"binds no zone before or after, so it is recorded as an improvement rather than a fixed card.Controls, verified unchanged: Jeleva, Nephalia's Scourge (the card #6959 fixed) is byte-identical; Aetherworks Marvel, Svella and Apex of Power are still correctly bare. All 50 bare-anaphor rows in the export residualize to
Noneand keep the full forwarded set.Scope Expansion
The change extends past the parser into one engine seam, deliberately and by necessity.
The issue scoped this as parser work. It is not sufficient: with only the parser half, the composed type leg is provably inert at runtime — the chain seam forwards every exiled card as a target, and
target_idsbeing non-empty skips the one place the cast filter is applied. A parser change nothing observes is not a fix.cast_from_zone.rs(+53) and one newTargetFiltermethod (without_exile_anaphor, no new variant and no new field on a serialized type) close that.The retain is gated on
references_exiled_by_source(), so explicitly-targeted grants (Emry, Bring to Light, Urza) are untouched, and the 50 bare-anaphor rows residualize toNoneand are a strict no-change path. It sits above every early-returning router so the gate is universal rather than partial.Validation Failures
None outstanding.
/engine-implementerran plan →/review-engine-plan→ implement →/review-impl→ commit, each round in a fresh agent context against the artifact alone. Three rounds each found a defect invisible to the round before, and all three are worth stating:EffectZoneChoiceis the private-library-peek state; this class parks atPrioritywith a live permission. It also found one fixture vacuous:as_instantonly stripsCreature, so a "trap instant" was[Sorcery, Instant]and satisfied the gate honestly.ExiledBySourceanaphor against a snapshot captured before the ability's own exile step, which would have dropped every id and converted the over-permissive bug into a total no-op, regressing even bare-anaphor cards with no type gate. Every fixture missed it by leavingtrigger_source: None; a test stamping a real trigger context now pins it.One suggested fix was investigated and not applied: a mill-class exploit (Summons of Saruman, Jace's Mindseeker) turned out unreachable —
Effect::Millis in no chain-forwarding arm, so that clause is inert upstream of the gate. The requested test was written, found to pass vacuously with its positive reach-guard failing, and removed rather than shipped. The Mill-forwarding gap is a separate pre-existing defect.CI Failures
None known — CI has not run at the time of writing.
Follow-ups (not in this PR)
Effect::Millwrites no chain-forwarded ids, so the whole mill→cast clause (Summons of Saruman, Jace's Mindseeker) never reaches the cast gate at all. Pre-existing and independent of parse_cast_type_disjunction misses conjunctive, counted, and subtype forms — 5 cards keep a bare cast filter #6960.Spawnsire of Ulamog's zone axis."from among cards you own outside the game"binds no zone; the type gate now applies but the candidate set is still unbound.CR 610.3citations elsewhere inoracle_effect(theparse_from_among_exiled_this_wayfn-doc andoracle_effect/tests.rs:42106) attribute a word-order claim to a rule about "until"-triggered zone-change reversal. Left alone as out of scope;CR 601.3carries the claim.without_exile_anaphorhas noNotarm, andreferences_exiled_by_sourcehas none either. The invariant that the two stay in lockstep is documented and pinned by unit-test rows rather than enforced structurally, because adding aNotarm toreferences_exiled_by_sourcewould makeNot{ExiledBySource}a context ref and delete a real target slot. No production filter reaches it today.Summary by CodeRabbit
Bug Fixes
Tests