fix(parser): UNSUPPORTED cluster: Secret Lair — The Fourteenth Doctor: enter-as-a-COPY-from-GRAVEYAR#5679
Conversation
…: enter-as-a-COPY-from-GRAVEYAR
…0-unsupported-cluster-secret-lair
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for parsing and executing copy replacements and zone-change partitions involving possessive owner scopes (e.g., "your graveyard") and zone-change predicates (e.g., "put there from your library this turn"), enabling cards like The Fourteenth Doctor. Feedback on the changes highlights a potential parsing issue in oracle_replacement.rs where the zone-change suffix is only consumed if the filter is TargetFilter::Typed. If the filter is of another type, the unconsumed suffix could cause downstream parsing failures; it is recommended to parse and consume the suffix independently of the filter type check.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
Parse changes introduced by this PR · 22 card(s), 36 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: changes requested
The partition fix is architecturally right and repairs a real, wide misparse class. Three blockers, one of them a genuine correctness regression on a card the PR doesn't mention.
What's good (verified, not assumed)
- The seam is correct. No new engine enum variant; you dispatch onto existing runtime handlers plus one parser-IR field (
rest_library_position). That's the right layer. - The CR reasoning is right and the citations govern. CR 401.4 — "If an effect puts two or more cards in a specific position in a library at the same time, the owner of those cards may arrange them in any order" — is exactly on point, and routing randomness to whichever pile returns to the library is the correct reading. Your
"in a random order"(The Fourteenth Doctor →random_order: true) vs"in any order"(Garruk / Goblin Ringleader →false, owner arranges) distinction is right. CR 608.2c and CR 701.17a also verify. - The class repair is real. On
main, Goblin Ringleader parsed toPutAtLibraryPosition(count=1, position=Bottom, target=Goblin)— it never put anything into hand, and it put a Goblin on the bottom rather than the rest. Plainly broken. Same for Sylvan Messenger, Merfolk Wayfinder, Enshrined Memories, Alrund, Brass Herald, and the rest of the reveal-partition class. Activated Sleeper was ignoring its"put there from the battlefield this turn"restriction entirely; your predicate now enforces it. These are 22 correct repairs. - The integration test drives the real pipeline —
GameScenario+add_real_card(..., db), not a hand-built ability — and the parser tests pin both partition shapes including the complement's zone, position, and randomness. That is discriminating.
Blockers
1. The declared population is off by 23×.
The body says "affecting 1 card(s)". The head-current parse-diff measures 23 cards / 38 signatures. Twenty-two of those are correct class repairs — that's "build for the class, not the card" working as intended, and it is not scope contamination. But the body has to say so. State the real population and name the class, so the next reviewer doesn't have to reconstruct it from the sticky.
2. Muse Vortex — the guard is broader than the class it documents, and mis-binds.
let is_this_way_partition = nom_primitives::scan_contains(lower, "the rest")
&& nom_primitives::scan_contains(lower, "this way");Muse Vortex reads:
"Then put the exiled instant and sorcery cards that weren't cast this way into your hand and the rest on the bottom of your library in a random order."
Both substrings hit, so it's pulled into the partition path — but it is an exile-origin partition whose primary subject is itself a negation ("...that weren't cast this way"), not the "put all <filter> revealed this way into <zone>" shape your own comment names. The head-current sticky shows the result:
ability/ChangeZone · added: ChangeZone (target=any target, to=hand) [Muse Vortex]
any target is a targeting filter, on a card whose text contains no targeting at all. main was also wrong here (PutAtLibraryPosition(count=Fixed { value: 0 }, ...) — a no-op), so this isn't correct→broken; it's broken→differently-broken, and now it binds a target where none exists.
Either narrow the guard to the reveal/tracked-set origin it actually means, or handle the exile-origin partition properly — and pin Muse Vortex with a test either way. Right now nothing in the suite would notice this.
3. Unexplained static_structure removal.
The sticky's collapsed section carries 1 card(s) · ability/static_structure · removed: static_structure with no example named. Please identify the card and justify the removal, or restore it.
Non-blocking
The parse_effect_chain unit tests exercise the standalone entry point rather than the card pipeline (parse_effect_chain_with_context). The Fourteenth Doctor is covered end-to-end by the integration test, so this is fine for the headline card — but for the class cards, the parse-diff sticky is currently the only pipeline-level evidence.
Nice work on the lowering — the random-order routing in particular is a subtle call and you got it right.
…0-unsupported-cluster-secret-lair
…se Vortex) Addresses @matthewevans's review on phase-rs#5679. The `is_this_way_partition` guard in `parse_put_imperative` was scoped to the bare "this way" anaphor, which also matched Muse Vortex's EXILE-origin partition ("put the exiled instant and sorcery cards that weren't cast this way into your hand and the rest on the bottom of your library in a random order"). That is not the "put all <filter> revealed this way into <zone> and the rest into <zone2>" reveal shape the guard's own comment names, so Muse Vortex was hijacked onto the tracked-set path and bound a phantom `ChangeZone { target: Any }` ("any target") into the hand — on a card whose text contains no targeting at all. Narrow the guard to the full "revealed this way" reveal-origin anaphor. This: - returns Muse Vortex to its byte-identical-with-main positional handling (`PutAtLibraryPosition { target: ExiledBySource }`, no phantom target); - preserves every correct reveal-partition class repair — all of them ("revealed this way" cards: Goblin Ringleader, Sylvan Messenger, Merfolk Wayfinder, Enshrined Memories, Alrund, Brass Herald, The Fourteenth Doctor, etc.) still fire the guard. Pin Muse Vortex with a parser test asserting no hand-bound zone change carries the phantom `Any` target (nothing in the suite caught this before). CR 608.2c: partition of a tracked (revealed) set. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
…t-lair Port the generated integration fixture by preserving current main and the Fourteenth Doctor record. Co-authored-by: Nicholas Tindle <nicholas.tindle@agpt.co>
Co-authored-by: Nicholas Tindle <nicholas.tindle@agpt.co>
…t-lair Preserve the Doctor integration fixtures on the current main baseline. Co-authored-by: Nicholas Tindle <nicholas.tindle@agpt.co>
Co-authored-by: Nicholas Tindle <nicholas.tindle@agpt.co>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current 22-card parse diff and full CI are green; the reveal-partition, Muse Vortex, fixture, and suffix-consumption findings are resolved on this head.
… parity rows it exposes (CR 603.3b + CR 603.4) (phase-rs#5732) `integration_cards.json` is a cached subset of the card-data export, but it was last FULLY regenerated at b9685cc (phase-rs#5695). Nineteen parser/types PRs merged since then; every fixture touch in between was surgical (phase-rs#5672 +1, phase-rs#5679 +2, phase-rs#5727 2 entries), so the parse values silently drifted. Regenerated from the export at 8c35dc5 (oracle-gen, MTGJSON 5.3.0+20260629). Population (json deep-equality, not line counts — the file is one line): committed 2658 entries -> 2726. 70 added, 2 removed, 44 changed values. Attribution of the 44 changed (causal: exports built at phase-rs#5720 / phase-rs#5717 / phase-rs#5730 and compared, NOT shape-guessing): phase-rs#5723 (P02-U3b shared condition grammar) ...... 3 archive trap, temple of civilization, thaumaton torpedo (all gained the comparator/lhs/rhs/qty/scope condition shape) phase-rs#5721 + phase-rs#5719 (where-X quantity channel + ..... 21 restriction grammar; both merged BEFORE phase-rs#5717 — merge order != PR-number order) bellowsbreath ogre, cryptex, deadly rollick, deflecting swat, desert, dread wanderer, esquire of the king, flesh, fraying sanity, gloomlake verge, great desert hellion, gutterbones, officious interrogation, once upon a time, potioner's trove, ribald shanty, rock jockey, second little pig, shifting woodland, snuff out, starport security phase-rs#5695..phase-rs#5720 no-regen window (bloc) ........... 20 Stale already at phase-rs#5720, so attributable to the 19-PR window above the b9685cc anchor, not to any single PR: alrund god of the cosmos, animal friend, approach of the second sun, cavernous maw, fblthp the lost, from father to son, hour of revelation, increasing vengeance, jodah the unifier, mana reflection, misty salon, puca's eye, ram through, reidane god of the worthy, secrets of the key, sevinne's reclamation, temple of the dead, the dining car, unleash the flux, valgavoth terror eater The 70 added keys are new test-source card references the generator collects; phase-rs#5729 (tests-only) contributed zero parse delta, as expected. Corrected premise: 44 entries are truly stale, not 7. Six of the seven originally reported reproduce; `osteomancer adept` is NOT stale (committed == fresh). The regen turns `ordering_parity_sweep` red, so the gate's evidence rows ship ATOMICALLY with it. Both rows are population entries, not ordering regressions: the sweep skips Unimplemented-bearing triggers, so a card only enters it once its parse binds. great desert hellion -> BATCH_GENUINE_ROWS. Its LTB Draw was Unimplemented until phase-rs#5721/phase-rs#5719 bound Intensity{Source}. Each co-departing Hellion draws off its OWN intensity but discards the SHARED hand, so the second trigger discards the cards the first just drew: with intensities a != b the final hand, graveyard and library differ by order. The members are not identical functions, so commutation genuinely fails and the new prompt is the CR 603.3b choice the legacy serde walk wrongly auto-ordered (CR 603.5: each "may" is chosen on resolution). planar collapse -> DOCUMENTED_OVER_PROMPT (L8-held family). New fixture key. Upkeep ObjectCount(Creature) >= 4 intervening-if x DestroyAll + self-Sacrifice: the first copy's sweep drives the census to 0, so the sibling's CR 603.4 re-check is false and it does nothing. Monotone and self-limiting — identical siblings commute up to relabeling, so the prompt is conservative, fail-closed and rules-correct. Neither row weakens the gate: both are direction-gated over-prompts (an under-prompt is never suppressible), and both are consumed by the ledger's exact-set asserts (over_prompt_hit 18->19, batch_genuine_hit 1->2), so a misclassification still trips the STRICT PROOF-GATE. Verification: engine lib 16481/16481 pass (was 16480 + 1 red); integration 2929/2929 pass. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
2 participants
Summary
Repairs the reveal-partition parser class — the "Put all
<filter>revealed this way into<zone>and the rest on the bottom/top of<library>[in a random/any order]" shape — and lands the Secret Lair headline card The Fourteenth Doctor (its cast-trigger mill partition and its enter-as-a-copy-from-your-graveyard replacement).Real population (correcting the earlier "affecting 1 card(s)")
The earlier body understated this as 1 card. The head-current parse-diff measures 22 cards after the reviewer fix below. (@matthewevans measured 23 cards / 38 signatures at review time; this revision narrows the Muse Vortex guard, which returns Muse Vortex to byte-identical-with-
mainbehavior and removes it from the diff → 22 cards, all correct class repairs.)This is "build for the class, not the card" working as intended — not scope contamination. The fix dispatches onto existing runtime handlers plus one parser-IR field (
rest_library_position); no new engine enum variant. The headline card The Fourteenth Doctor is one of the 22.Why 22, not 1 — the reveal-partition class
On
mainevery card of this shape mis-parsed to a single-cardPutAtLibraryPosition(count=1, position=Bottom, target=<subtype>): it never put the primary pile into hand/graveyard, and it put one filtered card on the bottom instead of the complement. Plainly broken. Repaired cards (Fix A, reveal-partition): Goblin Ringleader, Sylvan Messenger, Merfolk Wayfinder, Enshrined Memories, Alrund God of the Cosmos, Brass Herald, Garruk Caller of Beasts, Ajani Unyielding, Lair Delve, Kavu Howler, Enlistment Officer, Grave Defiler, Tidal Courier, and The Fourteenth Doctor's own cast-trigger mill. Each now parsesRevealTop → ChangeZoneAll(primary) → ChangeZoneAll(complement, library, bottom/top), with"in a random order"→random_order: truerouted to whichever pile returns to the library (CR 401.4) vs"in any order"→ owner-arranged (random_order: false).The remaining repairs are Fix B clone-source field repairs — e.g. Activated Sleeper now enforces its
"put there from the battlefield this turn"restriction (previously dropped entirely) via aFilterProp::ZoneChangedThisTurnon the copy-source filter.Reviewer response — @matthewevans (changes requested)
Blocker 1 — declared population off by 23×. Fixed in this body (see Real population above): the true population is 22 correct reveal-partition / clone-source class repairs (was mislabeled "1 card"), and the class is named. None of the 22 is scope contamination.
Blocker 2 — Muse Vortex mis-binding. Fixed in
602fe5a54. Theis_this_way_partitionguard inparse_put_imperativewas scoped to the bare"this way", which also matched Muse Vortex's exile-origin partition — "put the exiled instant and sorcery cards that weren't cast this way into your hand and the rest on the bottom of your library in a random order" — whose primary subject is a negation over the cast set, not the"put all <filter> revealed this way into <zone>"reveal shape the guard's own comment names. With the broad guard it was hijacked onto the tracked-set path and bound a phantomChangeZone { target: TargetFilter::Any }("any target") into the hand, on a card with no targeting at all."revealed this way"reveal-origin anaphor — exactly the class the comment documents.mainpositional handling —ExileTop(X) → CastFromZone → PutAtLibraryPosition { target: ExiledBySource, position: Bottom }— with no phantomany target.muse_vortex_exile_partition_does_not_bind_phantom_any_targetasserts no hand-boundChangeZone/ChangeZoneAllcarriesTargetFilter::Any."the rest" + "this way"but not"revealed this way"(the complete set that could flip under this guard). Zero phantomAnybindings, zero partition-clause regressions; the pre-existingUnimplementednodes on Collision of Realms / The Joiner of Cats / Yuna's Whistle are on unrelated clauses ("who [condition]", token-copycreate, reflexive counterwhen) and are identical tomain.Blocker 3 — unexplained
static_structureremoval. The card is The Fourteenth Doctor — its own clone clause, and the intended headline repair (unparsed → parsed), not a regression.<...>in your graveyard" (possessive) — the exact pattern Fix B's clone-source-zone table addition (" in your graveyard"→Zone::Graveyard+ControllerRef::You) newly parses. Onmainno clone-source candidate matched"in your graveyard"(only" in a graveyard"/" in any graveyard"/" on the battlefield"), so the clause fell through toEffect::Unimplemented { name: "static_structure" }.BecomeCopyreplacement gated onFilterProp::ZoneChangedThisTurn { from: Library, to: Graveyard }with a reflexiveHastesub-ability (0 Unimplemented / 0 Unknown).static_structure. The other clone-from-graveyard cards (Activated Sleeper, Body Double, The Mimeoplasm, Superior Spider-Man, Naga Fleshcrafter, Mirrorhall Mimic, Chameleon) use"in a/any graveyard"or battlefield, whichmainalready parsed — Fix B only adds aZoneChangedThisTurnfilter prop to those (a field-level change, not astatic_structureremoval).Cards corrected
Fix
FIX A (cast-trigger reveal partition —
imperative.rs,ast.rs,oracle_effect/mod.rs). The "put all<filter>revealed this way into<z1>and the rest on the bottom of<library>" partition previously hijacked to a single-cardPutAtLibraryPosition{Bottom}filtered to the subtype (dropping the primary move, inverting the partition). Added anis_this_way_partitiongating pre-filter that diverts the sentence to the tracked-set path, a parser-IR fieldrest_library_position: Option<LibraryPosition>onPutImperativeAst::ZoneChangeAll, aparse_put_rest_library_positiontail-scan helper, and routing so"in a random order"(random) vs"in any order"(owner-arranged) lands on the correct pile. Reviewer revision: the guard's anaphor is narrowed from the bare"this way"to"revealed this way"so exile-origin negation partitions (Muse Vortex) are not hijacked (Blocker 2).FIX B (enter-as-copy-from-graveyard replacement —
oracle_replacement.rs,oracle_target.rs). Added possessive" in your graveyard"to the clone source-zone table (owner-scoped viaControllerRef::You, B1), madeparse_zone_changed_this_turn_suffixpub(crate)(B2), consumed the "that was put there from your library this turn" clause intoFilterProp::ZoneChangedThisTurn { Library→Graveyard }on the copy-source filter (B3), and accepted the "if you do" reflexive connector (B4). The Fourteenth Doctor now parses to aBecomeCopyreplacement gated on the this-turn zone change with a reflexive Haste sub-ability; 0 Unimplemented / 0 Unknown.DEVIATION (proven by the mandated card-test). Plan §6 predicted the reflexive condition would be
WhenYouDo, butparse_effect_chainmaps"if you do"toEffectOutcome{OptionalEffectPerformed}. The runtime card-test proved that condition never fires on theBecomeCopycopy-completion path, whereaseffects/mod.rsdocumentsWhenYouDoas THE condition for BecomeCopy reflexives. A scoped normalization inparse_when_you_do_reflexiveconvertsEffectOutcome{OptionalEffectPerformed}→WhenYouDofor clone-replacement reflexives only.Files changed
crates/engine/src/parser/oracle_effect/imperative.rs(Fix A + Blocker-2 guard narrowing)crates/engine/src/parser/oracle_effect/tests.rs(Blocker-2 Muse Vortex pin + partition class tests)crates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_replacement.rscrates/engine/src/parser/oracle_target.rscrates/engine/src/game/engine_replacement.rscrates/engine/tests/integration/the_fourteenth_doctor_graveyard_copy.rscrates/engine/tests/integration/main.rscrates/engine/tests/fixtures/integration_cards.jsonCR references
Verification
cargo fmt --all— clean.check-parser-combinators.sh(base =upstream/mainmerge-base) — Gate A PASS.cargo clippy -p engine -p phase-ai -p engine-wasm --all-targets -- -D warnings— clean, exit 0.cargo test -p engine -p phase-ai— PASS (0 failed); includesmuse_vortex_exile_partition_does_not_bind_phantom_any_target, the reveal-partition class tests, and thethe_fourteenth_doctor_mills_doctor_then_enters_as_hasty_copycast-pipeline integration test.oracle-genon The Fourteenth Doctor, Goblin Ringleader (class), Muse Vortex, and all 24"the rest"+"this way"-but-not-"revealed this way"cards — correct AST, no phantomAnybindings, no partition-clause Unimplemented.🤖 Generated with Claude Code