Skip to content

Fix Dwarven Armorer counter choice parsing #6934

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
keloide wants to merge 1 commit into
base: main
Choose a base branch
from
+174 −95
Open

Fix Dwarven Armorer counter choice parsing #6934

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
155 changes: 60 additions & 95 deletions crates/engine/src/parser/oracle_effect/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5396,6 +5396,24 @@ enum ChoiceListShape {
SharedNoun,
}

/// Parse one complete counter noun phrase in a distributed choice list.
///
/// The counter-type parser intentionally admits open-ended named counters, but
/// a distributed item is valid only when that name is followed by the complete
/// singular or plural counter noun. This keeps bare noun disjunctions from
/// reaching the counter-choice branch builder.
fn parse_full_counter_noun(input: &str) -> Option<(CounterType, QuantityExpr)> {
let (count, rest) = parse_count_expr(input.trim())?;
let (rest, counter_type) = nom_primitives::parse_counter_type_typed(rest).ok()?;
all_consuming(alt((
tag::<_, _, OracleError<'_>>(" counters"),
tag(" counter"),
)))
.parse(rest)
.ok()?;
Some((counter_type, count))
}

/// CR 122.1b: keyword counters distribute over a single shared noun. Recognize
/// the "shared-noun" disjunctive list shape: ONE leading article, a list of
/// bare keyword adjectives, and ONE trailing "counter" — e.g. "a menace,
Expand Down Expand Up @@ -5429,6 +5447,39 @@ fn recognize_shared_noun_counter_list(input: &str) -> Option<Vec<&str>> {
Some(items)
}

/// Classify a counter-choice list and validate every member for the classified
/// shape. This is the single authority for the priority order and guards shared
/// by context-free callers and the branch-reparsing parser.
fn classify_counter_choice_list(input: &str) -> Option<(ChoiceListShape, Vec<&str>)> {
let (shape, items) =
if let Ok((rest, _)) = tag::<_, _, OracleError<'_>>("a counter from among ").parse(input) {
(ChoiceListShape::FromAmong, split_choice_list_items(rest)?)
} else if let Some(items) = recognize_shared_noun_counter_list(input) {
(ChoiceListShape::SharedNoun, items)
} else {
(
ChoiceListShape::Distributed,
split_choice_list_items(input)?,
)
};

if items.len() < 2 || items.iter().any(|item| item.trim().is_empty()) {
return None;
}

let valid = match shape {
ChoiceListShape::Distributed => items
.iter()
.all(|item| parse_full_counter_noun(item).is_some()),
ChoiceListShape::FromAmong | ChoiceListShape::SharedNoun => items.iter().all(|item| {
all_consuming(nom_primitives::parse_strict_counter_type)
.parse(item.trim())
.is_ok()
}),
};
valid.then_some((shape, items))
}

Comment on lines +5450 to +5482

@coderabbitai coderabbitai Bot Aug 2, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
# Description: Check case-normalization contract for classify_counter_choice_list callers and recognize_shared_noun_counter_list.
rg -n -B3 -A15 'fn recognize_shared_noun_counter_list' crates/engine/src/parser/oracle_effect/mod.rs
rg -n -B3 -A5 'classify_counter_choice_list\(' crates/engine/src/parser/oracle_effect/mod.rs

Repository: phase-rs/phase

Length of output: 2629

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- classifier callers and surrounding parser ---'
sed -n '5480,5615p' crates/engine/src/parser/oracle_effect/mod.rs
printf '%s\n' '--- case-normalization helpers and TextPair usage ---'
rg -n -B5 -A12 'try_parse_put_counter_choice|nom_on_lower|choices_tp|struct TextPair|impl TextPair' crates/engine/src/parser/oracle_effect crates/engine/src/parser
printf '%s\n' '--- classifier tests and call sites outside the file ---'
rg -n -B5 -A12 'classify_and_parse_counter_choice_list|classify_counter_choice_list|recognize_shared_noun_counter_list' crates/engine

Repository: phase-rs/phase

Length of output: 50371

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- exact classifier region ---'
sed -n '5415,5610p' crates/engine/src/parser/oracle_effect/mod.rs
printf '%s\n' '--- exact case-sensitive/case-insensitive references in mod.rs ---'
rg -n -B8 -A20 'try_parse_put_counter_choice|nom_on_lower|choices_tp|classify_and_parse_counter_choice_list|classify_counter_choice_list' crates/engine/src/parser/oracle_effect/mod.rs
printf '%s\n' '--- bridge implementation ---'
rg -n -B5 -A25 'pub.*fn nom_on_lower|fn nom_on_lower' crates/engine/src/parser/oracle_nom

Repository: phase-rs/phase

Length of output: 50371

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- TextPair implementation and split behavior ---'
rg -n -B8 -A45 'pub struct TextPair|struct TextPair|impl.*TextPair|fn split_around' crates/engine/src/parser/oracle_util.rs crates/engine/src/parser
printf '%s\n' '--- construction of TextPair for effect parsing ---'
rg -n -B8 -A12 'TextPair::new|TextPair \{' crates/engine/src/parser/oracle_effect/mod.rs | head -n 240
printf '%s\n' '--- bridge implementation only ---'
rg -n 'nom_on_lower' crates/engine/src/parser/oracle_nom/bridge.rs
sed -n '1,180p' crates/engine/src/parser/oracle_nom/bridge.rs
printf '%s\n' '--- focused tests near counter-choice code ---'
rg -n -B10 -A25 'counter.*choice|choice.*counter|from among|shared.noun|Reluctant Role Model|Invoke the Ancients|Aragorn' crates/engine/src/parser/oracle_effect/mod.rs | tail -n 500

Repository: phase-rs/phase

Length of output: 50372

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- list splitting and full counter noun parsers ---'
rg -n -B10 -A45 'fn split_choice_list_items|fn parse_full_counter_noun|parse_full_counter_noun' crates/engine/src/parser/oracle_effect/mod.rs
printf '%s\n' '--- strict counter type parser ---'
rg -n -B10 -A45 'parse_strict_counter_type' crates/engine/src/parser/oracle_nom
printf '%s\n' '--- focused counter-choice tests only ---'
rg -n -B4 -A18 'classify_and_parse_counter_choice_list|counter_choice|from_among|shared_noun|shared.noun' crates/engine/src/parser/oracle_effect/mod.rs

Repository: phase-rs/phase

Length of output: 47094

🏁 Script executed:

#!/bin/bash
set -e
printf '%s\n' '--- choice separator parser ---'
rg -n -B8 -A18 'fn parse_choice_list_separator' crates/engine/src/parser/oracle_effect/mod.rs
printf '%s\n' '--- parser entry points and lowercase contract ---'
rg -n -B8 -A18 'pub.*parse_effect|fn parse_effect|parse_effect_chain' crates/engine/src/parser/oracle_effect/mod.rs | head -n 180
printf '%s\n' '--- relevant exact call-site context ---'
sed -n '5565,5592p' crates/engine/src/parser/oracle_effect/mod.rs

Repository: phase-rs/phase

Length of output: 10720

🏁 Script executed:

#!/bin/bash
set -e
python3 - <<'PY'
# Read-only behavioral probe of the exact case-sensitive dispatch contract shown
# in mod.rs. This models only tag-based dispatch and separator splitting; it
# does not execute repository code.
SEPARATORS = [", or ", ", and ", ", ", " or ", " and "]

def split_items(s):
    items = []
    start = 0
    i = 0
    while i < len(s):
        sep = next((x for x in SEPARATORS if s.startswith(x, i)), None)
        if sep:
            items.append(s[start:i])
            i += len(sep)
            start = i
        else:
            i += 1
    items.append(s[start:])
    return items

def classify_shape(s):
    prefix = "a counter from among "
    if s.startswith(prefix):
        return "FromAmong", split_items(s[len(prefix):])
    # Structural approximation of recognize_shared_noun_counter_list:
    if s.startswith("a ") and s.endswith(" counter"):
        body = s[2:-len(" counter")]
        items = split_items(body)
        if len(items) >= 2:
            return "SharedNoun", items
    return "Distributed", split_items(s)

cases = [
    "a counter from among first strike, vigilance, deathtouch, and lifelink",
    "a Counter from among first strike, vigilance, deathtouch, and lifelink",
    "a menace, trample, reach, or haste counter",
    "A menace, trample, reach, or haste counter",
]
for case in cases:
    print(f"{case!r} -> {classify_shape(case)}")

assert classify_shape(cases[0])[0] == "FromAmong"
assert classify_shape(cases[1])[0] != "FromAmong"
assert classify_shape(cases[2])[0] == "SharedNoun"
assert classify_shape(cases[3])[0] != "SharedNoun"
PY

Repository: phase-rs/phase

Length of output: 688

Make counter-choice parsing case-insensitive.

try_parse_put_counter_choice passes original-case text to case-sensitive classifier and counter parsers. Mixed-case lists can select Distributed incorrectly or fail validation. Run grammar parsing on lowered slices and preserve original-case text only for display.

🤖 Prompt for AI Agents 
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@crates/engine/src/parser/oracle_effect/mod.rs` around lines 5450 - 5482,
Update try_parse_put_counter_choice and classify_counter_choice_list so grammar
detection and counter validation use lowercased input/item slices, while
retaining the original text for returned display values. Ensure mixed-case “from
among,” shared-noun, and distributed lists classify and validate identically to
lowercase input without changing output casing.

Source: Path instructions

/// CR 122.1 + CR 608.2d: Context-free classifier for a disjunctive
/// counter-choice list. Given the choices payload (the text BETWEEN
/// "your choice of " and " on TARGET"), recognize which of the three list
Expand Down Expand Up @@ -5458,50 +5509,15 @@ fn recognize_shared_noun_counter_list(input: &str) -> Option<Vec<&str>> {
pub(crate) fn classify_and_parse_counter_choice_list(
choices_text: &str,
) -> Option<Vec<(CounterType, QuantityExpr)>> {
let (shape, choice_items) =
match tag::<_, _, OracleError<'_>>("a counter from among ")(choices_text) {
Ok((rest, _)) => (ChoiceListShape::FromAmong, split_choice_list_items(rest)?),
// CR 122.1b: keyword counters distribute over a single noun; only
// classify as SharedNoun when the shape matches AND every item is a
// recognized counter type — otherwise distributed lists and
// non-counter lists leak through. Fall through to Distributed when
// the strict guard fails.
Err(_) => match recognize_shared_noun_counter_list(choices_text) {
Some(items)
if items.len() >= 2
&& items.iter().all(|item| {
all_consuming(nom_primitives::parse_strict_counter_type)
.parse(item.trim())
.is_ok()
}) =>
{
(ChoiceListShape::SharedNoun, items)
}
_ => (
ChoiceListShape::Distributed,
split_choice_list_items(choices_text)?,
),
},
};

if choice_items.len() < 2 {
return None;
}
let (shape, choice_items) = classify_counter_choice_list(choices_text)?;

let mut entries: Vec<(CounterType, QuantityExpr)> = Vec::with_capacity(choice_items.len());
for item in &choice_items {
let item = item.trim();
if item.is_empty() {
return None;
}
let entry = match shape {
// CR 122.1: full counter noun phrase ("a +1/+1 counter", "two charge
// counters"). Parse count then counter type from the remainder.
ChoiceListShape::Distributed => {
let (count, rest) = parse_count_expr(item)?;
let (_after, counter_type) = nom_primitives::parse_counter_type_typed(rest).ok()?;
(counter_type, count)
}
ChoiceListShape::Distributed => parse_full_counter_noun(item)?,
// CR 122.1b: bare keyword name ("first strike"); count is one.
ChoiceListShape::FromAmong | ChoiceListShape::SharedNoun => {
let (_rest, counter_type) =
Expand Down Expand Up @@ -5549,83 +5565,32 @@ fn try_parse_put_counter_choice(
// (Reluctant Role Model: "put a flying, lifelink, or +1/+1 counter on it").
// Both resolve to the same `ChooseOneOf` of `PutCounter` branches — the
// controller still picks one kind at resolution. The bare form is allowed
// ONLY for the strictly-validated SharedNoun/FromAmong shapes (every item
// must name a real counter type), so noun-phrase disjunctions like "put a
// creature or a land into play" never misclassify as a counter choice.
let explicit_choice;
// only when every distributed item is a complete counter noun phrase, so
// noun-phrase disjunctions like "put a creature or a land into play" never
// misclassify as a counter choice.
let after_choice_original = if let Some(((), rest)) = nom_on_lower(tp.original, tp.lower, |i| {
value((), tag("put your choice of ")).parse(i)
}) {
explicit_choice = true;
rest
} else {
explicit_choice = false;
nom_on_lower(tp.original, tp.lower, |i| value((), tag("put ")).parse(i))?.1
};

let consumed = tp.original.len() - after_choice_original.len();
let after_choice = TextPair::new(after_choice_original, &tp.lower[consumed..]);
let (choices_tp, target_tp) = after_choice.split_around(" on ")?;

// Split the post-"on" choices into individual items via nom combinators.
// Three list shapes (CR 122.1 + CR 608.2d), classified in priority order:
// 1. FromAmong — "a counter from among X, Y, ..., and Z" (bare keywords)
// 2. SharedNoun — "a X, Y, ..., or Z counter" (one leading article + bare
// keyword adjectives + one trailing "counter")
// 3. Distributed — "a A counter, a B counter, or a C counter" / binary
// ("a A counter or a B counter"), each item a full counter noun phrase.
// CR 122.1b: both FromAmong and SharedNoun name bare keywords; each branch
// is later synthesized as "a <keyword> counter".
// The shared classifier validates FromAmong, SharedNoun, and Distributed
// lists before branch reparsing. In particular, a bare distributed list is
// accepted only when every item is a complete counter noun phrase.
let choices_text = choices_tp.original;
let (shape, choice_items) =
match tag::<_, _, OracleError<'_>>("a counter from among ")(choices_text) {
Ok((rest, _)) => (ChoiceListShape::FromAmong, split_choice_list_items(rest)?),
// CR 122.1b: keyword counters distribute over a single noun; CR
// 608.2d: choice made at resolution; CR 601.2c: shared target at
// cast. Only classify as SharedNoun when the shape matches AND every
// item is a recognized counter type — otherwise distributed lists
// ("a +1/+1 counter, ...") and non-counter lists ("a red or blue
// creature") would leak through. Fall through to Distributed when
// the strict guard fails.
Err(_) => match recognize_shared_noun_counter_list(choices_text) {
Some(items)
if items.len() >= 2
&& items.iter().all(|item| {
all_consuming(nom_primitives::parse_strict_counter_type)
.parse(item.trim())
.is_ok()
}) =>
{
(ChoiceListShape::SharedNoun, items)
}
// The bare "put <list> on ..." form has no "your choice of"
// disambiguator, so it must NOT fall through to the permissive
// Distributed shape — that would let arbitrary "A or B" noun
// phrases reach the counter-branch builder. Require the strict
// SharedNoun/FromAmong shapes for the bare form.
_ if !explicit_choice => return None,
_ => (
ChoiceListShape::Distributed,
split_choice_list_items(choices_text)?,
),
},
};

// Require at least 2 branches.
if choice_items.len() < 2 {
return None;
}
let (shape, choice_items) = classify_counter_choice_list(choices_text)?;

let target_text = target_tp.original.trim().trim_end_matches('.');
if target_text.is_empty() {
return None;
}

// Validate each choice item is non-empty.
if choice_items.iter().any(|item| item.trim().is_empty()) {
return None;
}

// Parse each branch as "put <choice> on <target>".
let diagnostics_snapshot = ctx.diagnostics.len();
// Parse each branch as "put <choice> on <target>" so existing counter
Expand Down
114 changes: 114 additions & 0 deletions crates/engine/src/parser/oracle_effect/tests.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -39761,6 +39761,88 @@ fn choose_one_of_detects_shared_target_counter_choice() {
}
}

#[test]
fn dwarven_armorer_bare_distributed_counter_choice_preserves_cost_and_branches() {
use crate::types::counter::CounterType;

let parsed = parse_oracle_text(
"{R}, {T}, Discard a card: Put a +0/+1 counter or a +1/+0 counter on target creature.",
"Dwarven Armorer",
&[],
&["Creature".to_string()],
&["Dwarf".to_string()],
);

assert_eq!(
parsed.abilities.len(),
1,
"Dwarven Armorer must produce exactly one activated ability: {:#?}",
parsed.abilities
);
let ability = &parsed.abilities[0];
assert_eq!(ability.kind, AbilityKind::Activated);

let AbilityCost::Composite { costs } = ability
.cost
.as_ref()
.expect("Dwarven Armorer must retain its activation costs")
else {
panic!(
"expected composite mana, tap, discard cost, got {:?}",
ability.cost
);
};
assert_eq!(costs.len(), 3);
assert!(matches!(
&costs[0],
AbilityCost::Mana {
cost: ManaCost::Cost {
shards,
generic: 0,
}
} if shards == &vec![ManaCostShard::Red]
));
assert!(matches!(&costs[1], AbilityCost::Tap));
assert!(matches!(
&costs[2],
AbilityCost::Discard {
count: QuantityExpr::Fixed { value: 1 },
filter: None,
..
}
));

assert!(matches!(&*ability.effect, Effect::TargetOnly { .. }));
let choice = ability
.sub_ability
.as_deref()
.expect("the shared target must lead to the counter-choice sub-ability");
let Effect::ChooseOneOf { chooser, branches } = &*choice.effect else {
panic!(
"expected ChooseOneOf after shared target, got {:?}",
choice.effect
);
};
assert_eq!(*chooser, PlayerFilter::Controller);
assert_eq!(branches.len(), 2);

let expected = [(0, 1), (1, 0)];
for (branch, (power, toughness)) in branches.iter().zip(expected) {
assert!(
matches!(
&*branch.effect,
Effect::PutCounter {
counter_type: CounterType::PowerToughness { power: actual_power, toughness: actual_toughness },
count: QuantityExpr::Fixed { value: 1 },
target: TargetFilter::ParentTarget,
} if (*actual_power, *actual_toughness) == (power, toughness)
),
"expected +{power}/+{toughness} ParentTarget counter branch, got {:?}",
branch.effect
);
}
}

#[test]
fn choose_one_of_detects_from_among_counter_choice() {
use crate::types::counter::CounterType;
Expand Down Expand Up @@ -39974,6 +40056,38 @@ fn classify_counter_choice_list_rejects_non_counter_and_singletons() {
);
}

#[test]
fn bare_distributed_counter_choice_rejects_non_counter_noun_disjunction() {
// Positive reach guard: the bare distributed path is live and fully
// supported before the hostile phrase is checked.
let valid = parse_effect_chain(
"Put a +0/+1 counter or a +1/+0 counter on target creature.",
AbilityKind::Spell,
);
assert!(
matches!(&*valid.effect, Effect::TargetOnly { .. })
&& valid
.sub_ability
.as_deref()
.is_some_and(|sub| matches!(&*sub.effect, Effect::ChooseOneOf { branches, .. } if branches.len() == 2)),
"a valid bare distributed counter list must reach ChooseOneOf: {valid:?}"
);

let hostile = parse_effect_chain(
"Put a red or blue creature on target creature.",
AbilityKind::Spell,
);
let is_counter_choice = matches!(&*hostile.effect, Effect::TargetOnly { .. })
&& hostile.sub_ability.as_deref().is_some_and(|sub| {
matches!(&*sub.effect, Effect::ChooseOneOf { branches, .. }
if branches.iter().all(|branch| matches!(&*branch.effect, Effect::PutCounter { .. })))
});
assert!(
!is_counter_choice,
"bare non-counter noun disjunction must not reach counter branches: {hostile:?}"
);
}

#[test]
fn shared_noun_counter_choice_rejects_non_counter_list() {
// "a red or blue creature" is a noun-phrase disjunction, not a counter
Expand Down
Loading