fix: Hall of the Bandit Lord creature-spell haste mana rider (#5283)

[andriypolanski]

Summary

Hall of the Bandit Lord's activated mana ability was parsed with an incorrect Spell sub-ability (SelfRef haste static) instead of a ManaSpellGrant on the produced mana. Creature spells cast with its colorless mana therefore did not gain haste.

Fixes #5283.

Root cause

The mana-rider parser only recognized subtype-specific grants with an explicit "until end of turn" suffix:

"If that mana is spent on a Dragon creature spell, it gains haste until end of turn."

Hall of the Bandit Lord prints the broader, permanent form:

"If that mana is spent on a creature spell, it gains haste."

parse_conditional_keyword_grant declined this clause, so the sequence parser fell through to a generic If absorption that lowered haste onto SelfRef (the land) rather than folding a spend rider into Effect::Mana.grants.

Changes

Parser (oracle_effect/mana.rs)

• Extend parse_conditional_keyword_grant to accept:
  • bare "a creature spell" → OnlyForSpellType("Creature")
  • optional "until end of turn" suffix → Duration::UntilEndOfTurn vs Duration::Permanent
• Continuation path already folds matching clauses into ManaGrant on the parent Effect::Mana.

Types (types/mana.rs)

• Parameterize ManaSpellGrant::AddKeywordUntilEndOfTurn with duration: Duration (serde-default UntilEndOfTurn for existing Dragon-land cards).

Runtime (game/casting.rs)

• apply_mana_spell_grants respects per-grant duration instead of hardcoding UntilEndOfTurn.

Tests

• Parser: parses_hall_of_bandit_lord_creature_spell_haste_grant
• Integration: issue_5283_hall_of_bandit_lord.rs (parse shape + end-to-end cast with Hall mana → haste on battlefield)

Test plan

• cargo fmt --all
• cargo test -p engine --lib bandit_lord
• cargo test -p engine --lib conditional_mana_keyword_grant
• cargo test -p engine --test integration issue_5283 (2/2)

Risk notes

• Subtype + until-EOT dragon-land pattern unchanged (default duration preserves prior behavior).
• Permanent haste only applies to the new bare-creature-spell phrasing (Hall of the Bandit Lord class).
• Regenerating card-data.json will move Hall's rider from sub_ability into grants and clear the SwallowedClause warning.

[gemini-code-assist]

Code Review

This pull request adds support for parsing and applying permanent keyword grants from mana (such as from Hall of the Bandit Lord) by parameterizing the keyword grant with a Duration. The feedback highlights a critical bug in the casting logic where deduplicating keyword grants solely by keyword can discard permanent durations if a temporary duration is processed first.

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.

[gemini-code-assist]

[HIGH] Deduplicating keyword grants solely by the keyword discards permanent durations if a temporary duration is processed first.

If a creature spell is cast using both a temporary keyword-granting mana (e.g., UntilEndOfTurn from a Dragon-land) and a permanent keyword-granting mana (e.g., Permanent from Hall of the Bandit Lord), and the temporary grant is processed first, the permanent grant will be ignored because the keyword is already present in keyword_grants. The creature will incorrectly lose the keyword at the end of the turn, violating MTG rules.

We should deduplicate by both the keyword and the duration to ensure both distinct durations are applied correctly.

Suggested change

	if !keyword_grants.iter().any(|(k, _)| k == keyword) {
	keyword_grants.push((keyword.clone(), duration.clone()));
	}
	if !keyword_grants.iter().any(|(k, d)| k == keyword && d == duration) {
	keyword_grants.push((keyword.clone(), duration.clone()));
	}

References

1. Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. Discarding permanent keyword grants in favor of temporary ones violates this rule. ^(link)

[github-actions]

Parse changes introduced by this PR · 4 card(s), 3 signature(s) (baseline: main bdcb1e9e69da)

2 card(s) · ability/grant Haste · removed: grant Haste (affects=self, duration=until end of turn, grants=grant Haste)

Examples: Arena of Glory, Generator Servant

1 card(s) · ability/grant Haste · removed: grant Haste (affects=self, grants=grant Haste)

Examples: Hall of the Bandit Lord

1 card(s) · ability/grant Riot · removed: grant Riot (affects=self, grants=grant Riot)

Examples: Domri, Chaos Bringer

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

CI is red on 52572bb2, and the failure is real rather than infrastructure. Both failing checks trace to the same clippy lint, so this is one fix, not two.

error: large size difference between variants
##[error]   --> crates/engine/src/types/mana.rs:836:1
    |
836 | /   pub enum ManaSpellGrant {
837 | |       /// The spell cast with this mana can't be countered.
838 | |       CantBeCountered,
839 | |       /// CR 106.6 + CR 702.10: If the spell this mana is spent on satisfies
...   |
842 | | /     AddKeywordUntilEndOfTurn {
843 | | |         keyword: Keyword,
844 | | |         #[serde(default, skip_serializing_if = "Option::is_none")]
845 | | |         restriction: Option<ManaRestriction>,
846 | | |         #[serde(default = "default_mana_keyword_grant_duration")]
847 | | |         duration: crate::types::ability::Duration,

You added a variant to the mana enum in crates/engine/src/types/mana.rs and it now dominates the enum's size, so every value of that type pays for the largest variant. The usual remedy is to box the large payload. cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings is what CI runs, so that reproduces it locally.

I have not reviewed the substance yet, and will once CI is green. Two things I did check while I was here, so you know the premise holds:

• Scryfall confirms the card reads {T}, Pay 3 life: Add {C}. If that mana is spent on a creature spell, it gains haste. with no "until end of turn" suffix, which matches your root-cause description exactly.
• Your parse-diff sticky is present and fresh, and the integration test is correctly placed under crates/engine/tests/integration/ with the mod line added.

Ping me when it is green.

[matthewevans]

CI is green on a171541e and the clippy fix looks right, so the only thing standing in the way now is a merge conflict.

I tried gh pr update-branch and GitHub declined: "Cannot update PR branch due to conflicts." That means the conflict is not something the auto-merge can resolve, so it needs a rebase from you. Several PRs landed on main in the last hour touching the engine, so this is timing rather than anything wrong with your change.

Please rebase onto current main and push. Once mergeable goes back to clean I will review the substance. Two things I will be looking at when I do, so you can pre-empt them:

• The commit message says "dedup by duration" alongside the boxing fix. Boxing Duration to satisfy large_enum_variant is mechanical, but deduplicating grants by duration is a behavior change. I will want to know what happens when the same keyword is granted twice with different durations, and whether a test pins it.
• casting_tests.rs grew by 73 lines, which is encouraging. I will be checking that the new tests drive the real cast path rather than constructing a ManaSpellGrant by hand, because the bug you are fixing lives in the arrow from the parsed mana rider to the spell that spends the mana.

Nothing else outstanding.

[matthewevans]

Approved. Thanks for this one.

I verified the card text against Scryfall before reviewing the parse: Hall of the Bandit Lord reads {T}, Pay 3 life: Add {C}. If that mana is spent on a creature spell, it gains haste. There is no "until end of turn" suffix on the rider.

The dedup change is the fix, not a side effect

This is the thing I most want to flag, because your commit message undersells it. You describe the "dedup by duration" change as if it rides along with the mechanical large_enum_variant boxing. It doesn't ride along. It is the consumption half of the fix.

apply_mana_spell_grants in casting.rs previously collected a Vec<Keyword>, deduped on the keyword alone, and applied every grant with a hardcoded Duration::UntilEndOfTurn. A correctly parsed Permanent grant was therefore discarded at the point of use — the parser could produce the right AST all day and the board would never see it. Collecting Vec<(Keyword, Duration)>, deduping on the pair, and applying each grant with its own duration is a widening of that seam, and it is what lets the parser fix actually reach the battlefield. Without those nine lines the parser change would have been inert.

Duration assignment is correct

The parser sets Duration::UntilEndOfTurn only when the "until end of turn" suffix is present, and Duration::Permanent otherwise. Hall therefore grants permanent haste, which matches the printed card.

For your reassurance, since I expect you eyed it: default_mana_keyword_grant_duration() returning UntilEndOfTurn is a serde default for legacy data. It does not sit on the parse path.

The integration test drives the real seam

It builds Hall from Oracle text, activates the mana ability and asserts colorless mana in the pool, casts a creature spell with that mana, and asserts has_keyword(..., &Keyword::Haste) on the resulting battlefield permanent. A mocked grant could not make that pass. The arrow it traces — parsed rider, to spent mana, to keyword on the permanent — runs directly through where the defect lived.

Rules and style

CR 106.6 ("Some spells or abilities that produce mana restrict how that mana can be spent"), CR 702.10, and CR 702.10a are all grep-verified against docs/MagicCompRules.txt and all apt. Nom compliance is clean in the added parser lines.

Ignore the parse-diff sticky — it is a renderer bug, not a regression

Your parse-diff sticky shows four cards (Hall of the Bandit Lord, Arena of Glory, Generator Servant, Domri, Chaos Bringer) losing a grant Haste / grant Riot sub-ability with no compensating addition. Please do not chase this. It reads like a regression and it is not one.

ManaSpellGrant appears zero times in coverage.rs, so the signature renderer has no way to display the grant you attached to the produced mana. Only the removal of the old bogus self-affecting sub-ability is visible to it. The positive half of the change is real, and I verified it through your integration test instead. I have filed #5507 for the renderer gap; it is the third instance of that family, after #5492 and #5495.

Blast radius

Worth calling out: removing the bogus Spell sub-ability with its SelfRef haste static fixes three sibling cards beyond the one in the title. Nice reach for a targeted change.

Thirteen checks green. Approving and enqueuing.