fix(coverage): expose GrantAll{Activated,Triggered}AbilitiesOf source in the parse-diff signature - #5585
Conversation
… in the parse-diff signature
There was a problem hiding this comment.
Code Review
This pull request updates fmt_modification in crates/engine/src/game/coverage.rs to expose the source filter (and cap when present) for GrantAllActivatedAbilitiesOf and GrantAllTriggeredAbilitiesOf modifications, ensuring distinct signatures are generated. A corresponding unit test was added to verify this behavior. The reviewer suggests expanding the test coverage to explicitly assert that the cap field is also correctly exposed when present.
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.
| use crate::types::ability::{ContinuousModification, TargetFilter}; | ||
|
|
||
| let act = |source: TargetFilter| { | ||
| fmt_modification(&ContinuousModification::GrantAllActivatedAbilitiesOf { | ||
| source, | ||
| cap: None, | ||
| }) | ||
| }; | ||
| let trg = |source: TargetFilter| { | ||
| fmt_modification(&ContinuousModification::GrantAllTriggeredAbilitiesOf { source }) | ||
| }; | ||
|
|
||
| // The source filter must appear in each signature ... | ||
| assert!( | ||
| act(TargetFilter::Controller).contains(&fmt_target(&TargetFilter::Controller)), | ||
| "activated-grant signature must expose its source filter", | ||
| ); | ||
| assert!( | ||
| trg(TargetFilter::SelfRef).contains(&fmt_target(&TargetFilter::SelfRef)), | ||
| "triggered-grant signature must expose its source filter", | ||
| ); | ||
| // ... so different source filters produce distinct signatures, not one bare label. | ||
| assert_ne!( | ||
| act(TargetFilter::Controller), | ||
| act(TargetFilter::SelfRef), | ||
| "different source filters must produce different activated-grant signatures", | ||
| ); |
There was a problem hiding this comment.
[MED] Incomplete test coverage for the cap field of GrantAllActivatedAbilitiesOf. Evidence: crates/engine/src/game/coverage.rs:10495.
Why it matters: The test asserts that the source filter is exposed in the signature, but does not verify that the cap field is also correctly exposed when present, leaving a potential blind spot for future modifications.
Suggested fix: Update the test to assert that the cap field is exposed in the signature when present.
use crate::types::ability::{ActivationRestriction, ContinuousModification, TargetFilter};
let act = |source: TargetFilter, cap: Option<ActivationRestriction>| {
fmt_modification(&ContinuousModification::GrantAllActivatedAbilitiesOf {
source,
cap,
})
};
let trg = |source: TargetFilter| {
fmt_modification(&ContinuousModification::GrantAllTriggeredAbilitiesOf { source })
};
// The source filter must appear in each signature ...
assert!(
act(TargetFilter::Controller, None).contains(&fmt_target(&TargetFilter::Controller)),
"activated-grant signature must expose its source filter",
);
assert!(
trg(TargetFilter::SelfRef).contains(&fmt_target(&TargetFilter::SelfRef)),
"triggered-grant signature must expose its source filter",
);
// ... so different source filters produce distinct signatures, not one bare label.
assert_ne!(
act(TargetFilter::Controller, None),
act(TargetFilter::SelfRef, None),
"different source filters must produce different activated-grant signatures",
);
// ... and the cap must be exposed when present.
assert!(
act(TargetFilter::Controller, Some(ActivationRestriction::OnlyOnceEachTurn))
.contains("OnlyOnceEachTurn"),
"activated-grant signature must expose its cap when present",
);
Parse changes introduced by this PR · 16 card(s), 13 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
Approve — right seam, minimal blast radius, and the test actually discriminates. Fifth in the established fmt_modification blind-spot series (#5492 PreventDamage, #5495 ChangeZone, #5501 ChangeZoneAll, #5507 Mana, #5511 cap-when-set), and it follows the pattern faithfully.
✅ Clean
- The fix is at the seam, not around it.
GrantAllActivatedAbilitiesOf/GrantAllTriggeredAbilitiesOfrendered bare labels, swallowingsourcewith... A parser change to which permanents' abilities are granted therefore produced a removal with no compensating addition in the sticky — the signature of a regression, when it was really a half-rendered modification. Exposingsourcethrough the existingfmt_targetreuses the building block instead of hand-rolling a formatter. - Fully destructured —
{ source, cap }, no... This is the load-bearing detail: a future field added toGrantAll*is now a compile error rather than another silent omission. That's what stops this bug class from recurring a sixth time, and it's why the fix is worth more than the four lines it changes. caprendered only whenSome, so unqualified signatures stay byte-identical and this doesn't churn the coverage baseline for cards that never set a cap.grant_all_abilities_signature_exposes_sourceis discriminating. Theassert_ne!on two different source filters would fail on the pre-fix code — both rendered the identical bare label. It tests the building block's behavior across its input range rather than replaying one card.- All 13 checks green; 1 file, +48/−4.
Approving and enqueueing as bug.
fmt_modificationrenderedGrantAllActivatedAbilitiesOf/GrantAllTriggeredAbilitiesOfas the bare labelsgrant all activated abilities of/grant all triggered abilities of, swallowing theirsourcefilter (andcap) with...So a parser change to which permanents' abilities are granted — the
sourcefilter — produces a removal with no compensating addition in the coverage-parse-diff sticky: the false signature of a regression, when it's really a half-rendered modification. This is the same class as #5492 (PreventDamage), #5495 (ChangeZone), #5501 (ChangeZoneAll), and #5507 (Mana grants).Fix
Expose the
sourcefilter via the existingfmt_target, and thecaponly when set so unqualified signatures stay byte-identical (mirroring #5511). Fully destructured (no..) so a newGrantAll*field is a compile error, not another silent omission.Test
grant_all_abilities_signature_exposes_source— asserts the source filter appears in both signatures and that two different source filters produce distinct signatures (rather than one shared bare label).Full engine lib: 16167 passed, 0 failed;
fmt/ parser-combinator gate /clippy -D warningsall clean.