fix(engine): randomize Dig library remainders - #7319
Conversation
|
Warning Review limit reached
Next review available in: 2 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 (1)
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 ignored due to path filters (2)
📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughThe PR adds ChangesDig rest-ordering
Estimated code review effort: 4 (Complex) | ~45 minutes Mergeability Score: 🔵 Low · up to The PR changes Dig remainder ordering and adds rules-sensitive branching. It is mergeable with owner awareness for the inconsistent or missing rules citations and duplicated shuffle-gating logic, which create bounded verification and maintenance risk but no demonstrated production failure. Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant DigResolution
participant GameRng
participant Library
OracleParser->>DigResolution: create Dig with rest_order
DigResolution->>GameRng: shuffle rest cards when order is Random
GameRng-->>DigResolution: shuffled rest cards
DigResolution->>Library: place rest cards on the library
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 |
|
Generated for head Parse changes introduced by this PR · 3 card(s), 7 signature(s) (baseline: main
|
![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/mtgish-import/src/convert/action.rs (1)
4916-4939: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winAdd a CR citation for the new random-order branch.
Each of these four blocks implements a rules distinction: whether the remaining library cards are randomized (
DigRestOrder::Random) or not (DigRestOrder::Preserve). None of the four carries its own CR citation for this specific branch; the surrounding doc comments cite CR numbers for the overall disposition-to-Dig shape, not for the order determination.Add a short comment citing the applicable CR rule (or an honest
CR ???if the rule text does not name this distinction) next to eachrest_ordercomputation, consistent with the citation already added inast.rsforDigFromAmong.rest_order("CR 400.5 + CR 608.2c").Based on learnings: "Every code arm implementing a Magic rules requirement must include a CR annotation verified against
docs/MagicCompRules.txt; use an honestCR ???annotation when the rule is absent from the text" (crates/mtgish-import/CLAUDE.md).Also applies to: 4966-4989, 5103-5126, 5127-5150
🤖 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/mtgish-import/src/convert/action.rs` around lines 4916 - 4939, Add a short CR citation comment for the Random/Preserve rest_order determination in each affected match arm: the shown Dig conversion and the corresponding blocks around lines 4966, 5103, and 5127. Verify the applicable rule in docs/MagicCompRules.txt, or use an honest “CR ???” annotation if the distinction is not specified, placing the comment next to each rest_order computation and matching the existing DigFromAmong citation style.Source: Learnings
🧹 Nitpick comments (2)
crates/engine/src/parser/oracle_effect/conditions.rs (1)
4339-4357: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocument the
rest_orderfallback alongsiderest_destination.The comment above
alt_effectexplains whyrest_destinationprefers the alternative's inline value and falls back to the preceding Dig's value otherwise. It does not explain the equivalentrest_order: alt_rest.map_or(*prev_rest_order, |_| alt_rest_order)line, whose correctness depends on the same reasoning: whenalt_restisNone,alt_rest_orderis a meaningless default (Preserve) rather than a real alternative-clause value, so falling back toprev_rest_orderis required.Add one sentence to the existing comment covering this field.
🤖 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/conditions.rs` around lines 4339 - 4357, Extend the comment immediately above the alt_effect construction to document rest_order: when alt_rest is None, use prev_rest_order because alt_rest_order is only meaningful for an alternative inline rest destination and otherwise defaults to Preserve.crates/engine/src/game/engine_resolution_choices.rs (1)
760-776: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate shuffle-guard logic between
route_rest_partitionand theDigChoicedirect-push branch.
route_rest_partition(Lines 764-774) and theDigChoicehandler's direct-library-push branch (Lines 3380-3386) both implement the identical rule: shuffle the rest pile only whenrest_zone/rest_destinationisZone::Libraryandrest_order == DigRestOrder::Random. The two implementations carry near-identical CR comments. If a future change adjusts one shuffle condition (for example, to cover an additional zone or a different randomization scope), the other site can silently drift out of sync.Extract a small shared helper, for example
fn maybe_randomize_rest(ids: &mut Vec<ObjectId>, rest_zone: Zone, rest_order: DigRestOrder, rng: &mut impl Rng), and call it from both sites.♻️ Proposed shared helper
+fn maybe_randomize_rest_for_library( + ids: &mut Vec<ObjectId>, + destination_zone: Zone, + rest_order: DigRestOrder, + state: &mut GameState, +) { + // CR 400.5: randomize exactly this rest pile when the printed + // instruction requires a random-order library placement. + if destination_zone == Zone::Library && rest_order == DigRestOrder::Random { + ids.shuffle(&mut state.rng); + } +}Then in
route_rest_partition:- let mut ordered_ids = rest_ids.to_vec(); - if rest_zone == Zone::Library && rest_order == DigRestOrder::Random { - // CR 400.5 + CR 608.2c: Exact Oracle text requires a randomized - // remainder; only this rest pile, not the remainder of the library, - // consumes entropy. - ordered_ids.shuffle(&mut state.rng); - } + let mut ordered_ids = rest_ids.to_vec(); + maybe_randomize_rest_for_library(&mut ordered_ids, rest_zone, rest_order, state);And in the
DigChoicedirect-push branch:- Some(Zone::Library) => { - if rest_order == DigRestOrder::Random { - // CR 400.5 + CR 608.2c: Randomize exactly the - // unchosen pile immediately before bottom placement. - unkept.shuffle(&mut state.rng); - } + Some(Zone::Library) => { + maybe_randomize_rest_for_library(&mut unkept, Zone::Library, rest_order, state);Also applies to: 3362-3396
🤖 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/engine_resolution_choices.rs` around lines 760 - 776, Extract the shared rest-pile randomization condition from route_rest_partition and the DigChoice direct-library-push branch into a helper such as maybe_randomize_rest, accepting mutable ObjectId storage, rest zone/order, and the RNG. Call this helper from both sites, preserving the existing Library plus Random condition and its scoped shuffle behavior, and keep the rule’s explanatory comment in the shared helper.
🤖 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/parser/oracle_ir/ast.rs`:
- Around line 423-426: Update ContinuationAst::PutRest to retain and parse the
“in a random order” suffix into a #[serde(default)] rest_order: DigRestOrder
field, then thread that value through apply_clause_continuation and
patch_rest_destination_recursively when constructing Effect::Dig, preserving
Preserve as the default for other forms.
In `@crates/engine/src/types/ability.rs`:
- Around line 10636-10652: Update the documentation comments for DigRestOrder to
cite CR 401.4 alongside CR 400.5 and CR 608.2c, while preserving the existing
explanation and enum behavior.
---
Outside diff comments:
In `@crates/mtgish-import/src/convert/action.rs`:
- Around line 4916-4939: Add a short CR citation comment for the Random/Preserve
rest_order determination in each affected match arm: the shown Dig conversion
and the corresponding blocks around lines 4966, 5103, and 5127. Verify the
applicable rule in docs/MagicCompRules.txt, or use an honest “CR ???” annotation
if the distinction is not specified, placing the comment next to each rest_order
computation and matching the existing DigFromAmong citation style.
---
Nitpick comments:
In `@crates/engine/src/game/engine_resolution_choices.rs`:
- Around line 760-776: Extract the shared rest-pile randomization condition from
route_rest_partition and the DigChoice direct-library-push branch into a helper
such as maybe_randomize_rest, accepting mutable ObjectId storage, rest
zone/order, and the RNG. Call this helper from both sites, preserving the
existing Library plus Random condition and its scoped shuffle behavior, and keep
the rule’s explanatory comment in the shared helper.
In `@crates/engine/src/parser/oracle_effect/conditions.rs`:
- Around line 4339-4357: Extend the comment immediately above the alt_effect
construction to document rest_order: when alt_rest is None, use prev_rest_order
because alt_rest_order is only meaningful for an alternative inline rest
destination and otherwise defaults to Preserve.
🪄 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: 072b3b6e-848c-4da2-9724-082e51be58d7
⛔ Files ignored due to path filters (1)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__follow_the_lumarets_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (34)
crates/engine/src/database/hideaway.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/effects/choose_card.rscrates/engine/src/game/effects/dig.rscrates/engine/src/game/effects/explore.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/reveal_until.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/visibility.rscrates/engine/src/parser/oracle_effect/conditions.rscrates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/parser/oracle_ir/ast.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/types/ability.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/cost_zone_pipeline.rscrates/engine/tests/integration/dig_impossible_keep_count.rscrates/engine/tests/integration/dig_rest_pile_stranding_on_etb_pause.rscrates/engine/tests/integration/issue_5996_planetarium_look_cast.rscrates/engine/tests/integration/issue_6367_thassas_oracle.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/metamorphic_alteration.rscrates/mtgish-import/src/convert/action.rscrates/phase-ai/src/determinize.rscrates/phase-ai/src/features/control.rscrates/phase-ai/src/features/spellslinger_prowess.rscrates/phase-ai/src/features/tests/graveyard_types.rscrates/phase-ai/src/search.rscrates/server-core/src/session.rs
|
Review follow-up: I verified this against the repositorys current official CR text. I am not adding CR 401.4: it governs the owners choice to arrange simultaneous library placements in any order, whereas this field implements an exact random-order instruction (no player ordering choice). CR 400.5 establishes the library-order constraint and CR 608.2c requires carrying out the cards random-order instruction, so those remain the accurate annotations. |
1 participant
Closes #6367.
Preserves typed ordering semantics for dig-like effects and randomizes library remainders when Oracle text requires it.
Summary by CodeRabbit
New Features
Bug Fixes