test(engine): lock Bloodthirsty Blade opponent-equip goad (#5973) - #6605
test(engine): lock Bloodthirsty Blade opponent-equip goad (#5973)#6605claytonlin1110 wants to merge 1 commit into
Conversation
…e-rs#5973) The Discord report was a combat softlock when the Blade goaded an opponent's creature in two-player — the only legal attack target is the goading player, so the away-from clause is unsatisfiable. That class was fixed by the CR 508.1d requirement solver (phase-rs#6164); these regression tests pin parse shape, activate-attach, pump, must-attack, and 2p/3p attack legality for the card. Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds integration coverage for Bloodthirsty Blade’s oracle parsing, opponent-creature attachment, +2/+0 modification, goad attack constraints, and activated ability resolution across two- and three-player scenarios. ChangesBloodthirsty Blade regression coverage
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Review: test coverage is substantive at 60b34e06bc17fd4e9f06553c83fdc98ebfa48df1
No code findings.
- The inline Oracle text matches the current Scryfall printing, and no existing Bloodthirsty Blade regression test covers this opponent-controlled host.
- The 2-player pair reaches
validate_attack_declaration, the CR 508.1d maximum-requirement authority: attacking the sole possible target (the goader) is legal, while declaring no attacker fails. Either a missing generic goad requirement or an over-strict away-from requirement makes one of those assertions fail. - The 3-player pair is a source-attribution discriminator: P0 (the Blade controller) must be rejected and P2 accepted. It exercises the actual
StaticMode::Goadedscan, whose goading player is the static carrier controller. - The activation test uses the normal attach resolver; the parse-shape and layer assertions cover the former double-graft failure surface without relying only on direct state setup.
CI is not clear yet on this exact head: Rust lint, both Rust test shards, and CodeRabbit are still pending. I am recording this as a clean implementation review only; it is not an approval or merge-queue action.
![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
🧹 Nitpick comments (1)
crates/engine/tests/integration/bloodthirsty_blade_goad_5973.rs (1)
55-91: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDuplicate scenario builders — parameterize instead.
setup_two_playerandsetup_three_playerare identical except for theGameScenario::new()vs.GameScenario::new_n_player(3, 42)call. Consider collapsing into one helper taking the scenario constructor or player count as a parameter to avoid future drift between the two copies.♻️ Proposed consolidation
-fn setup_two_player() -> (engine::game::scenario::GameRunner, ObjectId, ObjectId) { - let mut scenario = GameScenario::new(); - scenario.at_phase(Phase::DeclareAttackers); - ... -} - -fn setup_three_player() -> (engine::game::scenario::GameRunner, ObjectId, ObjectId) { - let mut scenario = GameScenario::new_n_player(3, 42); - scenario.at_phase(Phase::DeclareAttackers); - ... -} +fn setup(mut scenario: GameScenario) -> (engine::game::scenario::GameRunner, ObjectId, ObjectId) { + scenario.at_phase(Phase::DeclareAttackers); + let host = scenario.add_creature(P1, "Opponent Bear", 2, 2).id(); + let blade = scenario + .add_creature(P0, "Bloodthirsty Blade", 0, 0) + .as_artifact() + .with_subtypes(vec!["Equipment"]) + .from_oracle_text(BLOODTHIRSTY_BLADE) + .id(); + let mut runner = scenario.build(); + attach(&mut runner, blade, host); + refresh(&mut runner); + runner.state_mut().active_player = P1; + (runner, blade, host) +} +// callers: setup(GameScenario::new()) / setup(GameScenario::new_n_player(3, 42))🤖 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/bloodthirsty_blade_goad_5973.rs` around lines 55 - 91, Consolidate setup_two_player and setup_three_player into a single parameterized scenario builder, such as a helper accepting the desired player count or scenario constructor. Preserve the shared creature, equipment, attachment, refresh, and active-player setup, while selecting GameScenario::new for two players and GameScenario::new_n_player(3, 42) for three players; update callers to use the unified helper.
🤖 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/bloodthirsty_blade_goad_5973.rs`:
- Around line 199-207: Strengthen the assertion for host in
attacker_constraints_for_active_player by destructuring
CombatRequirement::MustAttack and validating its away-from-goader payload
identifies P0 as the player to avoid, including the expected two-player
exemption behavior. Mirror the corresponding checks in
validate_attack_declaration instead of accepting any MustAttack variant.
- Around line 158-171: Strengthen the attach-target assertion in the Typed
target-filter branch to verify both opponent control and the creature-type
restriction expressed by the Oracle text. Use the existing type-related field or
predicate on tf, while preserving the current failure messages and rejection of
non-Typed filters.
---
Nitpick comments:
In `@crates/engine/tests/integration/bloodthirsty_blade_goad_5973.rs`:
- Around line 55-91: Consolidate setup_two_player and setup_three_player into a
single parameterized scenario builder, such as a helper accepting the desired
player count or scenario constructor. Preserve the shared creature, equipment,
attachment, refresh, and active-player setup, while selecting GameScenario::new
for two players and GameScenario::new_n_player(3, 42) for three players; update
callers to use the unified helper.
🪄 Autofix (Beta)
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: 69fcbbbe-e0e5-4a29-970e-9c6c92536a93
📒 Files selected for processing (2)
crates/engine/tests/integration/bloodthirsty_blade_goad_5973.rscrates/engine/tests/integration/main.rs
| let Effect::Attach { target, .. } = attach.effect.as_ref() else { | ||
| unreachable!() | ||
| }; | ||
| match target { | ||
| TargetFilter::Typed(tf) => { | ||
| assert_eq!( | ||
| tf.controller, | ||
| Some(ControllerRef::Opponent), | ||
| "attach target must be opponent-controlled, got {tf:?}" | ||
| ); | ||
| } | ||
| other => panic!("expected Typed opponent-creature filter, got {other:?}"), | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Attach-target assertion doesn't verify the "creature" restriction, only "opponent".
The Oracle text is "target creature an opponent controls," but the assertion only checks tf.controller == Some(ControllerRef::Opponent). If the parser dropped the creature-type restriction (letting the ability target any opponent permanent), this test would still pass — leaving a real semantic regression uncaught.
🧪 Suggested addition
match target {
TargetFilter::Typed(tf) => {
assert_eq!(
tf.controller,
Some(ControllerRef::Opponent),
"attach target must be opponent-controlled, got {tf:?}"
);
+ assert!(
+ tf.types.iter().any(|t| t == "Creature"),
+ "attach target must be restricted to creatures, got {tf:?}"
+ );
}
other => panic!("expected Typed opponent-creature filter, got {other:?}"),
}As per path instructions, "Test adequacy is the highest-frequency contributor finding — scrutinize it."
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let Effect::Attach { target, .. } = attach.effect.as_ref() else { | |
| unreachable!() | |
| }; | |
| match target { | |
| TargetFilter::Typed(tf) => { | |
| assert_eq!( | |
| tf.controller, | |
| Some(ControllerRef::Opponent), | |
| "attach target must be opponent-controlled, got {tf:?}" | |
| ); | |
| } | |
| other => panic!("expected Typed opponent-creature filter, got {other:?}"), | |
| } | |
| } | |
| let Effect::Attach { target, .. } = attach.effect.as_ref() else { | |
| unreachable!() | |
| }; | |
| match target { | |
| TargetFilter::Typed(tf) => { | |
| assert_eq!( | |
| tf.controller, | |
| Some(ControllerRef::Opponent), | |
| "attach target must be opponent-controlled, got {tf:?}" | |
| ); | |
| assert!( | |
| tf.types.iter().any(|t| t == "Creature"), | |
| "attach target must be restricted to creatures, got {tf:?}" | |
| ); | |
| } | |
| other => panic!("expected Typed opponent-creature filter, got {other:?}"), | |
| } | |
| } |
🤖 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/bloodthirsty_blade_goad_5973.rs` around lines
158 - 171, Strengthen the attach-target assertion in the Typed target-filter
branch to verify both opponent control and the creature-type restriction
expressed by the Oracle text. Use the existing type-related field or predicate
on tf, while preserving the current failure messages and rejection of non-Typed
filters.
Source: Path instructions
| let constraints = attacker_constraints_for_active_player(runner.state(), &valid); | ||
| assert!( | ||
| matches!( | ||
| constraints.get(&host), | ||
| Some(CombatRequirement::MustAttack { .. }) | ||
| ), | ||
| "display constraints must surface MustAttack for the host, got {:?}", | ||
| constraints.get(&host) | ||
| ); |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
MustAttack { .. } wildcard hides the away-from-goader payload this PR is actually regression-testing.
The match only confirms some MustAttack variant is present for host, discarding its fields via { .. }. The #5973 bug was specifically about the "attack a player other than the equipment controller if able" clause — if attacker_constraints_for_active_player returns a MustAttack with the wrong or missing away-from-goader data (e.g., pointing at the wrong player, or omitting the exemption for the 2-player case), this assertion still passes.
Consider destructuring the actual field(s) of MustAttack (e.g., an away-from-player set/goader id) and asserting they identify P0 as the player to avoid, mirroring what validate_attack_declaration checks later in the file. As per path instructions, negative/behavioral test adequacy for the exact regression under test should be scrutinized closely.
🤖 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/bloodthirsty_blade_goad_5973.rs` around lines
199 - 207, Strengthen the assertion for host in
attacker_constraints_for_active_player by destructuring
CombatRequirement::MustAttack and validating its away-from-goader payload
identifies P0 as the player to avoid, including the expected two-player
exemption behavior. Mirror the corresponding checks in
validate_attack_declaration instead of accepting any MustAttack variant.
Source: Path instructions
Closed: required model/tier declaration missingThis PR was opened at The current PR body has neither declaration. Its only model-adjacent commit evidence is This is a policy close, not a judgment of the test change. Please resubmit from current |
2 participants
Summary
StaticMode::Goaded, no ContinuousAddStaticModedouble-graft), activate-attach to an opponent, +2/+0 pump, must-attack, 2p attack-into-goader legality, and 3p must-attack-away-from equipment controller.Test plan
cargo test -p engine --test integration bloodthirsty_blade_goad_5973— 7/7 passedSummary by CodeRabbit