fix(coverage): expose Mana effect grants in the parse-diff signature (Closes #5507) - #5511
Conversation
…n the parse-diff signature (phase-rs#5507) `effect_details` rendered only `produced` for `Effect::Mana`, swallowing `restrictions`/`grants`/`expiry`/`target` with `..`. So attaching a `ManaSpellGrant` to produced mana (Hall of the Bandit Lord's creature-spell haste rider, phase-rs#5502) showed only removals in the coverage-parse-diff sticky with no compensating addition — reading as a regression when it was a half-rendered effect. Third instance of the family (phase-rs#5492/phase-rs#5493 PreventDamage, phase-rs#5495/phase-rs#5501 ChangeZone). Following phase-rs#5501, fully destructure `Effect::Mana` (no `..`) so a new field is a compile error not another silent omission, emitting each only when set so unqualified mana signatures stay byte-identical. Adds mana_signature_exposes_grants. Closes phase-rs#5507. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request updates the Effect::Mana destructuring in effect_details to fully destructure and render all of its fields (produced, restrictions, grants, expiry, and target) rather than swallowing them with ... This prevents silent omissions of fields in the mana signature. A corresponding test was added to verify that the grants field is correctly exposed. The review feedback suggests expanding this test to cover all the newly exposed fields (restrictions, expiry, and target) to ensure comprehensive test coverage of the entire building block.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| #[test] | ||
| fn mana_signature_exposes_grants() { | ||
| use crate::types::ability::ManaContribution; | ||
| use crate::types::mana::ManaSpellGrant; | ||
|
|
||
| // #5507: a `ManaSpellGrant` attached to produced mana (e.g. Hall of the | ||
| // Bandit Lord's creature-spell haste rider, #5502) is parser-alterable but | ||
| // was swallowed by `..`. It must appear in the mana signature when set and | ||
| // be absent when the grants list is empty, so unqualified mana signatures | ||
| // stay byte-identical. (Mirrors #5493/#5501.) | ||
| let signature_keys = |grants: Vec<ManaSpellGrant>| -> Vec<String> { | ||
| effect_details(&Effect::Mana { | ||
| produced: ManaProduction::AnyOneColor { | ||
| count: QuantityExpr::Fixed { value: 1 }, | ||
| color_options: vec![ManaColor::White, ManaColor::Blue], | ||
| contribution: ManaContribution::Base, | ||
| }, | ||
| restrictions: vec![], | ||
| grants, | ||
| expiry: None, | ||
| target: None, | ||
| }) | ||
| .into_iter() | ||
| .map(|(k, _)| k) | ||
| .collect() | ||
| }; | ||
| assert!( | ||
| signature_keys(vec![ManaSpellGrant::AddKeywordUntilEndOfTurn { | ||
| keyword: Keyword::Haste, | ||
| restriction: None, | ||
| duration: Box::new(Duration::UntilEndOfTurn), | ||
| }]) | ||
| .iter() | ||
| .any(|k| k == "grants"), | ||
| "a set ManaSpellGrant must appear in the mana parse-diff signature", | ||
| ); | ||
| assert!( | ||
| !signature_keys(vec![]).iter().any(|k| k == "grants"), | ||
| "an empty grants list must not appear (unqualified mana signature unchanged)", | ||
| ); | ||
| } |
There was a problem hiding this comment.
The new test mana_signature_exposes_grants correctly verifies that the grants field is exposed in the Effect::Mana signature. However, the associated change in effect_details also exposes restrictions, expiry, and target fields, which are not covered by this test.
To make the test suite more robust and prevent future regressions for these other fields, I suggest expanding this test to cover all newly exposed fields. This aligns with the testing philosophy of asserting the properties of the entire building block, not just one input.
Here's a more comprehensive test that verifies each field individually:
#[test]
fn mana_signature_exposes_all_fields() {
use crate::types::ability::{ManaContribution, ManaSpendRestriction};
use crate::types::mana::{ManaExpiry, ManaSpellGrant};
// #5507: The 'Effect::Mana' signature must expose all its fields when set,
// and omit them when empty/default, to keep parse-diffs accurate.
let base_effect = Effect::Mana {
produced: ManaProduction::AnyOneColor {
count: QuantityExpr::Fixed { value: 1 },
color_options: vec![ManaColor::White, ManaColor::Blue],
contribution: ManaContribution::Base,
},
restrictions: vec![],
grants: vec![],
expiry: None,
target: None,
};
// Base case: only 'produced' is set.
let keys = effect_details(&base_effect).into_iter().map(|(k, _)| k).collect::<Vec<_>>();
assert_eq!(keys, vec!["mana"], "base effect should only have 'mana' key");
// Test 'restrictions'.
let with_restrictions = Effect::Mana {
restrictions: vec![ManaSpendRestriction::SpellOnly],
..base_effect.clone()
};
let keys = effect_details(&with_restrictions).into_iter().map(|(k, _)| k).collect::<Vec<_>>();
assert!(keys.contains(&"restrictions".to_string()), "must expose restrictions when set");
assert_eq!(keys.len(), 2, "should have mana and restrictions keys");
// Test 'grants'.
let with_grants = Effect::Mana {
grants: vec![ManaSpellGrant::AddKeywordUntilEndOfTurn {
keyword: Keyword::Haste,
restriction: None,
duration: Box::new(Duration::UntilEndOfTurn),
}],
..base_effect.clone()
};
let keys = effect_details(&with_grants).into_iter().map(|(k, _)| k).collect::<Vec<_>>();
assert!(keys.contains(&"grants".to_string()), "must expose grants when set");
assert_eq!(keys.len(), 2, "should have mana and grants keys");
// Test 'expiry'.
let with_expiry = Effect::Mana {
expiry: Some(ManaExpiry::EndOfTurn),
..base_effect.clone()
};
let keys = effect_details(&with_expiry).into_iter().map(|(k, _)| k).collect::<Vec<_>>();
assert!(keys.contains(&"expiry".to_string()), "must expose expiry when set");
assert_eq!(keys.len(), 2, "should have mana and expiry keys");
// Test 'target'.
let with_target = Effect::Mana {
target: Some(TargetFilter::Controller),
..base_effect.clone()
};
let keys = effect_details(&with_target).into_iter().map(|(k, _)| k).collect::<Vec<_>>();
assert!(keys.contains(&"target".to_string()), "must expose target when set");
assert_eq!(keys.len(), 2, "should have mana and target keys");
}
Parse changes introduced by this PR · 241 card(s), 87 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Approved. Closes #5507, the third instance of this family after #5492/#5495 (fixed by #5493/#5501), and it fixes the class rather than the instance.
The right seam and the right shape: effect_details is where the parse-diff signature is rendered, and the fix replaces Effect::Mana { produced, .. } with a full destructure of all five fields. I confirmed against types/ability.rs:10388-10406 that produced, restrictions, grants, expiry, and target are exactly the field set — so the .. is genuinely gone and a sixth Effect::Mana field becomes a compile error instead of a fourth silent omission. That converts a recurring silent-omission class into a compiler-enforced invariant, which is the durable fix.
The conditional emission is the detail that makes it safe: each field is pushed only when non-empty/Some, so unqualified mana signatures stay byte-identical and no baseline churn is introduced. The test asserts both directions — grants present when set, absent when empty — so it discriminates on revert in both the positive and the no-churn direction.
quality: exhaustive destructure over .., compile-time enforcement, zero baseline churn, paired positive/negative assertions.
Closes #5507.
Summary
The mana effect's parse-diff signature in
effect_detailsrendered onlyproduced, swallowingrestrictions/grants/expiry/targetwith... So a parser change that attaches aManaSpellGrantto produced mana — e.g. Hall of the Bandit Lord's creature-spell haste rider (#5502) — showed only removals in the coverage-parse-diff sticky with no compensating addition: the signature of a regression, when it was actually a half-rendered effect.This is the third instance of the family (#5492
PreventDamage→ #5493; #5495ChangeZone→ #5501). Following #5501's approach,Effect::Manais now fully destructured (no..), so a newly added field becomes a compile error rather than another silent omission, and each field is emitted only when set so unqualified mana signatures stay byte-identical.Fields now surfaced:
restrictions,grants(the named blind spot —ManaSpellGrant),expiry,target.Anchored on
crates/engine/src/game/coverage.rs— theEffect::ChangeZone/ChangeZoneAllarms (merged fix(coverage): expose ChangeZone/ChangeZoneAll entry qualifiers in the parse-diff signature #5501): same fully-destructure-and-emit-only-when-set shape,format!("{:?}")for complex payloads.crates/engine/src/game/coverage.rs—prevent_damage_signature_exposes_damage_source_filter/change_zone_signature_exposes_enters_attackingtests (fix(coverage): expose damage_source_filter in the PreventDamage parse-diff signature #5493/fix(coverage): expose ChangeZone/ChangeZoneAll entry qualifiers in the parse-diff signature #5501): the newmana_signature_exposes_grantsmirrors them exactly (build the effect, collect signature keys, assert present-when-set / absent-when-empty).CR
None. Per CLAUDE.md,
effect_detailsis a signature renderer (review plumbing), not rule-implementing code, so no CR annotations are added — consistent with the maintainer note on #5500 that these annotations are not required here (and it avoids a wrong-citation risk).Gate A
./scripts/check-parser-combinators.sh→ exit 0 (no violations). No parser-dispatch code — this extends a signature renderer with typed field reads.Verification
cargo test -p engine --lib coverage::tests→ green, including the newmana_signature_exposes_grants(grants appears when aManaSpellGrantis set, absent when empty; reverting the emission fails it).cargo fmt -p engineclean...) compiles, so any futureEffect::Manafield is a compile error until it's considered here.Scope / risk
Single file, review-signature tooling only (
effect_detailsfeeds the coverage-parse-diff sticky). No game-logic, parser, or serialization change — it does not alter how any card parses or resolves, only what the diff sticky reports. Zero runtime impact.Model: claude-opus-4-8
Tier: Frontier
Thinking: High