fix(parser): UNSUPPORTED seam: Play historic from graveyard once/turn + granted leave-battlefield re#5762
Open
ntindle wants to merge 2 commits into
Conversation
… + granted leave-battlefield re
…3-unsupported-seam-play-historic
Contributor
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
requested changes
Jul 13, 2026
matthewevans
left a comment
Member
There was a problem hiding this comment.
Changes requested — the new leave-battlefield rider is recovered from its source rather than from the permission that actually authorized this play/cast.
🔴 Blocker
crates/engine/src/game/casting.rs:2255-2273re-scans every graveyard permission on the source and takes the first one with a rider. It does not retain the selected permission definition (or rider) whoseaffectedfilter admitted this particular spell. A source with two overlappingGraveyardCastPermissiondefinitions can therefore attach the wrong rider. Carry the selected permission provenance/rider throughGraveyardPermissionSourceandCastingVariant::GraveyardPermission, rather than looking it up after the cast. Add a collision regression proving that a spell admitted by a no-rider permission does not receive a different permission's rider.crates/engine/src/game/casting.rs:2282-2287repeats the same source-wide lookup for lands; cover that path in the collision regression too.- CI currently fails with
E0063atcrates/mtgish-import/src/convert/player_effect.rs:243: the newStaticMode::GraveyardCastPermissionfield is not initialized. Threadgranted_replacement: None(or the appropriate mapping) through every external constructor.
🟡 Non-blocking
crates/engine/src/game/casting.rs:3173-3178also changes emblem/static-definition behavior as an unrelated side effect. Please split it out or add focused evidence that it is required for this permission path.
✅ Clean
- The parser uses a typed static mode and the runtime tests cover the core permanent, persistence-after-source-leaves, and land-play scenarios. The verified Oracle text for The Eighth Doctor does require the granted leave-battlefield replacement.
Recommendation: retain the selected authorizing permission at choice time, use it for both spell and land finalization, add the conflicting-permission regressions, and repair the initializer before requesting re-review.
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
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
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED seam: Play historic from graveyard once/turn + granted leave-battlefield replacement rider on the played object
Cards corrected
Fix
Implemented the approved plan for Cluster-113 (The Eighth Doctor): a play/cast-from-graveyard permission that GRANTS a per-object leave-the-battlefield → exile replacement to the permanent played this way. Built the general carrier seam, not a Doctor-special-case. No new enum variant — two struct-field additions plus a shared install helper and a shared parser def-builder.
WHAT CHANGED (by subsystem):
Types: added
granted_replacement: Option<Box<ReplacementDefinition>>as a fourth leaf rider onStaticMode::GraveyardCastPermission(serde-only, Display/FromStr default None; Hash/Display already destructure with..so no change there — ReplacementDefinition lacks Hash, confirmed). Added transientGameState.pending_etb_replacements: Vec<(ObjectId, Box<ReplacementDefinition>)>mirroringpending_etb_countersat all four lifecycle sites (field/Default/destructure/PartialEq).Parser: extended
parse_leave_battlefield_rider_refwith a~self-ref alt arm; extracted sharedparse_leave_battlefield_exile_replacement_def(byte-identical effect-path output preserved via a thin wrapper); replaced the honest-gapreturn Nonebail intry_parse_disjunctive_graveyard_cast_permissionwith a nomsplit_once_on(". if you do, it gains ")+delimited(char('"'), take_until, char('"'))quote-strip that feeds the shared builder (rider present-but-unparsable still aborts — honest gap preserved).Runtime: new single-authority
GameObject::install_replacement(def, to_base)(add_target_replacement.rs refactored to call it); readerspermission_granted_replacement/selected_static_permission_granted_replacement(spell, CR 607.1 self-grant fallback) /graveyard_permission_granted_replacement(land) in casting.rs; spell seam in finalize_cast (casting_costs.rs) and land seam in handle_play_land (engine.rs) both push ontopending_etb_replacements; the drain+base-install lives at the top ofapply_zone_delivery_tailgated onto == Battlefield— the single convergence point of the immediate deliver fall-through AND the counter-pause resume, so it fires exactly once; turn-cleanup backstop clear in turns.rs.VERIFICATION (worktree off-Tilt, cargo run directly): cargo fmt clean; precise parser diff-gate on my added lines = CLEAN (no string dispatch);
cargo clippy -p engine -p phase-ai -p engine-wasm -- -D warnings(no --all-targets per disk ceiling) = 0 warnings (covers phase-ai/engine-wasm lib exhaustiveness);cargo test -p engine= 16517 lib + 2958 integration, 0 failed;cargo test -p phase-ai= 0 failed. oracle-gen for The Eighth Doctor now emits the GraveyardCastPermission static with granted_replacement (Moved/SelfRef → Exile), the mill-3 trigger, NO Unimplemented, and parse_warnings: [].RUNTIME PROOF (new card-test, GameScenario+GameRunner, 3 tests all green): (1) cast a historic permanent from graveyard via the permission → enters battlefield with the redirect on base_replacement_definitions, then Destroy → lands in EXILE (proves SelfRef Moved install fires and binds to the entrant, not the Doctor); (2) CR 611.2c persistence — remove the Doctor, then the grantee's exit still redirects to exile; (3) net-new land path — play a historic land from graveyard via PlayLand → rider installed on the land's base, exit → exile. Building-block parser tests: honest-gap test flipped to assert Some(rider) with correct shape; def-builder tested on both
~and pronoun forms.NEIGHBOR REGRESSIONS re-run green: l02_bb3_cast_permission_finality (all), finality_counter_death_to_exile (all 4 death paths), ark_of_hunger land-play, mill_rest_in_peace_redirect, living_death_replacement, statics serde/Display round-trip, casting rules integration.
JUDGEMENT CALLS: (a) followed orchestrator disk-ceiling override (NO --all-targets) rather than the plan's literal
--all-targets; lib clippy still catches the non-exhaustive-match class in phase-ai/engine-wasm, which is the whole exhaustiveness risk. (b) install_replacement doc cites CR 613.1 + CR 614.1a (accurate to base-restore + replacement semantics) rather than the plan's tentative CR 611.2b, since 611.2b is specifically 'for as long as' durations; the preserved 611.2b in the add_target_replacement invariant comment remains correct for the ControllerControlsSource ('for as long as you control') case.STOP-AND-RETURN: none — no CR ambiguity, no multi-PR seam. The adjacent reanimation-rider class (Gruesome Encore etc.) uses a different carrier (effect rider) and is explicitly out of cluster-113 scope; the extracted def-builder + install helper are the seams that unlock it later.
VERIFICATION NOT RUN (disk preservation, ~6Gi free): cargo semantic-audit, cargo coverage, and frontend pnpm type-check/lint. The targeted oracle-gen already proved this specific card parses clean (no Unimplemented, no swallow warnings); no frontend code changed and the new field is an optional serde field (backward-compatible, tsify auto-regenerated). Also note: the full
./scripts/check-parser-combinators.shreports FAIL, but it diffs against an ancient fixed base commit (ff799f8, PR #904) and every flagged line is pre-existing code in oracle.rs/other files — the precise per-diff gate on my added lines is clean.Files changed
CR references
Verification
cargo fmt --all— clean (exit 0, no changes; nothing to commit)check-parser-combinators.sh (scoped to upstream/main merge-base 7c63d29fe)— clean (Gate A PASS, exit 0)cargo clippy -p engine -p phase-ai -p engine-wasm -- -D warnings— clean (exit 0)cargo test -p engine -p phase-ai— clean (exit 0, 22 suites ok, 0 failed)oracle-gen data --filter "the eighth doctor"— clean (exit 0, AST matches oracle text, no Unimplemented/Unknown)Cards confirmed re-parsed correctly: The Eighth Doctor
🤖 Generated with Claude Code