fix(parser): preserve filtered partitions in do-the-same clauses - #6854
Merged
Conversation
Contributor
📝 WalkthroughWalkthroughThe parser supports type-only “do the same for ChangesTyped continuation replication
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant SequenceParser
participant EffectReplicator
participant EffectResolver
OracleText->>SequenceParser: parse "do the same for <type>"
SequenceParser->>EffectReplicator: provide continuation and antecedent effect
EffectReplicator->>EffectReplicator: replace typed target filters
EffectReplicator->>EffectResolver: emit retyped effect
Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
matthewevans
force-pushed
the
maintainer/rescue-5774-do-same-type
branch
4 times, most recently
from
August 2, 2026 09:21
e87c5c7 to
6ca6606
Compare
"..., then do the same for <type> cards" replicates the immediately-preceding mass zone-change for a sibling card type. Estrid, the Masked's ult — "Return all non-Aura enchantment cards from your graveyard to the battlefield, then do the same for Aura cards." — dropped the Aura return entirely (#4779): the comma-"then do the same" tail was glued into the first clause, and the "do the same for <type>" verb was never recognized. Parser-only, no new engine variant: - split_comma_clause_boundary: treat ", then do the same for <type>" as a Then boundary. The "do the same" verb is not in the imperative-verb table, so — mirroring the villainous-choice guard directly above — the continuation was glued into the prior clause and dropped. - new try_parse_do_the_same_for_type recognizer + chunk-loop dispatch that clones the antecedent sibling effect and swaps its type filter. This is the same antecedent-clone mechanic try_parse_scoped_does_the_same uses for the player-scoped fan-out, so it emits an ordinary sibling Effect (no disposition, resolver, or scope added). Estrid now emits both returns: non-Aura enchantments, then Auras — zones and controller preserved (CR 608.2c: the antecedent action is replicated modulo the stated type substitution). Building-block level: covers the "do the same for <type>" clause class, not Estrid alone. Closes #4779.
…tion Addresses review on #5774: the general path must not touch continuations it cannot model. Three narrowings keep the blast radius to exactly the handled class (Estrid); every richer sibling stays exactly as before and strict-fails until its full grammar lands (CR #1: a flagged gap beats a silent misparse). - Recognizer (try_parse_do_the_same_for_type): drop the broader "repeat this process for" family; accept ONLY a pure card-type substitution — reject any filter carrying a FilterProp predicate (Gruesome Menageries
matthewevans
force-pushed
the
maintainer/rescue-5774-do-same-type
branch
from
August 2, 2026 09:22
6ca6606 to
4495e9d
Compare
matthewevans
force-pushed
the
maintainer/rescue-5774-do-same-type
branch
from
August 2, 2026 09:42
4495e9d to
34bc8e2
Compare
![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: 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_4779_do_same_for_type.rs`:
- Around line 36-41: Update the fixture setup in issue_4779_do_same_for_type to
move both cards through the authoritative ProposedEvent::ZoneChange pipeline
instead of zones::move_to_zone, ensuring replacement effects can modify their
entry into the graveyard.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 739f6bca-243d-4602-8505-cc8aee304c09
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/issue_4779_do_same_for_type.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_effect/mod.rs
- crates/engine/src/parser/oracle_effect/tests.rs
- crates/engine/src/parser/oracle_effect/sequence.rs
Comment on lines
+36
to
+41
| engine::game::zones::move_to_zone( | ||
| runner.state_mut(), | ||
| object_id, | ||
| Zone::Graveyard, | ||
| &mut setup_events, | ||
| ); |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Use the replacement-aware zone-change pipeline for fixture setup.
zones::move_to_zone bypasses ProposedEvent::ZoneChange. This bypasses replacement effects that can modify entry to the graveyard. The test can then validate a state that normal game actions cannot produce. Set up both cards through the authoritative zone-change pipeline. CR 608.2c permits replacement effects to modify instructions. (media.wizards.com)
🤖 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/tests/integration/issue_4779_do_same_for_type.rs` around lines
36 - 41, Update the fixture setup in issue_4779_do_same_for_type to move both
cards through the authoritative ProposedEvent::ZoneChange pipeline instead of
zones::move_to_zone, ensuring replacement effects can modify their entry into
the graveyard.
Source: Path instructions
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.
Rescues closed contributor PR #5774 for the
then do the same for <type> cardsparser pattern.Preserves nested tracked-set filters, handles the exact Glimpse of Tomorrow continuation, and includes parser plus runtime coverage.
Summary by CodeRabbit
New Features
Bug Fixes
Tests