Partial: Extract Power - #7032
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Warning Review limit reached
Next review available in: 24 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)
📝 WalkthroughWalkthroughThis change updates Oracle parsing for “of each player's library” so look-then-exile effects keep per-player scope during ChangesEach-player library exile
Estimated code review effort: 3 (Moderate) | ~25 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 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 |
|
Maintainer fixup pushed at |
![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
🤖 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_effect/imperative.rs`:
- Around line 217-224: Update the ScopedPlayer detection in the current parser
function to validate the possessive library phrase at the current noun-phrase
boundary instead of scanning all of rest_lower with preceded/take_until. Reuse
parse_library_player_suffix if it supports this grammar, and preserve
ScopedPlayer only when that helper confirms the current library owner.
In `@crates/engine/tests/integration/extract_power_each_player_exile.rs`:
- Around line 264-273: Extend the integration test around the Extract Power
resolution to exercise the retained play permission through the game pipeline,
not just inspect the sub-ability AST. After all-player exile resolves, have P0
cast an exiled P1 or P2 card and assert that the cast succeeds and the card
resolves, covering permission access across the union of exiled cards; retain
AST assertions only as supplemental checks.
🪄 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: 81275b44-6383-42a0-9a75-674358bf6276
📒 Files selected for processing (4)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/sequence.rscrates/engine/tests/integration/extract_power_each_player_exile.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 0 card(s), 0 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — both current-head correctness gaps remain.
🔴 Blocker
[HIGH] The new each-player owner detection is still an unbounded remainder scan. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:217-224 applies take_until("of each player's library") to all of rest_lower, and the caller supplies the whole post-"look at the top " remainder at :2992-3006. The established phrase-to-owner authority, parse_library_player_suffix, instead matches the current card[s] of <owner>'s library noun phrase at :8233-8264. Why it matters: a later "of each player's library" in the same clause can incorrectly turn a dig of a different library into ScopedPlayer; the continuation then materializes an all-player ExileTop fan-out. Suggested fix: bind the owner at the current noun phrase using the shared suffix authority (thread ParseContext if needed), and add a later-phrase negative sibling proving the earlier library remains the selected owner.
[HIGH] The regression test does not execute the retained permission. Evidence: crates/engine/tests/integration/extract_power_each_player_exile.rs:69-141 proves the three cards moved to exile, while :263-272 only checks that the parsed sub-ability is CastFromZone; it never sends an exiled P1/P2 card through GameAction::CastSpell. Why it matters: split_player_scope_chain can preserve an AST node while failing to make its tracked union usable by the casting pipeline. Suggested fix: after Extract Power resolves, use a known castable P1 or P2 top card, cast it as P0 through GameAction::CastSpell, and assert acceptance plus resolution; retain the AST check only as supplemental coverage.
✅ Clean
The 02b5555b follow-up corrects the earlier CR-text assertion, but it changes only test prose and leaves both CodeRabbit findings against the implementation and runtime coverage unresolved.
Recommendation: request changes — bind the library owner at the current noun phrase and add the cross-player runtime cast test before re-review.
matthewevans
removed
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — current head feb51365c5b7fe7e090fabd6f8b0c269c85f65f6
[MED] The each-player dig owner matcher recognizes only the singular noun phrase and leaves the plural sibling controller-scoped. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:217-224 accepts only "card of each player's library", while its documented suffix authority parse_library_player_suffix supports both "card" and "cards" at :8254-8255. The caller passes the post-count noun phrase into this matcher at :2990-3003, so a multi-card form such as “look at the top two cards of each player's library, then exile those cards” falls through to TargetFilter::Controller; the later ExileTop materialization cannot restore the missing all-player scope.
Why it matters: this is the same each-player look-then-exile class the PR claims to fix, but it remains wrong for plural counts.
Suggested fix: bind the current noun phrase through parse_library_player_suffix (threading ParseContext), or extract and reuse its noun-boundary authority. Add a plural each-player regression and a negative case proving a later “of each player's library” phrase does not re-scope an earlier library.
[HIGH] Current-head parser-impact evidence is still missing. The only <!-- coverage-parse-diff --> sticky is bound to 02b5555b7b12be08255efc6b6e30724f8a0b2c28, not this parser-changing head. Publish a fresh artifact bound to feb51365c5b7fe7e090fabd6f8b0c269c85f65f6 and account for its surface before re-review.
Correction/update — current head
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off: current head fixes the library-owner boundary and proves the cross-player cast permission through the production pipeline.
2 participants
Summary
Fixes a parse-fidelity defect on Extract Power.
Issue: ExileTop scope is
player: Controller(only controller's top card), but Oracle exiles the top card of EACH player's library.Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— passed (exit 0)./scripts/check-parser-combinators.sh (Gate A)— passed (Gate G PASS + Gate A PASS, exit 0; Family-D skipped — python3 Windows-Store stub, documented env limitation)cargo clippy-strict— passed (exit 0, no warnings)cargo test -p phase-engine— failed (exit 101, 2 failures) — both unrelated to Extract Power: Windows path-separator artifacts in committed bounded-offer infra (game/engine.rs); Extract Power's own 3 integration tests passcargo export-cards data --stats --sidecar-dir client/public && cp client/public/card-data.json data/card-data.json— passed (28154 cards regenerated; Extract Power present)cargo coverage— passed (exit 0; Extract Power supported:true gap_count:0)cargo semantic-audit— passed (exit 0; Extract Power 0 findings, 32615 cards audited)Scope Expansion
None.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
Bug Fixes
Tests