Partial: Doom's Time Platform - #7022
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Warning Review limit reached
Next review available in: 5 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)
📝 WalkthroughWalkthroughExile parsing now separates trailing counter clauses from descriptive targets in counterless zones. The parser applies those counters when cards enter exile and preserves battlefield counter filters. Parser and integration tests cover both behaviors. ChangesExile counter parsing
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Combat
participant Doom's Time Platform
participant Graveyard
participant Exile zone
Combat->>Doom's Time Platform: resolve attack trigger
Doom's Time Platform->>Graveyard: target a nonland card
Doom's Time Platform->>Exile zone: exile the card with two time counters
Possibly related PRs
Suggested labels: 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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/tests.rs`:
- Around line 35589-35664: Complete the regression matrix around the existing
exile counter tests, covering graveyard, hand, library, battlefield, and a
non-counter “with …” clause. In each origin-sensitive case, assert the parsed
target preserves the expected origin via origin or FilterProp::InZone, while
confirming only graveyard/hand/library lift counters to enter_with_counters and
battlefield retains FilterProp::Counters. Use the existing parse_effect_chain
and target pattern in the named tests.
🪄 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: f4dbb2a0-05a7-42fd-beb8-f57e4fa53cb4
📒 Files selected for processing (5)
crates/engine/src/parser/oracle_effect/imperative.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/tests/integration/doom_s_time_platform_exile_with_time_counters.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 3 card(s), 5 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the implementation is a three-card parser-class change, but the public scope and regression evidence still describe only Doom's Time Platform.
🟡 Required before merge
-
Scope/impact disclosure.
crates/engine/src/parser/oracle_effect/mod.rs:34513-34527lifts the rider for every inferredGraveyard | Hand | Libraryorigin, not only Doom's Time Platform. The current-head parse-diff artifact forb4d57fe4ec336db44b8fc89fb720174bb8441c0fmeasures five modified signatures across Doom's Time Platform, Altaïr Ibn-La'Ahad, and The Animus. Please update the PR body to state this exact three-card impact/class (or constrain the implementation to the intended scope) so review and future regressions are honest. -
Complete the origin regression matrix.
crates/engine/src/parser/oracle_effect/tests.rs:35589-35664exercises library lifting but does not assert itsorigin/InZonepreservation; it has no Hand case and no non-counter-rider case. Add parser regressions for graveyard, hand, library, battlefield, and a non-counterwith …clause. Each origin-sensitive case should assert the retained origin semantics; only graveyard/hand/library should lift counters, while battlefield and non-counter clauses must remain their respective filter/unparsed forms. This confirms the parser's new class-level gate rather than just the Doom fixture.
CodeRabbit independently raised the test-matrix gap on this head's predecessor; it remains present at the current head: #7022 (comment).
Recommendation: update the stated impact and add the complete regression matrix, then request re-review on the new head.
matthewevans
removed
the
needs-maintainer
|
Note GitHub couldn't provide a complete incremental comparison for this pull request, so CodeRabbit is performing a full review instead. This review may take a little longer. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the counterless-entry splitter drops a rules-bearing counter filter from persistent graveyard targets, and the parse-diff artifact is not bound to this head.
[HIGH] split_counterless_enter_counters treats every graveyard, hand, and library phrase as a counterless-entry context and strips the trailing with counters filter. Evidence: crates/engine/src/parser/oracle_effect/mod.rs:34669-34708 includes Graveyard in its counterless set before removing the phrase. That is not generally sound: CR 122.1a (docs/MagicCompRules.txt:1180) expressly permits a creature card in a non-battlefield zone to have counters. Why it matters: an instruction that targets a persistent graveyard card with counters loses FilterProp::Counters, widening the target set and producing rules-incorrect selection. Suggested fix: preserve the counter filter for graveyard (and other persistent zone) targets unless the surrounding grammar positively establishes an entry-with-counters rider; add a regression covering a graveyard card target whose counter predicate survives.
[MED] Parser-surface evidence is stale. Evidence: the only parse-diff receipt is bound to b4d57…, not current head 658fe3fa0313a1740c79453a8f7c79a009813133. Why it matters: the review cannot establish the current parser blast radius. Suggested fix: push/run CI until the <!-- coverage-parse-diff --> artifact names this exact head, then reconcile every resulting card/signature change.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — this revision still conflates a persistent-zone counter predicate with an entry-with-counters rider, and its regression no longer proves that non-counter qualifiers survive.
[HIGH] split_counterless_enter_counters strips the trailing counter predicate whenever the inferred origin is Graveyard | Hand | Library (crates/engine/src/parser/oracle_effect/mod.rs:34669-34708). Graveyard is not generically counterless: CR 122.1a permits a creature card in a non-battlefield zone to have counters. The current parse receipt confirms the resulting target widening for Altaïr Ibn-La'Ahad, Doom's Time Platform, and The Animus. Preserve FilterProp::Counters for persistent-zone target grammar; lift counters into enter_with_counters only when the surrounding syntax positively establishes an entry rider. Add a regression in which a graveyard counter predicate survives, alongside grammar-positive entry-rider coverage.
[MED] The latest test change removes the FilterProp::WithKeyword(Flying) assertion from the shared parser case (crates/engine/src/parser/oracle_effect/tests.rs:35855-35889). That lets the suite pass even if the splitter drops a non-counter with … qualifier. Restore a non-counter qualifier assertion (or an equivalent dedicated regression) so the test discriminates between extracting only counters and discarding the whole suffix.
Please update on a new head with the counter filter preserved for persistent graveyard targets and the non-counter regression restored; then request re-review.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verified current head: the parser regression matrix now covers the required origin and non-counter cases; CI is green.
2 participants
Summary
Fixes a parse-fidelity defect on Doom's Time Platform.
Issue: "with two time counters on it" should place two time counters on the exiled card (enter_with_counters), but is parsed as a target-filter requiring the card to already have >=2 time counters, dropping the counter placement.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— pass (exit 0, clean)./scripts/check-parser-combinators.sh (Gate A)— pass (Gate G PASS + Gate A PASS, exit 0; ran with real python3 at /c/msys64/mingw64/bin/python3 by removing the WindowsApps stub from PATH, so Family-D genuinely ran rather than skipped)cargo clippy-strict— pass (exit 0, clean)cargo test -p phase-engine— fail (exit 101; 18516 passed, 1 failed): game::engine::stage2_injector_tests::the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_event at engine.rs:15178 - deterministic Windows-only path-separator artifact, unrelated to the card, in a shared file not part of card work; not modified per multi-agent safetycargo export-cards data --output client/public/card-data.json --stats --sidecar-dir client/public && cp -> data/card-data.json— pass (exit 0; corrected recipe in-loop by adding --output since the literal recipe dumps to stdout and would copy a stale file; fresh 98142340-byte file regenerated from this branch's engine)cargo coverage— pass (exit 0; Doom's Time Platform supported:true gap_count:0)cargo semantic-audit data— pass (exit 0; Doom's Time Platform present in audited data, 0 findings)Scope Expansion
None.
Validation Failures
See review/cross-check notes.
CI Failures
Summary by CodeRabbit
Bug Fixes
Tests
Parse-diff review
Current parser artifact measured 3 affected cards / 5 signatures: Doom’s Time Platform, Altaïr Ibn-La’Ahad, and The Animus. The shared pre-target
with <N> <counter>rider is the intended class; the regression matrix covers library, hand, battlefield, and non-counter siblings.