fix(engine): preserve Memory Jar delayed end-step scope - #7257
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe parser now represents “cards they exiled this way” as tracked filtered targets. Delayed effects retain tracked-set metadata, propagate player scope, infer exile origins, and recursively bind tracked-set IDs during resolution. Parser and Memory Jar integration tests cover the behavior. ChangesTracked exile sets in delayed effects
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
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)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.1)crates/engine/src/parser/oracle_effect/mod.rsast-grep timed out on this file 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}
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/delayed_trigger.rs (1)
1024-1031: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winPreserve
TrackedSetFilteredduring theChangeZoneupgrade.The
_arm replacesTrackedSetFilteredwithTrackedSet. This drops the nested filter andThisWayCause::Exiledprovenance from"card(s) they exiled this way".Handle
TargetFilter::TrackedSetFiltered { .. }explicitly. Clone it and callrebind_tracked_set_sentinel(real_id)before constructingChangeZoneAll.🤖 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/game/effects/delayed_trigger.rs` around lines 1024 - 1031, Update the target conversion around bound_target to handle TargetFilter::TrackedSetFiltered explicitly: clone the filtered target, call rebind_tracked_set_sentinel(real_id), and preserve it when constructing ChangeZoneAll. Keep the existing TrackedSet and fallback behavior unchanged, while ensuring nested filters and ThisWayCause::Exiled provenance are retained.
🤖 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/game/effects/delayed_trigger.rs`:
- Around line 1048-1059: Update bind_tracked_set_to_ability_definition to
traverse every entry in ability.mode_abilities and recursively call itself with
real_id, ensuring tracked-set targets in mode abilities are rebound from the
sentinel ID.
In `@crates/engine/src/parser/oracle_effect/tests.rs`:
- Around line 17887-17890: Update the regression test’s parse_effect_chain input
to use Memory Jar’s exact sequencing text, replacing “and” with “then” between
the discard and return instructions. Keep the test focused on the actual Memory
Jar parser path so it fails before the fix and verifies the return instruction
remains correctly chained.
---
Outside diff comments:
In `@crates/engine/src/game/effects/delayed_trigger.rs`:
- Around line 1024-1031: Update the target conversion around bound_target to
handle TargetFilter::TrackedSetFiltered explicitly: clone the filtered target,
call rebind_tracked_set_sentinel(real_id), and preserve it when constructing
ChangeZoneAll. Keep the existing TrackedSet and fallback behavior unchanged,
while ensuring nested filters and ThisWayCause::Exiled provenance are retained.
🪄 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: d29718c2-39ac-4703-8a11-de71c1c1fecf
📒 Files selected for processing (7)
crates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/memory_jar_delayed_end_step.rs
|
Generated for head Parse changes introduced by this PR · 13 card(s), 13 signature(s) (baseline: main
|
|
Maintainer update — fixes applied; waiting on current-head CI. I pushed The implementation review is otherwise complete. Required CI has restarted for this exact head; I will approve and enqueue once it settles green. |
98b95f6 to
9e3efb5
Compare
|
Maintainer update — corrected fixup; waiting on current-head CI. The prior fixup omitted the runtime CI has restarted for this exact head. I will approve and enqueue only after the current-head required checks settle green. |
… into fix/memory-jar-delayed-end-step # Conflicts: # crates/engine/src/game/effects/delayed_trigger.rs # crates/engine/src/parser/oracle_effect/tests.rs
… into fix/memory-jar-delayed-end-step # Conflicts: # crates/engine/src/game/effects/delayed_trigger.rs # crates/engine/src/parser/oracle_effect/tests.rs
|
Correction to my earlier maintainer update: Memory Jar's current Oracle text uses “and” between the delayed discard and return instructions, not “then” (Scryfall). The production integration fixture is therefore exact at |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — independently re-reviewed at 793ea4cded02c1836954695e77796b09dd7a35c8.
✅ Clean
- The delayed-chain parser now lowers the complete inner IR and binds its tracked-set consumer before scheduling (
oracle_effect/mod.rs:32250-32273); the production Memory Jar scenario exercises both players through activation and end-step resolution (memory_jar_delayed_end_step.rs:137-171). - The delayed resolver preserves
TrackedSetFilteredprovenance and recursively binds nested definition and runtime modal branches (delayed_trigger.rs:1024-1082). - The current-head parse-diff is present and attributable to this SHA; the deliberate Memory Jar/Magus tracked delayed-return changes were checked alongside the listed related parser deltas. All required CI checks for this SHA are successful.
Recommendation: enqueue through the merge queue.
2 participants
Summary
Preserve the per-player scope and tracked exiled-card provenance across Memory Jar's delayed end-step effect. Delayed effects now retain complete parser chains, recursively rewrite player-relative references, preserve
ThisWayCause::Exiled, and recursively rebind tracked-set sentinels at runtime.Files changed
crates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_target.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/memory_jar_delayed_end_step.rsTrack
Developer
LLM
Model: GitHub Copilot (canonical id not exposed)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
Verification
cargo fmt --all -- --check— PASS../scripts/check-parser-combinators.sh— PASS.cargo clippy --all-targets -- -D warnings— PASS.cargo test -p phase-engine --lib temporal_prefix_preserves_full_delayed_effect_chain && cargo test -p phase-engine --lib return_each_card_they_exiled_this_way_uses_exiled_tracked_set— PASS.cargo test -p phase-engine --lib uses_tracked_set_rebinds_filtered_change_zone_all— PASS.cargo test -p phase-engine— 4,813 passed, 0 failed, 2 ignored.cargo test -p phase-engine --test integration memory_jar_delayed_end_step::memory_jar_delayed_discard_stays_scoped_to_each_player— PASS../scripts/gen-card-data.sh— PASS.cargo coverage— 88.8%.CARGO_BUILD_JOBS=1 cargo semantic-audit—Audit complete: 32747 cards audited, 266 with findings.CARGO_BUILD_JOBS=1 cargo parser-gaps— completed successfully; informational result:Parser Gap Analysis: 4000 unsupported cards, 4653 classified gaps.2826 rules across 115 files,1802implemented,1024missing,63.8%coverage, no validation errors.0 1forupstream/main...HEAD, and nogit diff --checkoutput.Gate A
Gate A PASS head=7cdabeffdaf05e9b8ca3b5497d3e189a855c7a8b base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
crates/engine/src/types/ability.rs:15045— existing recursiveTargetFilter::rebind_tracked_set_sentinelauthority.crates/engine/src/parser/oracle_effect/mod.rs:27408— existing recursive player-scope rewrite authority.Final review-impl
Final review-impl PASS head=7cdabeffd
Claimed parse impact
Memory Jarnow parses its delayed end-step text as the complete discard-and-return chain, with each player's hand and exiled-this-way cards remaining independently scoped. The generic parser coverage also handles equivalenteach card(s) they exiled this wayreturn effects while preserving exiled-set provenance and the exile origin zone.Scope Expansion
The initial player-scope recursion fix expanded to the complete delayed-chain parser path,
ThisWayCause::Exiledprovenance andZone::Exileinference, and recursive runtime rebinding through filtered target trees, nested ability chains, and delayed payload definitions. This expansion is covered by parser unit tests, runtime binder tests, and the production-entry Memory Jar integration test.Validation Failures
None in the implementation. The repository-default
cargo rules-audit --cannot read its intentionally removedrules/registry; the current audit was run against a fresh temporary registry and passed with no validation errors.CI Failures
None. CI has not run yet; this newly opened PR will trigger it.
Summary by CodeRabbit
Bug Fixes
Tests