Partial: Cut a Deal - #7018
Conversation
github-actions
Bot
added
the
needs-maintainer
|
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 selected for processing (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThe engine records successful draw instructions as player action events. Oracle parsing recognizes draw-based “this way” clauses. Scoped continuations resolve outside unrelated fan-outs. Integration tests cover Cut a Deal and Kwain behavior. ChangesDraw provenance and this-way effects
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant EffectResolver
participant ActionLedger
participant ScopedEffect
OracleText->>EffectResolver: parse PerformedActionThisWay Draw filter
EffectResolver->>ActionLedger: resolve draw instruction
ActionLedger-->>ScopedEffect: provide players with recorded Draw actions
ScopedEffect-->>EffectResolver: apply dependent effect once to matching players
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: 3
🤖 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/lower.rs`:
- Around line 5106-5110: Make the PlayerFilter-to-PlayerRelation derivation in
the surrounding helper exhaustive by handling every PlayerFilter variant
explicitly rather than relying on the wildcard arm. Preserve the existing
Opponent and All mappings, and explicitly return None or apply the intended
behavior for each remaining variant before constructing the relative clause.
In `@crates/engine/src/parser/oracle_quantity.rs`:
- Around line 2720-2723: Update parse_drew_arm to accept the plural verb “draw”
in addition to the existing alternatives, placing it after the longer verb tags
to preserve parsing precedence. Add a regression test covering plural players
who “draw a card” and verifying it lowers to PerformedActionThisWay with
PlayerActionKind::Draw.
In `@crates/engine/src/types/events.rs`:
- Around line 148-157: The unsuccessful-draw annotations use an incorrect CR
608.2c citation. In crates/engine/src/types/events.rs lines 148-157 and
crates/engine/src/game/effects/draw.rs lines 449-460, replace that citation with
CR 121.1 for the draw definition and CR 121.4 for empty-library draw attempts;
retain CR 608.2c only for documentation of written instruction ordering.
🪄 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: 33b5869f-ff0c-47ce-82a2-5cf303dad8bb
📒 Files selected for processing (9)
crates/engine/src/game/effects/draw.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/log.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_quantity.rscrates/engine/src/types/events.rscrates/engine/tests/integration/cut_a_deal_draw_this_way_count.rscrates/engine/tests/integration/kwain_drew_this_way_gains_life.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PRBaseline pending for |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the parser misses a valid plural form, the draw annotations cite the wrong rule, and the current head has a required CI failure.
🔴 Blocker
[MED] parse_drew_arm does not accept the plural present-tense draw phrase. Evidence: crates/engine/src/parser/oracle_quantity.rs:2658-2663 accepts players , while :2720-2723 permits only draws and drew; therefore players who draw a card this way cannot produce PlayerFilter::PerformedActionThisWay { relation: All, action: Draw }. Why it matters: valid player-scoped action-count text is left unsupported despite the new shared action-tail authority. Suggested fix: add draw after the existing longer tags and add a registered parser/runtime regression that exercises the plural population and verb through the production lowering path.
[MED] The draw/no-draw documentation attributes behavior to CR 608.2c, which governs instruction order rather than whether a card was drawn. Evidence: crates/engine/src/types/events.rs:155-157 and crates/engine/src/game/effects/draw.rs:449-460; the repository rules text at docs/MagicCompRules.txt:1142-1160 defines drawing in CR 121.1 and the empty-library attempt in CR 121.4, whereas :2793 says CR 608.2c concerns written-order resolution. Why it matters: rules annotations are authoritative maintenance evidence and currently state a rule that does not support the claim. Suggested fix: revise the no-card/no-draw claims to CR 121.1/121.4; retain CR 608.2c only where the comment actually concerns ordering.
[MED] Required Rust tests shard 2 is red on this exact head because the PR shifted the pinned producer census without updating the intentional coordinates. Evidence: run 30976688653, crates/engine/src/game/engine.rs:15178: expected effects/mod.rs:6065/6142/9324, actual 6081/6158/9340; this PR adds 18 lines in crates/engine/src/game/effects/mod.rs:3569-3606. Why it matters: required CI is not passing and the census can no longer detect the intended producer set at its asserted locations. Suggested fix: after the substantive corrections/rebase, reconcile the assertion with the current producer coordinates rather than treating this as an external-agent failure.
✅ Clean
The current parse-diff receipt is bound to 6f27da93a4969ffd2702498d77a02d347e99bce7 and identifies one intended changed signature for Cut a Deal; it does not resolve the findings above.
Recommendation: address the three items on a new head, then request re-review.
matthewevans
removed
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head does not compile because both new regression tests call a three-argument helper with only two arguments.
[HIGH] resolve_log_entries requires the event slice plus both the pre- and post-resolution game states, but the new Cut a Deal tests omit the latter two arguments. Evidence: crates/engine/src/game/log.rs:14-18 declares resolve_log_entries(events, before, after), while crates/engine/tests/integration/cut_a_deal_draw_this_way_count.rs:1842 and :1857 call resolve_log_entries(&events, &after). Exact-head Rust lint/tests report E0061. Why it matters: the required CI cannot compile the engine test target, so the added behavior has no executable regression coverage. Suggested fix: retain the captured pre-resolution state and pass it with the post-resolution state in both calls: resolve_log_entries(&events, &before, &after).
The current parse-diff receipt is bound to 3a7e35d9268755cdadd322d443ad6581aeff0698 and is limited to the intended Cut a Deal signature; I found no additional current-head blocker.
|
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. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/game/log.rs (1)
1834-1835: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRemove the incorrect CR basis from the logging assertion.
CR 121.1defines drawing a card. It does not define whetherPlayerPerformedAction::Drawappears in a user-facing log. This test enforces an application-level visibility contract. Remove the citation, or replace it with a verified rule only if the implementation enforces that rule. (media.wizards.com)Proposed comment update
- // CR 121.1: the Draw ledger signal must not reach the visible log — + // Draw is a ledger-only signal and must not reach the visible log.As per path instructions,
crates/engine/**requires each CR citation to describe the implemented rule.🤖 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/log.rs` around lines 1834 - 1835, Remove the “CR 121.1” citation from the assertion comment near the Draw ledger visibility test, since the assertion enforces an application-level user-facing logging contract rather than a Comprehensive Rules requirement. Retain only a concise description of the visibility behavior, unless a verified rule governing that behavior is established.Sources: Path instructions, MCP tools
🤖 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/game/engine.rs`:
- Around line 15961-15963: Normalize the relative path variable rel to forward
slashes before formatting the producer strings compared against the listed
census path literals. Update the surrounding producer-generation logic in the
relevant engine code, preserving the existing line-number formatting and literal
expectations.
---
Nitpick comments:
In `@crates/engine/src/game/log.rs`:
- Around line 1834-1835: Remove the “CR 121.1” citation from the assertion
comment near the Draw ledger visibility test, since the assertion enforces an
application-level user-facing logging contract rather than a Comprehensive Rules
requirement. Retain only a concise description of the visibility behavior,
unless a verified rule governing that behavior is established.
🪄 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: b7bd665d-d16f-4453-be0d-17deb5eb66ae
📒 Files selected for processing (10)
crates/engine/src/game/effects/draw.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/engine.rscrates/engine/src/game/log.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_quantity.rscrates/engine/src/types/events.rscrates/engine/tests/integration/cut_a_deal_draw_this_way_count.rscrates/engine/tests/integration/kwain_drew_this_way_gains_life.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/engine/tests/integration/main.rs
- crates/engine/src/types/events.rs
- crates/engine/tests/integration/cut_a_deal_draw_this_way_count.rs
- crates/engine/tests/integration/kwain_drew_this_way_gains_life.rs
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/parser/oracle_effect/lower.rs
- crates/engine/src/game/effects/draw.rs
- crates/engine/src/parser/oracle_quantity.rs
|
Maintainer hold at Fresh implementation review and the approval/merge-queue decision will resume once the exact-head CI jobs are terminal and the |
# Conflicts: # crates/engine/src/game/engine.rs
|
Maintainer CI-evidence hold at Fresh implementation review and the approval/merge-queue decision will resume only after those exact-head checks settle. This is not an approval or enqueue. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after a fresh current-head review.
- Exact-head CI is green, including Rust lint, both Rust test shards, card-data/coverage, and the consolidated Rust gate.
- The parser receipt is in the permitted
baseline pending after maintainer mergestate for base07845973a8fa4eb6641dc5fa09dd7a117bf6e490; the preceding exact card-level receipt showed only Cut a Deal's intended draw-count signature. - The maintainer fixes preserve hidden
CardDrawndiagnostics, hide only the internal draw ledger signal, restore the discriminating regression, and retain the cross-platform census invariant. Current external review has no actionable findings and every live thread is resolved.
2 participants
Summary
Fixes a parse-fidelity defect on Cut a Deal.
Issue: Second draw's count "for each opponent who drew a card this way" parsed to QuantityRef::TrackedSetSize, but the preceding opponent-scoped Draw publishes no tracked set (Draw is not a tracked-set producer), so it resolves to 0/stale instead of counting opponents who drew; correct parse is PlayerCount{PerformedActionThisWay(Draw)}.
Files changed
CR references
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Tier: Frontier
Verification
cargo fmt --all— clean (exit 0)./scripts/check-parser-combinators.sh (Gate A)— clean — Gate G PASS + Gate A PASS (exit 0) when run with the real msys64 python3; the WindowsApps python3 stub causes a spurious exit-1 at the Family-D self-test, an env limitation the task pre-authorized, not a code failurecargo clippy-strict— clean (exit 0)cargo test -p phase-engine— FAILED (exit 101): 18509 passed, 1 failed, 6 ignored — game::engine::stage2_injector_tests::the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_eventcargo export-cards data --stats --sidecar-dir client/public --output client/public/card-data.json && cp client/public/card-data.json data/card-data.json— clean (exit 0) — fresh card-data.json regenerated to both client/public and data (91.9% implemented). Added --output because the literal recipe sends the main export to stdout and would leave the sidecar file stalecargo coverage— clean (exit 0) — Cut a Deal supported:true gap_count:0cargo semantic-audit— clean (exit 0) — Cut a Deal absent from flagged_cards (0 findings)Scope Expansion
None. (Deferred data/card-data.json regeneration + cargo semantic-audit to the measurement/orchestrator phase; additive variant → clean single-card parse diff expected. Parser Family-D combinator sub-gate unrunnable due to an environmental python3 stub; Gates A-G + manual grep clean.)
Validation Failures
See review/cross-check notes.
CI Failures
WaitingFor::OptionalEffectChoice {and asserts exact producer line numbers — it runs no card. Root cause: another agent's concurrent UNCOMMITTED work in crates/engine/src/game/effects/mod.rs (1215 insertions; the file was NOT in this session's initial git-status snapshot) shifted the three producer sites by +16 (committed HEAD = :6065/:6142/:9324 which exactly matches the pinned expectations; the on-disk working tree drifted to :6081/:6158/:9340). Additionally confounded by a Windows-only path-separator artifact: the runtime paths render asgame\effects\mod.rswhile the test pins are hardcoded forward-slashgame/effects/mod.rs, so this census cannot pass on any Windows checkout regardless of code. NOT FIXED by design: CLAUDE.md multi-agent safety forbids editing another agent's in-progress effects/mod.rs or the pins tracking it (the test's own DRIFT LOG assigns pin reconciliation to the agent that owns the insertion). Deterministic on re-run; the rest of the suite is green.Summary by CodeRabbit
New Features
Bug Fixes
Tests