feat(engine): Twinning Staff copy-count replacement - #1067
Conversation
Twinning Staff's activated ability ({7},{T}: copy target instant/sorcery
you control) already worked, but its replacement line — "If you would
copy a spell one or more times, instead copy it that many times plus an
additional time. You may choose new targets for the additional copy." —
fell back to Effect::Unimplemented{name:"replacement_structure"}.
Implement it as a first-class CopySpell ReplacementDefinition carrying
QuantityModification::Plus{value:1}, mirroring the token/counter doubling
family (Doubling Season, Hardened Scales). The count is applied at the
copy-count chokepoint (the repeat_for loop in effects/mod.rs) via the new
helper copy_spell::copy_count_with_replacements, because copies are
produced through that loop rather than the ProposedEvent replacement
pipeline.
Correctness (CR 707.10 + CR 614.1a):
- only copies of a *spell* are bumped, not abilities (Gogo);
- only the copying player's own Staff applies ("if YOU would copy");
- the bumped count flows into total_iterations/resume stash, so each
additional copy runs the normal per-copy retarget step.
Tests: parser (line -> CopySpell/Plus{1}) + 3 engine helper tests
(count bump, opponent-Staff ignored, ability-copies excluded).
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Copying a *targeted* spell with Twinning Staff exploded into dozens of copies (in-game "stuck in a loop"). Each copy pauses on CopyRetarget; the drain driver resumes the next iteration by feeding a single-iteration resume ability (repeat_for cleared) back through resolve_effect. The new CopySpell count hook re-fired on every resumed iteration, re-adding the "+1 additional copy" bonus each time and re-expanding the loop — runaway copies (CR 614.6: a replacement applies to the copy event once, not per copy). Fix: add `copy_count_finalized` to ResolvedAbility. The repeat-loop resume stash sets it, and the CopySpell count hook skips the bonus when it is set, so the Twinning Staff bonus is folded into total_iterations exactly once at the initial resolution. Untargeted copies (no pause) were already correct; this only affects the pause/resume path. Add a regression test that copies a targeted spell with Twinning Staff, drives each retarget pause to completion, and asserts exactly two copies (a runaway trips the loop guard). All ResolvedAbility struct literals get the new field; `..ResolvedAbility::new(..)` spread sites are unchanged. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
…ral parse, zero-copy guard
Addresses PR review findings against the project's architectural rules:
- R2 (no bool fields): replace `copy_count_finalized: bool` on ResolvedAbility
with a typed `CopyCountStatus { Pending, Finalized }` enum, which expresses
the design space and reads self-documentingly at the count hook and the
repeat-loop resume stash.
- R1 / L2 (modular combinators + plural sibling coverage): rebuild the
"additional time(s)" parse from composed nom combinators along three
independent axes — count (`an` => 1, else a number), the `additional` token,
and the singular/plural `time(s)` noun — instead of full-phrase tags. Now
"plus an additional time" and "plus N additional times" both parse; added a
plural/numbered parser test.
- L4 (edge case): guard `copy_count_with_replacements` so the bonus does not
apply when the base copy count is zero (CR 614.6 — "if you would copy a spell
one or more times" has no event to replace at zero); added a regression test.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
R6 self-review of the Twinning Staff copy-count work: the "applies once,
not per copy" anti-runaway guard and the "one or more times" precondition
were both annotated CR 614.6, whose body ("a replaced event never happens")
describes neither claim.
Verified against docs/MagicCompRules.txt and corrected:
- "applies once / not invoked repeatedly per copy" -> CR 614.5 (a
replacement effect gets only one opportunity to affect an event)
- "one or more times" precondition / zero-copies guard -> CR 614.1 (a
replacement effect watches for an event that would happen)
Comment/doc-comment only; no logic change.
Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
Keep the running copy count as usize and widen the u32 QuantityModification values into it (value as usize, always lossless) instead of narrowing base (usize) to u32 up front, which could truncate on 64-bit targets. Addresses the gemini-code-assist review on PR #1. No behavior change for realistic copy counts; removes a latent narrowing cast. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements MTG rules CR 707.10, CR 614.1a, and CR 614.5 to support copy-count replacement effects (such as Twinning Staff) for copy-spell abilities, introducing a CopyCountStatus enum to guard against runaway loops. The review feedback focuses on aligning the parser dispatch and implementation in oracle_replacement.rs with the R1 architectural rule, specifically recommending the elimination of scan_contains in favor of direct nom parser delegation and refactoring the manual loop into modular, composed nom combinators.
| if nom_primitives::scan_contains(&lower, "would copy a spell") { | ||
| if let Some(def) = parse_copy_count_replacement(&lower, &text) { | ||
| return Some(def); | ||
| } | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid using scan_contains for parsing dispatch. Evidence: crates/engine/src/parser/oracle_replacement.rs:407. Why it matters: Using scan_contains for parsing dispatch violates the R1 architectural rule of using nom combinators on the first pass. Suggested fix: Call parse_copy_count_replacement directly and let the nom parser handle the "would copy a spell" check.
if let Some(def) = parse_copy_count_replacement(&lower, &text) {
return Some(def);
}References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Findings: any new .contains("..."), .starts_with("..."), .ends_with("..."), .find("..."), or .split_once("...") used for parsing dispatch in non-test parser code. (link)
| fn parse_copy_count_replacement(lower: &str, original_text: &str) -> Option<ReplacementDefinition> { | ||
| use crate::types::ability::QuantityModification; | ||
|
|
||
| // Require the "plus [N] additional time(s)" tail so this only matches the | ||
| // count-increasing class, not an unrelated one-shot "copy a spell" effect. | ||
| // Composed from modular combinators along three independent axes — count | ||
| // (`an` => 1, else a number), the fixed `additional` token, and the | ||
| // singular/plural `time(s)` noun — rather than enumerating full-phrase tags, | ||
| // so "plus an additional time" and "plus N additional times" both parse. | ||
| let additional = nom_on_lower(lower, lower, |i| { | ||
| let (i, _) = take_until::<_, _, OracleError<'_>>("plus ").parse(i)?; | ||
| let (i, _) = tag("plus ").parse(i)?; | ||
| let (i, n) = alt((value(1u32, tag("an")), nom_primitives::parse_number)).parse(i)?; | ||
| let (i, _) = tag(" additional ").parse(i)?; | ||
| let (i, _) = alt((tag("times"), tag("time"))).parse(i)?; | ||
| Ok((i, n)) | ||
| }) | ||
| .map(|(n, _)| n)?; | ||
|
|
||
| Some( | ||
| ReplacementDefinition::new(ReplacementEvent::CopySpell) | ||
| .quantity_modification(QuantityModification::Plus { value: additional }) | ||
| .description(original_text.to_string()), | ||
| ) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid verbatim string equality and fragile manual loops for parsing Oracle phrases. Instead, decompose compound phrases into modular, reusable parsers for constituent parts (such as the copy subject and the quantity modifier) and compose them using idiomatic nom combinator aggregates to prevent combinatorial explosion and improve maintainability.
fn parse_copy_spell_subject(input: &str) -> IResult<&str, &str> {
tag("would copy a spell")(input)
}
fn parse_plus_modifier(input: &str) -> IResult<&str, u32> {
let (input, _) = tag("plus ")(input)?;
let (input, n) = alt((value(1u32, tag("an")), nom_primitives::parse_number))(input)?;
let (input, _) = tag(" additional ")(input)?;
let (input, _) = alt((tag("times"), tag("time")))(input)?;
Ok((input, n))
}
fn parse_copy_count_replacement(lower: &str, original_text: &str) -> Option<ReplacementDefinition> {
use crate::types::ability::QuantityModification;
let additional = nom_on_lower(lower, lower, |i| {
let (i, _) = take_until::<_, _, OracleError<'_>>("would copy a spell").parse(i)?;
let (i, _) = parse_copy_spell_subject(i)?;
let mut input = i;
loop {
if let Ok((remaining, n)) = parse_plus_modifier(input) {
return Ok((remaining, n));
}
let (next_input, _) = nom::character::complete::anychar(input)?;
input = next_input;
}
})
.map(|(n, _)| n)?;
Some(
ReplacementDefinition::new(ReplacementEvent::CopySpell)
.quantity_modification(QuantityModification::Plus { value: additional })
.description(original_text.to_string()),
)
}References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
Implements Twinning Staff and the broader "copy an additional time" replacement class.
What
"If you would copy a spell one or more times, instead copy it that many times plus an additional time. You may choose new targets for the additional copy."
Approach (building block, not a one-off)
CopySpellreplacement carrying aQuantityModification— the same shape as the token/counter doubling family (Doubling Season, Hardened Scales), so it generalizes to "plus N additional times", not just Twinning Staff.additional/ singular-pluraltime(s)); no verbatim-phrase matching. Coversplus an additional timeandplus N additional times.CopyCountStatusenum (not a bool) guards the per-copy resume so the bonus applies to the copy event once, not per copy.Rules fidelity (CR annotations verified against MagicCompRules)
Fixes
Tests
7 regression tests (parser base + plural/numbered variants; engine: +1 copy, zero-copy guard, opponent's Staff ignored, ability-copies excluded, targeted-spell no-runaway). Full engine suite: 434 passed, 0 failed.
Commits