fix(parser): keep dynamic "for each" scaling when a trailing type-addition follows

[minion1227]

CR 205.1b + CR 613.4c

Bug

A dynamic "gets +N/+M for each X and is a <Subtype> in addition to its other types" pump collapsed to a fixed +N/+M. The trailing type-addition tail stayed on the count clause and broke parse_for_each_clause_expr, so the whole pump fell through to the fixed-pump path — the wrong power/toughness:

• Avatar Destiny — "Enchanted creature gets +1/+1 for each creature card in your graveyard and is an Avatar in addition to its other types."
• Machinist's Arsenal — "Equipped creature gets +2/+2 for each artifact you control and is an Artificer in addition to its other types."

Both emitted AddPower/AddToughness (fixed) instead of AddDynamicPower/AddDynamicToughness. (Verified: the same counts parse dynamically when the trailing clause is absent.)

Fix

strip_trailing_keyword_clause — which already peels a trailing keyword grant (" and has flying") off the for-each count so the count parses — now also peels a trailing " and is <...> in addition to its other types" type-addition, guarded on the "in addition to" marker so a genuine "<count> and is <...>" count phrase is never truncated. The count then parses dynamically, and the trailing subtype is recovered by the existing parse_additive_type_clause_modifications pass (added in #5621).

This completes the for-each trailing-conjunct handling: #5621 recovered the trailing subtype/quoted-ability when the count already parsed (counter-based, Reaper's Scythe); this recovers the dynamic scaling itself when the tail was breaking the count (object-count, Avatar Destiny / Machinist's Arsenal).

Why it's the right seam / minimal blast radius

• One authority: strip_trailing_keyword_clause is the shared for-each count-clause stripper (both for-each paths in anthem.rs use it), so both inherit the fix.
• The " and is " arm is gated on the type-addition marker; counter-based pumps (Reaper's Scythe) and keyword-only pumps (Claws of Valakut) are unchanged — regression-tested.

Tests

• dynamic_for_each_pump_with_trailing_type_keeps_dynamic_scaling — Avatar Destiny → AddDynamicPower (NOT fixed AddPower) + AddSubtype("Avatar").
• Full parser::oracle_static::tests (1060 passed, 0 failed), including the feat(parser): recover trailing subtype/quoted-ability after a for-each dynamic pump #5621 counter/keyword regression tests.

CR verification

• CR 205.1b — "in addition to its other types" type-addition; CR 613.4c — layer-7c dynamic P/T (both verified in docs/MagicCompRules.txt).

AI-contributor notes

• Rules-correct: the equipped/enchanted creature's power/toughness now scales with the count, not a fixed value.
• Verified: live-parsed Avatar Destiny / Machinist's Arsenal (dynamic recovered) and Reaper's Scythe / Claws of Valakut (unchanged) before/after; cargo fmt, CI-exact clippy -p engine --all-targets --features proptest -D warnings, and the full oracle_static module are green.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request updates strip_trailing_keyword_clause in grammar.rs to handle trailing type-addition clauses (e.g., "and is... in addition to...") so that dynamic count clauses parse correctly. However, the implementation uses raw string methods .find() and .contains(), which violates Rule R1 of the style guide. This rule prohibits raw string searches for parsing dispatch under crates/engine/src/parser/ and requires using nom 8.0 combinators or existing helpers instead.

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]

Avoid verbatim string equality or raw string searches (like .find(), .contains(), or take_until with hardcoded literals) 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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability. Additionally, avoid swallowing all errors (e.g., using .is_ok() or .ok()) when parsing; propagate unexpected errors using ? to prevent masking critical bugs.

Suggested change

	if let Some(pos) = clause.find(" and is ") {
	if clause[pos..].contains("in addition to") {
	return &clause[..pos];
	}
	}
	let (rest, (subject, conjunction, addition)) = tuple((
	parse_subject,
	alt((tag(" and is "), tag(" in addition to "))),
	parse_addition
	))(clause)?;

References

1. Rule R1: Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Raw string search methods like .find() and .contains() are prohibited for parsing dispatch in non-test parser code. ^(link)
2. 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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested alt and tag sequences) to prevent combinatorial explosion and improve maintainability.
3. Avoid swallowing all errors (e.g., using .ok()) when only specific errors are expected to be handled or ignored. Propagate unexpected errors (such as invariant violations or invalid actions) to prevent masking critical bugs.

[github-actions]

Parse changes introduced by this PR

Baseline pending for 6f7cee8e867b74719c7b44cce5e71f1c9c374c2b — this populates once main publishes its coverage snapshot (a few minutes after that commit landed).

[matthewevans]

Request changes: the two-card parser fix is correctly scoped, but its new dispatch violates the parser gate and its tests do not cover both affected quantity classes.

🔴 Blocker

• crates/engine/src/parser/oracle_static/grammar.rs:1564-1566 adds raw .find(" and is ") and .contains("in addition to") to Oracle parsing dispatch. The terminal Rust lint (fmt, clippy, parser gate) job rejects these exact expressions. Express the boundary through nom_primitives::scan_split_at_phrase with tag/terminated, following the existing type-addition grammar at crates/engine/src/parser/oracle_static/type_change.rs:444-483.

🟡 Non-blocking

• crates/engine/src/parser/oracle_static/tests.rs:91-113 verifies only Avatar Destiny and only the dynamic-vs-fixed variant. Machinist’s Arsenal reads: “Equipped creature gets +2/+2 for each artifact you control and is an Artificer in addition to its other types.” Add exact-card assertions for Avatar’s graveyard ZoneCardCount and Machinist’s controlled-artifact ObjectCount, including their filter and scope.

✅ Clean

• crates/engine/src/parser/oracle_static/grammar.rs:1558-1568 is the right shared seam: both dynamic for-each P/T paths call this stripper.
• The current <!-- coverage-parse-diff --> artifact measures exactly the claimed scope: Avatar Destiny and Machinist’s Arsenal both change fixed P/T modifications to dynamic P/T while retaining the subtype.

Recommendation: request changes—replace the raw string dispatch with the established nom scanning pattern, then add exact AST assertions for both measured cards.

[minion1227]

Thanks — both items addressed (commit f661840).

🔴 Parser gate: replaced the raw .find(" and is ") + .contains("in addition to") in strip_trailing_keyword_clause with nom_primitives::scan_split_at_phrase, scanning word boundaries for the and is verb boundary whose remainder reaches in addition to (tag + take_until + tag) — the same type-addition grammar shape as type_change.rs:444-483. Behavior is preserved (still guarded on the in addition to tail); clause is already lowercase (after_for_each.lower) so tags match directly, and the trim_end on the returned span is structural. The Rust lint (parser gate) job passes locally.

🟡 Exact-card AST assertions for both quantity classes:

• ..._keeps_dynamic_scaling now asserts Avatar Destiny's exact count on both power and toughness: ZoneCardCount { zone: Graveyard, card_types: [Creature], scope: Controller, filter: None }, plus AddSubtype(Avatar) and no fixed pump.
• ..._machinists_arsenal (new) asserts Machinist's Arsenal: Multiply(2, ObjectCount { Typed[Artifact], controller: You }) on power and toughness, plus AddSubtype(Artificer). Note +2/+2 for each scales via a Multiply wrapper (factor 2), unlike Avatar's bare +1 count — the test proves both shapes.

Full parser::oracle_static::tests green (1061 passed, 0 failed); fmt + parser gate clean.

[matthewevans]

Approved: the current head resolves the parser-dispatch and quantity-class test findings; the isolated adversarial recheck and terminal CI are clean.