fix(parser): UNSUPPORTED cluster: CHOOSE-DISPATCHER seam — choose residual Unimplemented (choose N#4700
Conversation
…esidual Unimplemented (choose N Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…-unsupported-cluster-choose-dispatcher
![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 implements interactive counter-kind selection and placement (CR 608.2d, CR 122.1, CR 122.6) to support cards like 'The Caves of Androzani', and introduces tracked-set membership filtering (CR 608.2c) for cards like 'The Day of the Doctor IV'. The review feedback focuses on enforcing strict CR 122.6 target validation to ensure counters are only added to permanents already possessing that counter kind, along with adding a corresponding unit test. Additionally, the feedback recommends improving parser robustness and sibling coverage by refactoring the new parser arms to use modular, reusable combinators rather than duplicating inline string matches.
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 122.1 + CR 122.6: Delegate to the single counter-placement authority. | ||
| // The synthetic `PutCounter` inherits the resolving ability's targets so a | ||
| // `ParentTarget` resolves to the current `repeat_for` iteration object. | ||
| let mut synthetic = ability.clone(); | ||
| synthetic.sub_ability = None; | ||
| synthetic.effect = Effect::PutCounter { | ||
| counter_type, | ||
| count, | ||
| target, | ||
| }; | ||
| crate::game::effects::counters::resolve_add(state, &synthetic, events) |
There was a problem hiding this comment.
[HIGH] Enforce CR 122.6 target validation.
Why it matters: Under CR 122.6, if an effect instructs a player to put an additional counter of a certain kind on a permanent, that permanent must already have at least one counter of that kind on it. If it doesn't, the effect does nothing. Simply delegating to Effect::PutCounter will add the counter regardless of whether the target currently has it (e.g., if the counter was removed in response or if targets differ).
Suggested fix: Filter synthetic.targets to only include objects that currently possess at least one counter of the chosen kind, using is_some_and to avoid unnecessary map_or lints.
let mut synthetic = ability.clone();
synthetic.sub_ability = None;
synthetic.effect = Effect::PutCounter {
counter_type: counter_type.clone(),
count,
target,
};
// CR 122.6: "put an additional counter of that kind... that permanent must
// already have at least one counter of that kind on it." Filter targets to
// only those that currently possess the chosen counter kind.
synthetic.targets.retain(|t| match t {
crate::types::ability::TargetRef::Object(id) => state
.objects
.get(id)
.is_some_and(|obj| obj.counters.get(&counter_type).copied().unwrap_or(0) > 0),
crate::types::ability::TargetRef::Player(_) => false,
});
if synthetic.targets.is_empty() {
events.push(GameEvent::EffectResolved {
kind: EffectKind::from(&ability.effect),
source_id: ability.source_id,
});
return Ok(());
}
crate::game::effects::counters::resolve_add(state, &synthetic, events)References
- Strict fidelity to the MTG Comprehensive Rules (CR) — every game rule, validation, and computed value matches the CR exactly. (link)
| #[test] | ||
| fn no_chosen_kind_is_noop() { | ||
| let (mut state, source, target) = setup(); | ||
| state | ||
| .objects | ||
| .get_mut(&target) | ||
| .unwrap() | ||
| .counters | ||
| .insert(CounterType::Plus1Plus1, 1); | ||
| let before = state.objects[&target].counters.clone(); | ||
|
|
||
| let mut events = Vec::new(); | ||
| resolve(&mut state, &ability(source, target), &mut events).unwrap(); | ||
| assert_eq!( | ||
| state.objects[&target].counters, before, | ||
| "no chosen kind → no counters added" | ||
| ); | ||
| } | ||
| } |
There was a problem hiding this comment.
[MED] Add unit test for CR 122.6 target filtering.
Why it matters: Verifying that a target without the chosen counter kind is correctly skipped ensures regression safety for CR 122.6 compliance.
Suggested fix: Add target_without_counter_is_skipped test.
#[test]
fn no_chosen_kind_is_noop() {
let (mut state, source, target) = setup();
state
.objects
.get_mut(&target)
.unwrap()
.counters
.insert(CounterType::Plus1Plus1, 1);
let before = state.objects[&target].counters.clone();
let mut events = Vec::new();
resolve(&mut state, &ability(source, target), &mut events).unwrap();
assert_eq!(
state.objects[&target].counters, before,
"no chosen kind → no counters added"
);
}
/// CR 122.6: When the target permanent does not have any counters of the
/// chosen kind, the put is a no-op — no counters are added.
#[test]
fn target_without_counter_is_skipped() {
let (mut state, source, target) = setup();
state
.objects
.get_mut(&source)
.unwrap()
.chosen_attributes
.push(ChosenAttribute::Counter(CounterType::Stun));
let before = state.objects[&target].counters.clone();
let mut events = Vec::new();
resolve(&mut state, &ability(source, target), &mut events).unwrap();
assert_eq!(
state.objects[&target].counters, before,
"target does not have Stun counter → no counters added"
);
}
}| pub(super) fn try_parse_put_chosen_counter(input: &str) -> Option<Effect> { | ||
| let parsed = |i| -> OracleResult<'_, ()> { | ||
| let (i, _) = opt(alt((tag("an "), tag("a ")))).parse(i)?; | ||
| let (i, _) = opt(tag("additional ")).parse(i)?; | ||
| let (i, _) = tag("counter of that kind on ").parse(i)?; | ||
| let (i, _) = alt((tag("it"), tag("that permanent"), tag("that creature"))).parse(i)?; | ||
| let (i, _) = opt(tag(".")).parse(i)?; | ||
| let (i, _) = eof.parse(i)?; | ||
| Ok((i, ())) | ||
| }; |
There was a problem hiding this comment.
[MED] Expand parser sibling coverage for put-chosen-counter using modular parsers.
Why it matters: The parser currently only matches "a counter" or "an additional counter" on "it", "that permanent", or "that creature". Supporting "another" and more target types like "that player" or "that permanent or player" ensures robustness and sibling coverage (L2). To prevent combinatorial explosion and maintain clean code, we should decompose these subjects into a modular, reusable parser rather than duplicating inline tags.
Suggested fix: Update the parser combinators to support "another" and use a shared parse_subject parser.
| pub(super) fn try_parse_put_chosen_counter(input: &str) -> Option<Effect> { | |
| let parsed = |i| -> OracleResult<'_, ()> { | |
| let (i, _) = opt(alt((tag("an "), tag("a ")))).parse(i)?; | |
| let (i, _) = opt(tag("additional ")).parse(i)?; | |
| let (i, _) = tag("counter of that kind on ").parse(i)?; | |
| let (i, _) = alt((tag("it"), tag("that permanent"), tag("that creature"))).parse(i)?; | |
| let (i, _) = opt(tag(".")).parse(i)?; | |
| let (i, _) = eof.parse(i)?; | |
| Ok((i, ())) | |
| }; | |
| pub(super) fn try_parse_put_chosen_counter(input: &str) -> Option<Effect> { | |
| let parsed = |i| -> OracleResult<'_, ()> { | |
| let (i, _) = opt(alt((tag("an "), tag("a "), tag("another ")))).parse(i)?; | |
| let (i, _) = opt(alt((tag("additional "), tag("another ")))).parse(i)?; | |
| let (i, _) = tag("counter of that kind on ").parse(i)?; | |
| let (i, _) = parse_subject.parse(i)?; | |
| let (i, _) = opt(tag(".")).parse(i)?; | |
| let (i, _) = eof.parse(i)?; | |
| Ok((i, ())) | |
| }; |
References
- L2. Sibling coverage: If a parser arm or string was extended, are plural / possessive / negated / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' variants covered? (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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
| let anaphor = |i| -> OracleResult<'_, ()> { | ||
| let (i, _) = tag("a counter on ").parse(i)?; | ||
| let (i, _) = alt((tag("it"), tag("that permanent"), tag("that creature"))).parse(i)?; | ||
| let (i, _) = opt(tag(".")).parse(i)?; | ||
| let (i, _) = eof.parse(i)?; | ||
| Ok((i, ())) | ||
| }; |
There was a problem hiding this comment.
[MED] Expand parser sibling coverage for choose-counter-kind using modular parsers.
Why it matters: The parser currently only matches "a counter on it", "that permanent", or "that creature". Supporting more target types like "that player" or "that permanent or player" ensures robustness and sibling coverage (L2). To maintain clean code and prevent duplication, we should decompose these subjects into a modular, reusable parser.
Suggested fix: Update the parser combinators to use a shared parse_subject parser.
| let anaphor = |i| -> OracleResult<'_, ()> { | |
| let (i, _) = tag("a counter on ").parse(i)?; | |
| let (i, _) = alt((tag("it"), tag("that permanent"), tag("that creature"))).parse(i)?; | |
| let (i, _) = opt(tag(".")).parse(i)?; | |
| let (i, _) = eof.parse(i)?; | |
| Ok((i, ())) | |
| }; | |
| let anaphor = |i| -> OracleResult<'_, ()> { | |
| let (i, _) = tag("a counter on ").parse(i)?; | |
| let (i, _) = parse_subject.parse(i)?; | |
| let (i, _) = opt(tag(".")).parse(i)?; | |
| let (i, _) = eof.parse(i)?; | |
| Ok((i, ())) | |
| }; |
References
- L2. Sibling coverage: If a parser arm or string was extended, are plural / possessive / negated / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' variants covered? (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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
Parse changes introduced by this PR · 5 card(s), 10 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The declared-target counter-kind parser accepts the choice head but not the documented another counter of that kind consumer. Evidence: crates/engine/src/parser/oracle_effect/imperative.rs:3203 and crates/engine/src/parser/oracle_effect/counter.rs:318. Why it matters: cards in the documented Ichormoon Gauntlet-style class can parse ChooseCounterKind { target: target permanent }, but their follow-up Put another counter of that kind on that permanent or remove one from it is not covered by the new PutChosenCounter parser, so the PR partially recognizes the class while leaving its main consumer unsupported. Suggested fix: either add the declared-target another counter ... on that permanent/remove-one branch with tests, or remove the declared-target parser/comment so coverage remains an honest unsupported gap.
Reviewed current head 2844f272b2ffedfc138d414f99c8530e3a1b311a.
2 participants
Summary
Fixes a parser misparse affecting 3 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED cluster: CHOOSE-DISPATCHER seam —
chooseresidual Unimplemented (choose N of a set, then act per chosen)Cards corrected
Fix
Implemented PR 1 of the WHO cluster-82 "choose"-seam plan: Part A (The Day of the Doctor IV) and Part B (The Caves of Androzani II/III) are now fully supported (0 Unimplemented/Unknown). Part C (Caught in a Parallel Universe) is deferred to a separate PR per the approved plan — it remains an honest Unimplemented (the per-player-choose→per-player-act seam + ControllerRef::Neighbor is a genuine multi-layer effort, build plan in the plan's §C.2).
PART A — added FilterProp::InTrackedSet{id} (types/ability.rs) with the authoritative chain-first sentinel-resolution matcher in game/filter.rs (mirroring the TrackedSetFiltered id.0==0 precedent), plus arms in the other 4 exhaustive FilterProp matchers, coverage render, and the AI memo-safety classifier. Parser: a post-pass over the assembled AbilityDefinition converts an Unimplemented{choose}+multi_target head into Effect::ChooseObjectsIntoTrackedSet and rewrites the following mass-move's FilterProp::Another → Not(InTrackedSet(0)) ("Choose up to three Doctors. You may exile all other creatures."). The reviewed plan proposed a parse-time detector, but "up to N" is pre-stripped into multi_target before the choose dispatch runs, so I used the already-extracted multi_target in the post-pass instead. Reuses the existing ChooseObjectsIntoTrackedSet resolver/AI/UI unchanged.
PART B — decomposed "choose a counter on it … put an additional counter of that kind" onto two new effects: Effect::ChooseCounterKind{target} (resolver reuses WaitingFor::NamedChoice via a new ChoiceType::CounterKind{options} baked at resolution — 0 kinds skip, 1 auto-select, ≥2 prompt; persists ChosenAttribute::Counter) and Effect::PutChosenCounter{target,count} (reads the chosen kind, delegates to the single counters::resolve_add authority via a synthetic PutCounter). Also added ChoiceValue::Counter and wired compute_options/from_choice/choice_type/bind_named_choice (replace-on-rechoose) + all exhaustive Effect/EffectKind/ChoiceType matches across engine + phase-ai. Parser: try_parse_choose_counter_kind (anaphor→ParentTarget and declared-target forms) and try_parse_put_chosen_counter, with a ZoneCounterImperativeAst::PutChosenCounter seam. Frontend: NamedChoiceModal title mapping + game.json i18n key (chrome via t(); CounterType option labels stay raw pass-through; choice_type is typed generically so no TS union change).
DEVIATION (superior reuse): Part B reuses WaitingFor::NamedChoice + ChoiceType::CounterKind rather than the plan's proposed new WaitingFor::ChooseCounterKind — eliminating a new multiplayer filter, AI candidate arm, and frontend modal while honoring the plan's building-block-reuse mandate. Runtime proven by a GameScenario+GameRunner integration test (re-hosted Caves chapter II ability: repeat_for + ChooseCounterKind auto-select + optional PutChosenCounter → Stun 1→2).
BONUS (same class): Ichormoon Gauntlet ("choose a counter on target permanent; put an additional counter of that kind") is now fully supported (0 Unimplemented) via the same primitives, after making the resolver enumerate the ability's bound targets (so a declared "target permanent" scopes to that object, not the whole battlefield).
COLLATERAL SAFETY: the converter is gated on the exact "affect all other X" signature (a sub-ability ChangeZoneAll/DestroyAll carrying FilterProp::Another), so duneblast/mount doom ("Destroy the rest") and The Three Seasons ("choose … cards in each graveyard") are NOT mis-converted and stay at their prior honest Unimplemented rather than becoming silent misparses. animation module / aven courier are net improvements (choose head now correct ChooseCounterKind; their differently-worded consume remains a documented Unimplemented).
Verification (worktree runs cargo directly): cargo fmt --all clean; cargo clippy --all-targets -D warnings clean (workspace incl. phase-ai); full cargo test -p engine green (14416 lib + all integration; 7 new tests pass); check-parser-combinators.sh shows only the pre-existing whole-repo baseline (none in my helpers); parser diff gate clean (no contains/starts_with/find in added lines); gen-card-data.sh confirms the fixes; cargo semantic-audit flags none of my cards; client pnpm type-check + eslint pass. Restored crates/engine/data/known-tokens.toml (a dataset-drift regeneration artifact from the verification gen-card-data run, not a code change); left the pre-existing oracle-subtypes.json Shi-Ar change (another agent's) untouched. Did NOT commit.
RISKS for /review-impl: (1) The repeat_for+NamedChoice pause/resume is integration-proven only for the single-kind auto-select path; the ≥2-kind mid-iteration NamedChoice answer is unit-tested at the resolver but not in the full repeat integration. (2) animation module's ChooseCounterKind targets Or[Permanent,Player] and the resolver skips Player targets (players' counters not enumerated) — partial support, consume Unimplemented anyway. (3) The converter's Another-gate means a future "choose N X, affect all other X" card using a non-ChangeZoneAll/DestroyAll mass-move won't convert (stays Unimplemented — safe).
Files changed
CR references
Verification
cargo fmt --all— clean (exit 0, no files changed; nothing to commit)check-parser-combinators.sh (scoped to upstream/main merge-base d937775ea)— pass (exit 0, no output; no nom-combinator flags in this fix's changed lines)cargo clippy -p engine --all-targets -- -D warnings— pass (exit 0, zero warnings; fix's changed files clippy-clean, no pre-existing debt surfaced)cargo test -p engine— pass (136 'test result: ok' lines, 0 failed, EXIT 0; includes the fix's legendary-supertype and Caught-in-a-Parallel-Universe deferred-gap locking tests)oracle-gen (caught in a parallel universe | the caves of androzani | the day of the doctor)— pass (all three ASTs inspected and confirmed vs oracle text)Cards confirmed re-parsed correctly: The Caves of Androzani — fully fixed: chapters II/III parse to ChooseCounterKind(ParentTarget) -> PutChosenCounter(count:1, optional) with repeat_for ObjectCount(Permanent, Non(Subtype:Saga)); chapter I PutCounter stun x2 on <=2 tapped creatures; chapter IV SearchLibrary(Doctor)->Hand->Shuffle. Zero Unimplemented., The Day of the Doctor — fully fixed: chapters I/II/III parse to ExileFromTopUntil{NextMatches filter Typed[Card, HasSupertype: Legendary]} (supertype correctly scoped, not Any) -> CastFromZone(Play, UntilHostLeavesPlay) -> PutAtLibraryPosition(TrackedSet 0, Bottom); chapter IV ChooseObjectsIntoTrackedSet(Doctor, 0..3) -> optional ChangeZoneAll->Exile of creatures Not(InTrackedSet 0) -> DealDamage 13 gated on OptionalEffectPerformed. Zero Unimplemented., Caught in a Parallel Universe — confirmed correct as an intentional, test-locked deferred gap: encounter execute head stays Unimplemented{name:'choose'} (per-player left-neighbor parallel many-to-many selection needs infrastructure the engine lacks); trigger maps to PlaneswalkedTo and the CopyTokenOf+Menace tail parses. AST matches the state the fix's own test locks, so it is confirmed correct (deliberately not implemented with single-chooser machinery).
🤖 Generated with Claude Code