Fix Black Bolt, Inhuman King - #7011
Conversation
📝 WalkthroughWalkthroughThe PR normalizes relative census paths to forward slashes. It records the source controller in ChangesCensus path normalization
Becomes-target trigger resolution
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Casting
participant GameEvent
participant TriggerParser
participant TriggeredAbility
participant Battlefield
Casting->>GameEvent: emit BecomesTarget with source_controller
GameEvent->>TriggerParser: match becomes-target condition
TriggerParser->>TriggeredAbility: resolve "that player" as TriggeringPlayer
TriggeredAbility->>Battlefield: select and destroy opponent permanent
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.45.0)crates/engine/src/game/casting_tests.rsast-grep timed out on this file crates/engine/src/game/triggers.rsast-grep timed out on this file 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 |
|
Generated for head Parse changes introduced by this PR · 2 card(s), 1 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Snapshot the targeting spell or ability's controller in GameEvent::BecomesTarget. Evidence: crates/engine/src/parser/oracle_trigger.rs:1045-1055,1219-1227 maps the targeter to ControllerRef::TriggeringPlayer, while crates/engine/src/game/targeting.rs:1662-1665 later derives that player from the current state.objects[source_id].controller; the event emitted at crates/engine/src/game/casting.rs:621-636 carries only source_id despite already receiving controller. Why it matters: if an activated ability targets Black Bolt and its source changes controllers before Lethal Voice resolves, the trigger can select the new controller's permanent instead of the player who activated the targeting ability. CR 602.2a and CR 115.1c (verified in docs/MagicCompRules.txt) establish that the activated ability's controller and targets are set during activation. Suggested fix: capture the controller in GameEvent::BecomesTarget at emit_targeting_events, resolve this anaphor from that snapshot, and add a runtime regression test for an activated ability followed by source control change; the existing test covers only a spell without control change.
|
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
🤖 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/tests/integration/black_bolt_lethal_voice_destroys_triggering_opponents_permanent.rs`:
- Around line 1-18: Correct the rules annotation in the test documentation:
replace the CR 115.1 attribution for spell target declaration and
source-controller provenance with CR 601.2c for target selection during casting
and CR 405.4 for the spell controller being its caster. Ensure every cited CR
directly describes the annotated behavior, while preserving the test’s existing
rules explanation and pipeline scenario.
🪄 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: fea38331-ad82-4e7d-887d-74457e928a1b
⛔ Files ignored due to path filters (1)
crates/engine/src/parser/oracle_ir/snapshots/engine__parser__oracle_ir__snapshot_tests__bonecrusher_giant_ir.snapis excluded by!**/*.snap,!**/snapshots/**
📒 Files selected for processing (13)
crates/engine/src/game/casting.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/engine.rscrates/engine/src/game/log.rscrates/engine/src/game/stack.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/src/types/events.rscrates/engine/tests/integration/black_bolt_lethal_voice_destroys_triggering_opponents_permanent.rscrates/engine/tests/integration/main.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/engine/src/game/engine.rs
- crates/engine/tests/integration/main.rs
- crates/engine/src/parser/oracle_trigger.rs
- crates/engine/src/parser/oracle_trigger_tests.rs
| //! CR 115.1 + CR 603.2e + CR 608.2c — Black Bolt, Inhuman King (Lethal Voice). | ||
| //! | ||
| //! Verbatim Oracle text: | ||
| //! Flying | ||
| //! Whenever you cast a noncreature spell, Black Bolt gets +2/+2 until end of turn. | ||
| //! Lethal Voice — Whenever Black Bolt becomes the target of a spell or ability an | ||
| //! opponent controls, destroy target nonland permanent that player controls. | ||
| //! | ||
| //! "That player" is the controller of the *targeting* source — the opponent | ||
| //! (CR 608.2c, "apply the rules of English to the text," reads the effect's "that | ||
| //! player" anaphor as referring back to the player named by the trigger | ||
| //! condition; CR 603.2e fires on the "becomes the target" event; CR 115.1 fixes | ||
| //! the source's target when it is put on the stack). The parser previously | ||
| //! lowered "that player controls" to `ControllerRef::You`, so Lethal Voice | ||
| //! offered/destroyed one of BLACK BOLT'S OWN permanents. This test drives the | ||
| //! real pipeline: an opponent (P1) targets Black Bolt with a spell P1 controls, | ||
| //! and Lethal Voice must be able to destroy a nonland permanent P1 controls — | ||
| //! never one of P0's own. |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win
Correct the CR annotation for target declaration and source-controller provenance.
The statement that CR 115.1 fixes the source's target when it is put on the stack is incorrect. CR 601.2c defines spell target selection during casting. CR 405.4 defines a spell controller as the player who cast it. Update the annotation so it does not attribute announcement-time source_controller preservation to CR 115.1. (blogs.magicjudges.org)
As per path instructions, each CR citation must describe the code it annotates.
🤖 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/tests/integration/black_bolt_lethal_voice_destroys_triggering_opponents_permanent.rs`
around lines 1 - 18, Correct the rules annotation in the test documentation:
replace the CR 115.1 attribution for spell target declaration and
source-controller provenance with CR 601.2c for target selection during casting
and CR 405.4 for the spell controller being its caster. Ensure every cited CR
directly describes the annotated behavior, while preserving the test’s existing
rules explanation and pipeline scenario.
Source: Path instructions
matthewevans
left a comment
There was a problem hiding this comment.
Approved at 11a445f4621725f97b235b8dbb7a799a81261427.
The targeting-source controller is now snapshotted in GameEvent::BecomesTarget, the hostile controller-change integration path covers the prior bug, the exact-head parse-diff is limited to Black Bolt and Scalelord Reckoner, and required checks are green.
matthewevans
left a comment
There was a problem hiding this comment.
Verified current head: BecomesTarget now snapshots the announcement-time controller, with a control-change regression; CI is green.
Summary
Fixes a parse-fidelity defect on Black Bolt, Inhuman King.
Issue: Lethal Voice: destroy target's controller parsed as You instead of the triggering opponent; "that player controls" (the opponent controlling the spell/ability) anaphor defaulted to ControllerRef::You, so it targets your own nonland permanent rather than the opponent's.
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— clean - Gate G PASS + Gate A PASS. WindowsApps python3 stub falsely hard-fails D0 pre-check; re-ran with working msys64 python3 on PATH so Family D actually ran and passed (10/10 detector tests). Not merely skipped.cargo clippy-strict— clean (exit 0, -D warnings); re-verified after the census-test editcargo test -p phase-engine— clean (exit 0) after in-loop fix: lib 18483 passed/0 failed, integration 4489 passed/0 failed. Fixed a Windows path-separator artifact in the unrelated CR-603.5 census pin test (test-only .replace('\','/') in game/engine.rs; no-op on Linux CI, pins untouched).cargo export-cards data --stats --output client/public/card-data.json --sidecar-dir client/public + cp to data/card-data.json— clean (exit 0). Used --output because the literal recipe (no --output) streams main export to stdout and --sidecar-dir writes only localized card-data..json; the cp would have copied the stale sidecar. Fresh card-data.json regenerated against this branch's engine.cargo coverage— clean (exit 0); Black Bolt, Inhuman King supported:true gap_count:0cargo semantic-audit— clean (exit 0); Black Bolt, Inhuman King absent from all 295 flagged_cards -> 0 findingsScope Expansion
Also updated the Bonecrusher Giant IR snapshot (metadata-only relative_player_scope, another becomes-target trigger; effect unchanged); card-data.json not hand-regenerated (gitignored, Tilt-owned).
Validation Failures
None.
CI Failures
None.
Summary by CodeRabbit
Bug Fixes
Tests