fix(parser): preserve TrackedSet on plural 'those tokens' delayed exile#6199
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
There was a problem hiding this comment.
Code Review
This pull request resolves issue #5972 by ensuring that plural 'those tokens' references, which are rewritten to TrackedSet, are not incorrectly overwritten by LastCreated in rewrite_parent_target_to_last_created. The match criteria in lower.rs have been updated to exclude TargetFilter::TrackedSet, and the twinflame_full_parse test has been expanded to assert that the delayed trigger's zone change correctly targets the TrackedSet. No review comments were provided, so there is no additional feedback to address.
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 · 1 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the parser change identifies the plural anaphor but sends battlefield tokens to an exile-only delayed-trigger resolver.
🔴 Blockers
-
crates/engine/src/parser/oracle_effect/lower.rs:3032-3045preservesTrackedSetfor the cleanup, butcrates/engine/src/game/effects/delayed_trigger.rs:687-737then turns every delayedChangeZone { TrackedSet }intoChangeZoneAll { origin: Exile }. The current parse artifact already shows Mysterio's cleanup origin changingbattlefield → ∅. Saheeli, Mysterio, and Twinflame create the referenced tokens on the battlefield, so this resolver cannot exile them. Move the completion semantics to a zone/provenance-aware tracked-set binding atbind_tracked_set_to_effect, and add a production delayed-trigger regression that creates multiple tokens, resolves the cleanup, and proves a token that left the battlefield first is not found. -
The current head fails
parser::oracle::tests::altair_ibn_la_ahad_for_each_exile_memory_counter_copy_parsesatcrates/engine/src/parser/oracle_tests.rs:800: Altaïr's printed “Exile those tokens at end of combat” now lowers toTrackedSet, while the stale test requiresLastCreated. Update this sibling through the same runtime design; do not restore the single-token binding. -
The current
<!-- coverage-parse-diff -->reports only Mysterio, while the PR claims Saheeli.parse_effect_chainshape assertions do not prove the actual card pipeline or resolution. Reconcile the card-level diff and add the runtime proof for the claimed card class.
✅ Clean
- Scryfall confirms the relevant plural cleanup wording for Saheeli, Mysterio, and Altaïr. The parse-diff is current, and the existing Gemini review raised no separate technical finding.
Recommendation: request changes; move the completion semantics into the delayed-trigger tracked-set binding seam, then cover the multiple-token and left-battlefield cases through the real resolution pipeline.
|
Maintainer fixup pushed to
The refreshed CI suite is now the verification authority. |
|
Hi, @matthewevans |
|
Acknowledged. The maintainer fixup is still the current head, but its current CI run is red, so this remains blocked pending the failing-check diagnosis and a green rerun. |
de8a5fa to
903d268
Compare
|
Maintainer fixup pushed to The failing assertion required Saheeli in the deliberately curated integration fixture, but that fixture contains Twinflame only. I removed the invalid duplicate card-data assertion; the parser-shape test and the real Twinflame delayed-cleanup regression remain. CI has restarted for the new head. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved after the maintainer fixups and branch update. The delayed-trigger binding now preserves the battlefield-origin tracked set, the remaining runtime regression exercises two Twinflame tokens through end-step cleanup, and the invalid fixture assertion was removed. Required CI is still running; merge queue will wait for it.
Closes #5972
Summary
Discord report: Saheeli, the Gifted's [-7] creates a token copy of each artifact you control, grants them haste, then should exile all of those tokens at the next end step — but only the last-created token was exiled.
Root cause: after
rewrite_parent_targets_to_tracked_setcorrectly bound plural "those tokens" toTrackedSet { id: 0 }withuses_tracked_set: true, the post-tokenresolve_populated_token_anaphorspass rewroteTrackedSet→LastCreatedon the delayedChangeZoneexile.LastCreatedonly snapshots the final token in a multi-token batch.Fix: preserve
TrackedSetinrewrite_parent_target_to_last_created'sChangeZonearm. Singular "it"/"that token" cleanup (Flameshadow Conjuring, Inalla) still rewritesParentTarget/TriggeringSource→LastCreated.Changes
crates/engine/src/parser/oracle_effect/lower.rs— stop stompingTrackedSettoLastCreatedon delayed plural-token exile cleanup.crates/engine/src/parser/oracle_effect/tests.rs— Saheeli -7 regression: delayed exile staysTrackedSet+uses_tracked_set: true.crates/engine/src/parser/oracle_tests.rs— Twinflame regression: inner delayed exile target isTrackedSet, notLastCreated.Test Plan
cargo fmt --all -- --checkcargo test -p engine --lib saheeli_minus_seven_those_tokens_delayed_exile_keeps_tracked_setcargo test -p engine --lib twinflame_full_parsecargo test -p engine --lib etb_token_copier_exile_anaphor_binds_created_tokencargo test -p engine --lib delayed_trigger_change_zone_after_token_creator_rewrites_to_last_created./scripts/gen-card-data.shor Tiltcard-data) so Saheeli's [-7] execute body picks up the parser fix in runtime