fix(engine): retain look-result loop members - #7285
Conversation
|
Warning Review limit reached
Next review available in: 8 minutes You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (4)
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 (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughChangesThe engine now carries parent-produced iteration member IDs through child effects. Matching uses ordered candidate snapshots. Member-driven repeats handle per-member payment resolution and repeat state. Visibility projections redact private iteration IDs. Regression tests cover payment resolution and target preservation. Iteration target flow
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Player
participant Effects
participant Quantity
participant ViewerState
Player->>Effects: resolve Moonlight Bargain payment iteration
Effects->>Quantity: match targets within carried Dig IDs
Quantity-->>Effects: return one revealed card target
Effects->>ViewerState: redact private iteration IDs
ViewerState-->>Player: present payment choice
Player->>Effects: accept or decline card
Effects-->>Player: move card to hand or graveyard
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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/mod.rs (1)
11390-11450: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winStamp propagated targets for member-driven repeats.
When this branch propagates object targets to a sub-ability with
repeat_for: ObjectCountand aParentTargetreference,apply_parent_chain_contextclearsparent_target_iteration_members. The resolver then scans the battlefield instead of the propagated targets.Call
stamp_parent_target_iteration_members(&mut sub_with_targets)afterapply_parent_chain_context.🤖 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/mod.rs` around lines 11390 - 11450, The target propagation branch should preserve member-driven repeat context when the sub-ability uses repeat_for: ObjectCount with a ParentTarget reference. After apply_parent_chain_context(&mut sub_with_targets, ...), call stamp_parent_target_iteration_members(&mut sub_with_targets) before resolve_ability_chain so resolution iterates the propagated targets rather than rescanning the battlefield.
🤖 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/mod.rs`:
- Around line 5895-5906: Update rebind_member_driven_parent_target so member
rebinding replaces only the relevant object target while preserving all
non-object targets, especially the selected TargetRef::Player. Keep the existing
complete-list replacement behavior for the carried look-result collection, but
include retained non-object targets so SearchLibrary::resolve_library_owner
continues reading the correct player.
In `@crates/engine/src/types/ability.rs`:
- Around line 20475-20479: Update filter_state_for_viewer so every
opponent-facing ResolvedAbility projection clears
PendingCast.ability.parent_target_iteration_members while preserving the field
for the owning viewer; add a regression test verifying the IDs are redacted from
opponent state.
---
Outside diff comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 11390-11450: The target propagation branch should preserve
member-driven repeat context when the sub-ability uses repeat_for: ObjectCount
with a ParentTarget reference. After apply_parent_chain_context(&mut
sub_with_targets, ...), call stamp_parent_target_iteration_members(&mut
sub_with_targets) before resolve_ability_chain so resolution iterates the
propagated targets rather than rescanning the battlefield.
🪄 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: 05c6ad90-1d56-407c-bf2a-5d431dbcaf2b
📒 Files selected for processing (6)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/quantity.rscrates/engine/src/game/stack.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/issue_7151_moonlight_bargain.rscrates/engine/tests/integration/main.rs
19d539a to
dc30f4b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/engine/src/game/effects/mod.rs (1)
3094-3113: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a CR citation to
stamp_parent_target_iteration_members.This function implements a rules-relevant provenance rule (look-result membership ownership), but it carries no
CR <number>: <description>annotation. Sibling functions in the same file citeCR 701.20e + CR 608.2cfor the same concept (apply_parent_chain_contextat Line 2468,rebind_member_driven_parent_targetat Line 5895).Add the same or an equivalent citation here for consistency and future auditability.
As per path instructions, "rules-touching code with no verified
CR <number>: <description>annotation" is a finding forcrates/engine/**.🤖 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/mod.rs` around lines 3094 - 3113, Add a verified rules citation comment to stamp_parent_target_iteration_members, documenting the look-result membership ownership rule; use the same CR 701.20e + CR 608.2c citation as apply_parent_chain_context and rebind_member_driven_parent_target, or an equivalent accurate CR reference.Source: Path instructions
🤖 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/mod.rs`:
- Around line 9592-9600: Update resolve_effect’s deferred unless_pay path to
also detect member-driven abilities with no sub_ability and optional == false,
including iter_effective.unless_pay.is_some() in the chain re-entry condition.
Before re-entering, clear iter_ability.repeat_for so TrackedSetSize continues
using the existing outer repeat driver while the payment is applied per member.
Add a regression test covering this non-optional member-driven unless_pay case.
In `@crates/engine/src/game/visibility.rs`:
- Around line 28-35: Update redact_waiting_for_iteration_members to match every
WaitingFor variant explicitly instead of using a wildcard arm: retain redaction
for UnlessPayment and UnlessPaymentChooseCost, and list all current
non-redaction variants as explicit no-op arms so future ResolvedAbility-carrying
variants cannot bypass projection review.
---
Nitpick comments:
In `@crates/engine/src/game/effects/mod.rs`:
- Around line 3094-3113: Add a verified rules citation comment to
stamp_parent_target_iteration_members, documenting the look-result membership
ownership rule; use the same CR 701.20e + CR 608.2c citation as
apply_parent_chain_context and rebind_member_driven_parent_target, or an
equivalent accurate CR reference.
🪄 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: eb109ebb-ec09-46a5-a03d-68cd342fa379
📒 Files selected for processing (3)
crates/engine/src/game/effects/mod.rscrates/engine/src/game/visibility.rscrates/engine/tests/integration/issue_7151_moonlight_bargain.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/issue_7151_moonlight_bargain.rs
ee1b29e to
d4be186
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
d4be186 to
217be06
Compare
1 participant
Fixes #7151.
Retains the exact Dig result collection for the immediate member-driven loop, so Moonlight Bargain never substitutes unrelated battlefield permanents while resolving its per-card payment prompts.
Summary by CodeRabbit
Bug Fixes
Tests