fix(parser): count opponents, not returned creatures, for Faerie Slumber Party (#6943) - #6961
Conversation
|
Warning Review limit reached
Next review available in: 29 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 (14)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
…ber Party (#6943) Faerie Slumber Party created 18 tokens where 6 were correct. "Return all creatures to their owners' hands. For each opponent who controlled a creature returned this way, you create two 1/1 blue Faerie creature tokens..." -- the token count is driven by the number of DISTINCT OPPONENTS who controlled at least one returned creature, and the returned set is only the membership test. The parser dropped the "opponent who controlled" wrapper entirely. The emitted AST carried `repeat_for: Ref(TrackedSetSize)` -- the size of the returned set -- with no player-count node anywhere on the card. The resolver was correct; it did exactly what the AST said. 9 creatures returned x 2 tokens = 18. Add a `PlayerFilter::TrackedSetPossessor` shape so the count resolves over players who possess a member of the tracked set, and parse the "for each <player-filter> who <verb> ... this way" grammar with nom combinators composed from the existing oracle_nom helpers. The dangerous half of this change is the registration, not the parse. `quantity_expr_references_tracked_set` gates whether the PRODUCER publishes the tracked set at all, and it was a `matches!` allowlist -- a hand-maintained registry the compiler cannot census. Retargeting `repeat_for` to a shape absent from that allowlist would have de-registered the card as a consumer, so `BounceAll` would never publish, every membership test would fail, and the result would be 0 tokens rather than 6 -- with no compile error. That is the shape of the Seasoned Pyromancer bug (#740). So rather than adding one arm to the allowlist, extract `player_filter_references_tracked_set` as an exhaustive, wildcard-free match. A new `PlayerFilter` variant now cannot compile until it declares whether it reads the gated ledger. Adding a variant to the `false` group becomes a decision rather than an accident. 0 tokens is the de-registration signature and collides with the legitimate empty-board result, so the tests are built to distinguish them: T1 was observed RED at exactly 18 at base -- proving the tracked set genuinely publishes there and that 18 and 0 are separable outcomes -- then GREEN at 6. Removing only the allowlist entry reproduces the predicted 0, and a unit-layer test catches that directly. Verified end to end rather than at AST level: the card-data pipeline was rerun and the regenerated data carries the new shape. Parser-side, so it stays inert in the app until card data is regenerated and redeployed. Deferred and filed: a `PlayerFilter` in SCOPE position is still classified by a `matches!` predicate that silently detaches scoped continuations (#6957). This variant lands in the COUNT position, so that site is benign here.
02ebbd3 to
1844c91
Compare
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
…itted The CR 603.5 producer census pins `game/effects/mod.rs` line-exact. The phase-rs#6961 + v0.44.0 uniform +78 shift moved the three producers to :5996/:6073/:9048, and that re-pin was edited during the 851180c fold but never committed — it stayed in the working tree, so the pushed tip 0b5a2bf still asserted :5918/:5995/:8970 and CI went red on it. This commit is that edit and nothing else. Why the local gate was green while CI was red: the census test walks `src/` from disk at runtime (`env!("CARGO_MANIFEST_DIR")` + `read_to_string`), so it measures the WORKING TREE, never the commit. A dirty tree can never certify a commit — "green at the exact tip" requires HEAD == sha AND a clean `git status` on the gated paths, and that precondition is now part of the drift-log instructions. Re-verified in the `refs/pull/<n>/merge` layout rather than locally, per the drift log's own rule: `git merge-tree --write-tree HEAD upstream/main` (upstream 1738d5c) puts the producers at exactly :5996/:6073/:9048, with `scoped_library_search.rs:452` and `engine.rs:11427` unmoved — all five production producers accounted for, still no sixth. Upstream phase-rs#6959 is innocent. Assisted-by: ClaudeCode:claude-opus-5
Fixes #6943.
The bug
Faerie Slumber Party created 18 tokens where 6 were correct.
The token count is driven by the number of distinct opponents who controlled at least one returned creature. The returned set is only the membership test.
The parser dropped the
"opponent who controlled"wrapper entirely. The emitted AST carriedrepeat_for: Ref(TrackedSetSize)— the size of the returned set — with no player-count node anywhere on the card. The resolver was correct; it did exactly what the AST told it to. 9 creatures returned × 2 tokens = 18.The fix
Add a
PlayerFilter::TrackedSetPossessorshape so the count resolves over players possessing a member of the tracked set, and parse the"for each <player-filter> who <verb> … this way"grammar with nom combinators composed from the existingoracle_nomhelpers.The dangerous half was the registration, not the parse
quantity_expr_references_tracked_setgates whether the producer publishes the tracked set at all — and it was amatches!allowlist, i.e. a hand-maintained registry the compiler cannot census.Retargeting
repeat_forto a shape absent from that allowlist would have de-registered the card as a consumer:BounceAllwould never publish, every membership test would fail, and the result would be 0 tokens rather than 6 — with no compile error. That is the shape of the Seasoned Pyromancer bug (#740), whose comment already sits in that file.So rather than adding one arm to the allowlist, this extracts
player_filter_references_tracked_setas an exhaustive, wildcard-free match:A new
PlayerFiltervariant now cannot compile until it declares whether it reads the gated ledger. Adding a variant to thefalsegroup becomes a decision rather than an accident.Tests are built so 0 and 6 are distinguishable
0is the de-registration signature and the legitimate result of an empty-board hypothesis, so a test asserting "not 18" or "is 0" cannot tell the fix from the regression.1 × 2coincide with the correct1 × 2. All 5 runtime fixtures now fail at base.Verification
clippy,test-engine, andcard-dataall green and fresh.card-data.jsoncarriesPlayerCount/TrackedSetPossessor. That output also confirmed the serialization shape empirically —"relation":{"type":"Opponent"}internally tagged,"possession":"Controller"bare — rather than from the plan's hand-written JSON, which had it wrong.matches!/_ => falsepredicates) were opened and classified individually.Inertness: parser-side, so this stays invisible in the app until card data is regenerated and redeployed.
Deferred and filed
A
PlayerFilterin scope position is still classified by amatches!predicate whosebooldrives a chain-detachment decision — filed as #6957. This variant lands in the count position, so that site is benign here, and the plan states the condition rather than calling it flatly benign.