fix(engine): support non-first spell triggers - #7233
Conversation
|
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 (1)
📝 WalkthroughWalkthroughThe change adds comparator support to ChangesSpell trigger matching
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleTriggerParser
participant parse_spell_history_filter
participant TriggerConstraint
participant SpellHistory
OracleTriggerParser->>parse_spell_history_filter: Parse shared spell qualifiers
parse_spell_history_filter-->>OracleTriggerParser: Return spell history filter
OracleTriggerParser->>TriggerConstraint: Create NthSpellThisTurn with Comparator::GT
SpellHistory->>TriggerConstraint: Evaluate the caster spell count
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 |
11a9fb9 to
7924729
Compare
Co-authored-by: keloide <75585494+keloide@users.noreply.github.com>
|
Maintainer fixup pushed at |
|
Generated for head Parse changes introduced by this PR · 175 card(s), 21 signature(s) (baseline: main
|
|
Rechecked current head Manual review is clean: the generalized comparator stays at the existing per-caster The parse-diff sticky (updated HOLD: no approval or enqueue while the current Rust, frontend/WASM, AI, and required external checks are pending. No contributor action is requested. |
![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/game/coverage.rs`:
- Around line 4176-4180: Update the match on comparator in the timing
construction to replace the wildcard arm with explicit remaining Comparator
variants, preserving the current formatting behavior for each variant. Keep
Comparator::EQ and the Comparator::GT special case unchanged, and make the match
exhaustive so future enum additions require an explicit formatting decision.
- Around line 4177-4183: Update the Comparator::EQ formatting branch in the
timing description to use correct ordinal suffixes: 1st, 2nd, 3rd, default th,
and th for 11th–13th. Preserve the existing wording and target handling in the
surrounding filter match.
🪄 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: bac59f0e-3f64-44c9-a8eb-b0558ceb65d3
📒 Files selected for processing (12)
crates/engine/src/game/coverage.rscrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/ability.rscrates/engine/tests/integration/curse_misc_triggers.rscrates/engine/tests/integration/ichneumon_druid.rscrates/engine/tests/integration/lady_loki_agent_of_chaos.rscrates/engine/tests/integration/main.rscrates/mtgish-import/src/convert/trigger.rscrates/phase-ai/src/features/spellslinger_prowess.rs
|
Maintainer fixup pushed at HOLD: this exact head is rebuilding CI, the exact-head parse-diff artifact, and CodeRabbit's independent review. No contributor action is requested; the next sweep will recheck those external gates before approval or enqueue. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the new opponent-trigger class is not team-correct.
🔴 Blocker
[HIGH] Opponent-scoped spell triggers treat a teammate as an opponent in team games. Evidence: the new parser writes TypedFilter::controller(ControllerRef::Opponent) for the Ichneumon Druid form at crates/engine/src/parser/oracle_trigger.rs:16046-16048; match_spell_cast sends the caster through valid_player_matches at crates/engine/src/game/trigger_matchers.rs:1733; and its Typed(ControllerRef::Opponent) arm compares trigger_controller != player_id at crates/engine/src/game/trigger_matchers.rs:691-694. Verified CR 102.3 says, “a player’s teammates are the other players on their team, and the player’s opponents are all players not on their team.” Why it matters: in 2HG, the source controller's teammate casting their second instant satisfies this new an opponent casts … other than the first … trigger and incorrectly takes the 4 damage. The added runtime test is only two-player (crates/engine/tests/integration/ichneumon_druid.rs:16-87), so it cannot discriminate that failure. Suggested fix: make the shared player_matches_filter opponent arms use the established crate::game::players::is_opponent(state, trigger_controller, player_id) authority (already used by the sibling generic matcher in crates/engine/src/game/filter.rs:7018-7022), then add a 2HG regression proving a teammate's later instant does not trigger while an opposing team's later instant does.
Recommendation: request changes. Please make the shared opponent relation team-aware and add the discriminating 2HG runtime coverage; then the non-first trigger can be re-reviewed on its new head.
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/game/trigger_matchers.rs`:
- Around line 686-696: Annotate the team-opponent matching branches in the
trigger filter logic with the locally verified Comprehensive Rules citation,
using the format “CR <number>: <description>” and explicitly stating that
teammates are non-opponents and players::is_opponent supplies this relation.
Apply the annotation near TargetFilter::Opponent and the typed
ControllerRef::Opponent handling without changing their behavior.
🪄 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: 861e5dcb-2f9c-470a-89b7-825ecbc5f937
📒 Files selected for processing (3)
crates/engine/src/game/coverage.rscrates/engine/src/game/trigger_matchers.rscrates/engine/tests/integration/ichneumon_druid.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/src/game/coverage.rs
|
Maintainer fixup pushed at HOLD: this exact head is awaiting its new CI run, current-head parse-diff artifact, and CodeRabbit pass. No contributor action is requested; the next sweep will approve/enqueue only after those external checks settle cleanly. |
|
Correction to the prior hold: the exact current head is |
|
Rechecked current head The exact-head parse-diff is now published, all CI and platform checks other than the two AI-gate jobs are green, and CodeRabbit's current-head pass reports no actionable comments. The prior HOLD: only |
|
Rechecked and updated current head The prior head completed cleanly, but this new exact head is now rebuilding CI, the parse-diff artifact, the two AI gates, and CodeRabbit. HOLD: no approval or enqueue until those current-head external gates settle cleanly. No contributor action is requested; the next sweep will recheck this head. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current head 8e0efb41b225b87cb66dbdc5d8de5f7b6ff17387 is ready for the merge queue.
The comparator extends the existing per-caster NthSpellThisTurn fire-time authority, keeps legacy exact-ordinal serialization, and uses the shared team-aware opponent relation. The current-head parse artifact is published; required CI and AI gates are green. The 2HG scenario distinguishes teammate and opposing-player casts, and the current CodeRabbit pass has no actionable findings.
2 participants
Summary
Implements the reusable fire-time “other than the first [qualifying] spell” trigger class and uses it for Ichneumon Druid. Existing exact-Nth spell triggers remain
EQ; malformed or partially supported text fails closed instead of silently broadening.Files changed
crates/engine/src/types/ability.rs— parameterizesNthSpellThisTurnwith a backward-compatible comparator.crates/engine/src/parser/oracle_trigger.rs— parses the non-first spell family and strict-rejects incomplete tails.crates/engine/src/game/triggers.rs— evaluates the comparator at trigger fire time.crates/engine/src/game/coverage.rs— accurately renders non-first constraints.crates/engine/src/parser/oracle_trigger_tests.rs— parser, strict-failure, serde, and compatibility coverage.crates/engine/tests/integration/ichneumon_druid.rsandmain.rs— real casting-flow regression.crates/engine/tests/integration/curse_misc_triggers.rs— removes a noncanonical partial-Aura fixture.crates/engine/src/game/engine_phase_trigger_regression_tests.rsandtests/integration/lady_loki_agent_of_chaos.rs— explicit/wildcard constraint audit.crates/mtgish-import/src/convert/trigger.rsandcrates/phase-ai/src/features/spellslinger_prowess.rs— cross-crateEQconstructor and timing documentation updates.Track
Non-developer
LLM
Model: gpt-5.6-terra
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
./scripts/check-parser-combinators.sh— passed: Gate G PASS and Gate A PASS.git diff --check upstream/main...HEAD— passed.rustfmt --check --edition 2021 <all modified Rust files>— passed.cargo fmt --all, Cargo tests/checks, card-data generation, and coverage report — CI-owned: installed stable Cargo rejects the workspace's nightly-onlycodegen-backendmanifest feature;rustupand Tilt are unavailable locally.Final independent
/review-impl— CLEAN / LGTM for the current rebased head.Gate A
Gate A PASS head=7924729f8e7d8514deb47fe173878f886a1a79d7 base=448e9867fb82c26cc1e15ce40fed6c40ec4fe427
Anchored on
crates/engine/src/parser/oracle_trigger.rs:15894— existing per-caster Nth-spell parser and constraint seam.crates/engine/src/game/triggers.rs:9539— existing SpellCast event-controller/per-caster history matcher.Final review-impl
Final review-impl PASS head=7924729f8e7d8514deb47fe173878f886a1a79d7
Claimed parse impact
GT 1instant-spell trigger.or copies a spellevent leg; this avoids a false-green partial parse.Scope Expansion
None. The shared constraint parameterization is required to implement the selected card's reusable spell-history class. No protected architecture path or cross-span boundary is touched.
Validation Failures
Local Rust compilation and tests could not run: stable Cargo rejects the workspace's nightly-only
codegen-backendmanifest feature, and neitherrustupnor Tilt is installed. CI must run the Cargo, integration, card-data, and coverage checks.CI Failures
None.
Summary by CodeRabbit