feat(parser): turn-window CDA — leading timing condition + constant P/T form (Angry Mob)#5196
Conversation
…g condition + constant form (Angry Mob)
Angry Mob ("During your turn, ~'s power and toughness are each equal to 2 plus
the number of Swamps your opponents control. During turns other than yours, ~'s
power and toughness are each 2.") dropped entirely: the CDA-equality handler
ignored a leading turn-window timing condition, and had no "are each N" constant
form — so the off-turn clause never parsed and the multi-sentence splitter
(parse_multi_sentence_statics) failed the "every segment yields a static" guard.
Extend parse_cda_pt_equality (oracle_static/cda.rs) with two additions:
1. Peel a leading "During your turn," / "During turns other than yours," and
attach StaticCondition::DuringYourTurn / Not{DuringYourTurn} (mirrors the
base-P/T-set path via nom_tag_lower), so the CDA-equality form carries the
condition consistently.
2. Constant CDA P/T: "~'s power and toughness are each N" -> SetPower{N} +
SetToughness{N} (guarded by !both so the dynamic "equal to" framing keeps
priority).
With both clauses now parsing, the existing multi-sentence splitter emits the
two turn-gated CDA statics automatically. No new variant, no new runtime.
CR 611.3 + CR 613.4c + CR 613.7c.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request adds support for parsing characteristic-defining abilities (CDAs) with turn-window timing conditions (such as 'During your turn' or 'During turns other than yours') and constant characteristic-defining power and toughness values. The feedback identifies incorrect Comprehensive Rules (CR) citations (citing CR 613.7c instead of CR 611.3a/613.7a, and CR 613.4c instead of CR 613.4b) and highlights a fragile manual slicing implementation that could potentially panic, suggesting a safer parsing approach using nom_tag_lower.
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.
| // CR 611.3 + CR 613.7c: peel a leading turn-window timing condition so a CDA | ||
| // scoped to "During your turn," / "During turns other than yours," carries | ||
| // that condition (Angry Mob), mirroring the base-P/T-set path. Such a card's | ||
| // two clauses are split into separate sentences upstream by | ||
| // `parse_multi_sentence_statics`, so each clause reaches here independently. | ||
| let (lower, text, timing_condition) = | ||
| if let Some(rest) = nom_tag_lower(lower, lower, "during your turn, ") { | ||
| ( | ||
| rest, | ||
| &text[text.len() - rest.len()..], | ||
| Some(StaticCondition::DuringYourTurn), | ||
| ) | ||
| } else if let Some(rest) = nom_tag_lower(lower, lower, "during turns other than yours, ") { | ||
| ( | ||
| rest, | ||
| &text[text.len() - rest.len()..], | ||
| Some(StaticCondition::Not { | ||
| condition: Box::new(StaticCondition::DuringYourTurn), | ||
| }), | ||
| ) | ||
| } else { | ||
| (lower, text, None) | ||
| }; |
There was a problem hiding this comment.
[HIGH] Incorrect CR citation and fragile manual slicing. Evidence: crates/engine/src/parser/oracle_static/cda.rs:14-36.
Why it matters: Citing CR 613.7c (which is about counters) is incorrect for timing conditions on static abilities, and manual slicing using text.len() - rest.len() is fragile and can panic if case folding results in different byte lengths for lower and text.
Suggested fix: Use CR 611.3a / CR 613.7a and call nom_tag_lower on both lower and text to safely extract the remaining slices without manual slicing.
// CR 611.3a + CR 613.7a: peel a leading turn-window timing condition so a CDA
// scoped to "During your turn," / "During turns other than yours," carries
// that condition (Angry Mob), mirroring the base-P/T-set path. Such a card's
// two clauses are split into separate sentences upstream by
// `parse_multi_sentence_statics`, so each clause reaches here independently.
let (lower, text, timing_condition) =
if let Some(rest_lower) = nom_tag_lower(lower, lower, "during your turn, ") {
(
rest_lower,
nom_tag_lower(lower, text, "during your turn, ").unwrap(),
Some(StaticCondition::DuringYourTurn),
)
} else if let Some(rest_lower) = nom_tag_lower(lower, lower, "during turns other than yours, ") {
(
rest_lower,
nom_tag_lower(lower, text, "during turns other than yours, ").unwrap(),
Some(StaticCondition::Not {
condition: Box::new(StaticCondition::DuringYourTurn),
}),
)
} else {
(lower, text, None)
};References
- Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
| // CR 613.4c: constant characteristic-defining P/T — "~'s power and toughness | ||
| // are each N" (Angry Mob's off-turn clause "... are each 2"): a fixed base | ||
| // value, not a dynamic quantity. Guarded by `!both` so the dynamic "are each | ||
| // equal to" framing (which also contains "are each ") keeps priority. |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for P/T setting. Evidence: crates/engine/src/parser/oracle_static/cda.rs:43-46.
Why it matters: Citing CR 613.4c (which is Layer 7c for modifications) is incorrect for setting power and toughness to a specific number or value, which belongs in Layer 7b (CR 613.4b).
Suggested fix: Update the comment to cite CR 613.4b.
| // CR 613.4c: constant characteristic-defining P/T — "~'s power and toughness | |
| // are each N" (Angry Mob's off-turn clause "... are each 2"): a fixed base | |
| // value, not a dynamic quantity. Guarded by `!both` so the dynamic "are each | |
| // equal to" framing (which also contains "are each ") keeps priority. | |
| // CR 613.4b: constant characteristic-defining P/T — "~'s power and toughness | |
| // are each N" (Angry Mob's off-turn clause "... are each 2"): a fixed base | |
| // value, not a dynamic quantity. Guarded by !both so the dynamic "are each | |
| // equal to" framing (which also contains "are each ") keeps priority. |
References
- Every rules-touching line of engine code must carry a comment of the form CR : . The number must be verified against docs/MagicCompRules.txt before writing. (link)
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The new CR annotations point to rules that do not describe this CDA parsing path. Evidence: crates/engine/src/parser/oracle_static/cda.rs:14 cites CR 613.7c, which is counter timestamp ordering, and crates/engine/src/parser/oracle_static/cda.rs:43 / crates/engine/src/parser/oracle_static/tests.rs:78 cite CR 613.4c, which is the modifier/counter sublayer rather than CDA/set P/T; local rules text has CDA P/T in CR 604.3 / CR 613.4a and set P/T in CR 613.4b. Why it matters: engine/parser rule annotations are part of the verification contract, and these comments send future reviewers to the wrong layer/timestamp rules. Suggested fix: replace the 613.7c / 613.4c citations with the verified CDA/static-effect/layer citations that match the intended semantics.
[MED] The new regression test does not prove the on-turn clause carries the claimed quantity. Evidence: crates/engine/src/parser/oracle_static/tests.rs:102 only checks that some SetDynamicPower exists, but it never asserts SetDynamicToughness or that both values are 2 + ObjectCount(Swamps your opponents control). Why it matters: the PR claims to recover Angry Mob's dynamic clause, but the test would still pass if the parser emitted the wrong dynamic quantity or omitted dynamic toughness. Suggested fix: mirror the nearby CDA quantity tests and match both dynamic P/T modifications against the exact QuantityExpr::Offset { offset: 2, inner: Ref(ObjectCount{Swamp, controller: Opponent}) } shape.
Parse changes introduced by this PR · 1 card(s), 3 signature(s) (baseline: main
|
…indow CDA (review) Review response (matthewevans [MED], gemini [HIGH/MED]): - Correct CR annotations: the timing-condition peel cited CR 613.7c (counter timestamp ordering) — replaced with CR 611.3a (a continuous effect from a static ability applies per its text, so the turn window is a StaticCondition) + CR 604.3 (CDA). The constant P/T form cited CR 613.4c (Layer 7c modifiers) — replaced with CR 604.3 + CR 613.4a (Layer 7a: CDAs that define P/T). Verified against docs/MagicCompRules.txt. Test annotation updated to match. - Replace the fragile `text.len() - rest.len()` manual byte slicing with `nom_tag_tp(&TextPair::new(text, lower), ..)`, which slices the original and lowercased strings in lockstep — no offset arithmetic, no case-fold panic risk. No behavior change: 957 oracle_static tests pass, parser gate + fmt clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed in CR annotations (matthewevans, gemini) — corrected against
Fragile slicing (gemini [HIGH]) — replaced No behavior change: full oracle_static suite (957) passes, parser-combinator gate + fmt clean. The two-branch parse is unchanged (dynamic |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The prior test-proof blocker is still present on the new head. Evidence: crates/engine/src/parser/oracle_static/tests.rs:102 still only checks that the on-turn branch contains some SetDynamicPower { .. }; it does not assert SetDynamicToughness, the +2 offset, the ObjectCount, the Swamp subtype, or ControllerRef::Opponent. Why it matters: the PR claims to recover Angry Mob's on-turn 2 + Swamps your opponents control CDA, but this test would still pass if that quantity were wrong or if toughness were not emitted. Suggested fix: match both on-turn dynamic P/T modifications against the exact QuantityExpr::Offset { offset: 2, inner: Ref(ObjectCount{Typed Swamp, controller: Opponent}) } shape, similar to the nearby CDA quantity tests.
Strengthen the two-clause turn-window CDA test so the on-turn assertion proves the entire quantity by value — 2 + opponent Swamp count set on BOTH power and toughness — instead of merely checking that some SetDynamicPower mod exists. The vector-equality assertion now fails if the +2 offset, the counted Swamp subtype, the Opponent controller scope, or the toughness clause regress. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Addressed the remaining test-proof blocker in The on-turn assertion no longer merely checks that some let two_plus_opponent_swamps = QuantityExpr::Offset {
inner: Box::new(QuantityExpr::Ref {
qty: QuantityRef::ObjectCount {
filter: TargetFilter::Typed(
TypedFilter::new(TypeFilter::Subtype("Swamp".to_string()))
.controller(ControllerRef::Opponent),
),
},
}),
offset: 2,
};
assert_eq!(on_turn.modifications, vec![
ContinuousModification::SetDynamicPower { value: two_plus_opponent_swamps.clone() },
ContinuousModification::SetDynamicToughness { value: two_plus_opponent_swamps },
]);This fails if the |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved. I re-checked head 954392e: the change is scoped to the CDA equality parser and its tests, the parse-diff is limited to Angry Mob, the CR annotations now cite the CDA/static/layer rules, and the test asserts both dynamic P/T modifications with the +2 offset, Swamp subtype, and opponent controller scope.
2 participants
Angry Mob ("During your turn, ~'s power and toughness are each equal to 2 plus the number of Swamps your opponents control. During turns other than yours, ~'s power and toughness are each 2.") dropped entirely — the CDA-equality handler ignored a leading turn-window timing condition and had no
"are each N"constant form, so the off-turn clause never parsed and the multi-sentence splitter (parse_multi_sentence_statics) failed its "every segment yields a static" guard.Extends
parse_cda_pt_equality(test-freeoracle_static/cda.rs) with two additions:"During your turn,"→StaticCondition::DuringYourTurn,"During turns other than yours,"→Not{DuringYourTurn}(vianom_tag_lower, mirroring the base-P/T-set path). The CDA-equality form now carries the condition consistently with the base-P/T form."~'s power and toughness are each N"→SetPower{N}+SetToughness{N}(guarded by!bothso the dynamic"equal to"framing keeps priority).With both clauses parsing, the existing multi-sentence splitter emits the two turn-gated CDA statics automatically:
Offset{ObjectCount(Swamp, opponents), +2}, gatedDuringYourTurn2/2, gatedNot{DuringYourTurn}No new variant, no new runtime. CR 611.3 + CR 613.4c + CR 613.7c.
Verified: new
static_angry_mob_two_clause_turn_window_cdatest (asserts both CDA statics, conditions, dynamic vs constant); full oracle_static suite 957/957 green (no CDA regressions); parser-combinator gate + fmt clean.🤖 Generated with Claude Code