fix(engine): honor controller choice for self tucks - #6882
Conversation
|
Warning Review limit reached
Next review available in: 40 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. 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 (11)
📝 WalkthroughWalkthroughChangesPutOnTopOrBottom chooser flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OracleEffectParser
participant PutOnTopOrBottomResolve
participant TargetFilter
participant TopOrBottomChoice
OracleEffectParser->>PutOnTopOrBottomResolve: create effect with target and chooser filters
PutOnTopOrBottomResolve->>TargetFilter: resolve target
TargetFilter-->>PutOnTopOrBottomResolve: matching object
PutOnTopOrBottomResolve->>TargetFilter: resolve chooser
TargetFilter-->>PutOnTopOrBottomResolve: controller or owner
PutOnTopOrBottomResolve->>TopOrBottomChoice: create top-or-bottom choice
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (3)
crates/engine/src/parser/oracle_effect/mod.rs (1)
9076-9081: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a
CR 608.2dannotation totry_parse_self_put_on_top_or_bottom.This function parses a resolution-time choice and currently has no CR citation.
🤖 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/parser/oracle_effect/mod.rs` around lines 9076 - 9081, Add a `CR 608.2d` annotation to the `try_parse_self_put_on_top_or_bottom` function, preserving its existing parsing behavior and placement within the surrounding parser code.Source: Path instructions
crates/engine/src/game/effects/put_on_top_or_bottom.rs (1)
1-55: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the CR citations for
resolved_targets.
CR 608.2ccovers instruction ordering.CR 603.10covers look-back-in-time trigger exceptions. Neither documents the three-tier dispatch orSelfRefhandling. Use the applicable self-reference and object-identity rules, such asCR 201.5aandCR 400.7; retainCR 603.10only for the relevant leaves-the-battlefield trigger context.CR 608.2dremains appropriate for the choice player.🤖 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/put_on_top_or_bottom.rs` around lines 1 - 55, Correct the comments in resolve around resolved_targets: remove the inaccurate claim that CR 608.2c and CR 603.10 document the three-tier dispatch or SelfRef handling, and cite the applicable self-reference/object-identity rules such as CR 201.5a and CR 400.7. Retain CR 603.10 only where it specifically applies to the leaves-the-battlefield trigger context, and leave the CR 608.2d choice-player citation unchanged.Source: Learnings
crates/engine/src/game/coverage.rs (1)
3433-3435: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSuppress the default
chooserso unchanged cards keep a byte-identical coverage signature.
chooserdefaults toTargetFilter::ParentTargetOwner. Emitting it unconditionally adds a newchooserrow to every pre-existingPutOnTopOrBottomcard (Aether Gust, Subtlety, Aetherspouts), none of which changed behavior in this PR. The coverage parse-diff sticky will report those cards as changed.This matches the default-suppression convention already used in this function:
DrawskipsTargetFilter::Controller, and theManaarm (lines 2907-2915) documents emitting each field "only when set so unqualified signatures stay byte-identical".♻️ Emit `chooser` only when it is not the owner default
Effect::PutOnTopOrBottom { target, chooser } => { d.push(("target".into(), fmt_target(target))); - d.push(("chooser".into(), fmt_target(chooser))); + if !matches!(chooser, TargetFilter::ParentTargetOwner) { + d.push(("chooser".into(), fmt_target(chooser))); + } }🤖 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/coverage.rs` around lines 3433 - 3435, Update the Effect::PutOnTopOrBottom arm in the coverage formatter to emit the chooser field only when it differs from TargetFilter::ParentTargetOwner, while always retaining the target field. Follow the existing default-suppression convention used by Draw and Mana so unchanged cards preserve byte-identical coverage signatures.
🤖 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.
Nitpick comments:
In `@crates/engine/src/game/coverage.rs`:
- Around line 3433-3435: Update the Effect::PutOnTopOrBottom arm in the coverage
formatter to emit the chooser field only when it differs from
TargetFilter::ParentTargetOwner, while always retaining the target field. Follow
the existing default-suppression convention used by Draw and Mana so unchanged
cards preserve byte-identical coverage signatures.
In `@crates/engine/src/game/effects/put_on_top_or_bottom.rs`:
- Around line 1-55: Correct the comments in resolve around resolved_targets:
remove the inaccurate claim that CR 608.2c and CR 603.10 document the three-tier
dispatch or SelfRef handling, and cite the applicable
self-reference/object-identity rules such as CR 201.5a and CR 400.7. Retain CR
603.10 only where it specifically applies to the leaves-the-battlefield trigger
context, and leave the CR 608.2d choice-player citation unchanged.
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 9076-9081: Add a `CR 608.2d` annotation to the
`try_parse_self_put_on_top_or_bottom` function, preserving its existing parsing
behavior and placement within the surrounding parser code.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: e1e5b361-8de8-462f-993d-5f661535e834
📒 Files selected for processing (11)
crates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/put_on_top_or_bottom.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/arashin_sovereign_self_tuck.rscrates/engine/tests/integration/cost_zone_pipeline.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PR · 35 card(s), 3 signature(s) (baseline: main
|
9075f61 to
292dc56
Compare
Route the self-reflexive library tuck "put it on your choice of the top or bottom of its owner's library" to the existing Effect::PutOnTopOrBottom. The owner-framing patterns in try_parse_put_on_top_or_bottom recognize "its owner puts it on their choice of the top or bottom of their library" (Aether Gust, Subtlety, Aetherspouts). The controller-framing self form (Arashin Sovereign's dies trigger) had no arm, so the effect body lowered to Effect::Unimplemented and nothing happened at resolution -- Arashin stayed in the graveyard and the whole card rendered unsupported. try_parse_self_put_on_top_or_bottom adds one recognizer arm that routes the SELF form to the same Effect::PutOnTopOrBottom the owner-framing patterns produce -- no new effect variant. It is scoped to the self reference (resolve_it_pronoun == SelfRef): for the dying source's own library tuck the owner IS the controller, so the effect's owner-chooses resolution (CR 401.4) matches the card's "your choice". A non-self "it" (S.N.E.A.K. Dispatcher) or a "that card" subject (Hinder) is declined so it stays an honest coverage gap rather than letting the wrong player choose the position. PutOnTopOrBottom's resolver read ability.targets directly, which is empty for a SelfRef effect (SelfRef is not a chosen target). It now delegates to the shared resolved_targets 3-tier dispatch, matching Effect::Bounce and the other zone-change resolvers -- SelfRef short-circuits to ability.source_id while a chosen/parent target is unchanged (tier 3 returns the pre-selected targets). Adds a discriminating integration test that drives the real dies-trigger -> resolve -> ChooseTopOrBottom pipeline and fails on main.
292dc56 to
4d0ca53
Compare
1 participant
Rescues the valuable self-tuck controller-choice semantics from #5889.\n\n- models the effect-designated chooser separately from the card owner whose library receives the object;\n- parses controller/owner choice forms and preserves the existing owner-default behavior;\n- covers serialization, parser, resolver, and stolen-permanent behavior.\n\nThe protected pre-push gate passed on the published tree (parser gates, engine/AI tests, card-data validation/coverage, frontend lint/type-check).
Summary by CodeRabbit
New Features
Bug Fixes