fix(parser): UNSUPPORTED cluster: Player voting — secret/open votes with per-label tallies (Effect::#4402
Conversation
…ith per-label tallies (Effect::Vote) Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
…-unsupported-cluster-player-voting
![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 introduces support for parsing and resolving secret votes and trailing-suffix aggregate vote clauses, specifically handling the card 'Truth or Consequences' and its card-name/choice-list self-reference. It adds the ability to hoist random opponent selection setups to wrap votes, retargets player-directed effects to the chosen opponent, and handles bare plural draw card residues. Feedback on the changes notes that the helper function retarget_that_player_to_chosen is missing a mandatory CR <number>: <description> annotation in its documentation, violating style guide rule R6.
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.
| /// Retarget a `"that player"` anaphor (`TargetFilter::TriggeringPlayer`) to the | ||
| /// persisted chosen player (`TargetFilter::SourceChosenPlayer`) on a | ||
| /// player-directed effect. Used after a random "choose an opponent" setup is | ||
| /// hoisted to wrap the Vote: the damage clause's recipient is the chosen | ||
| /// opponent, recorded as `ChosenAttribute::Player` and resolved by | ||
| /// `deal_damage::player_context_target`. Only `Effect::DealDamage` carries this | ||
| /// anaphor in the suffix-vote class today; extend with new arms as new shapes | ||
| /// ship. |
There was a problem hiding this comment.
The rules-touching helper function retarget_that_player_to_chosen is missing a mandatory CR <number>: <description> annotation in its documentation comment, violating style guide rule R6.
| /// Retarget a `"that player"` anaphor (`TargetFilter::TriggeringPlayer`) to the | |
| /// persisted chosen player (`TargetFilter::SourceChosenPlayer`) on a | |
| /// player-directed effect. Used after a random "choose an opponent" setup is | |
| /// hoisted to wrap the Vote: the damage clause's recipient is the chosen | |
| /// opponent, recorded as `ChosenAttribute::Player` and resolved by | |
| /// `deal_damage::player_context_target`. Only `Effect::DealDamage` carries this | |
| /// anaphor in the suffix-vote class today; extend with new arms as new shapes | |
| /// ship. | |
| /// CR 608.2c: Retarget a "that player" anaphor (TargetFilter::TriggeringPlayer) to the | |
| /// persisted chosen player (TargetFilter::SourceChosenPlayer) on a | |
| /// player-directed effect. Used after a random "choose an opponent" setup is | |
| /// hoisted to wrap the Vote: the damage clause's recipient is the chosen | |
| /// opponent, recorded as ChosenAttribute::Player and resolved by | |
| /// deal_damage::player_context_target. Only Effect::DealDamage carries this | |
| /// anaphor in the suffix-vote class today; extend with new arms as new shapes | |
| /// ship. |
References
- Every rules-touching line of engine code must carry a comment of the form
CR <number>: <description>(regexCR \d{3}(\.\d+[a-z]?)?). (link)
Parse changes introduced by this PR · 161 card(s), 136 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
I reviewed current head 46f384709e759f53febe1bae97dff4cb5e69d4ee via the GitHub API diff, PR comments/reviews, parse-diff output, local worktree, and local CR/card-text lookup.
The architectural direction is close for the open-vote aggregate suffix and Truth-or-Consequences tally pieces, but this cannot be approved as-is.
[HIGH] Secret votes are modeled as ordinary public votes. Evidence: parse_each_player_votes_clause maps each player secretly votes for onto the same VoterScope::AllPlayers / normal Effect::Vote path at crates/engine/src/parser/oracle_vote.rs:432-439, with the comment saying secrecy is presentation-only. The existing WaitingFor::VoteChoice state carries live tallies and ballots at crates/engine/src/types/game_state.rs:3910-3920, and every ChooseOption immediately increments new_tallies, appends (player, choice_index), and emits GameEvent::VoteCast { choice } at crates/engine/src/game/engine_resolution_choices.rs:1231-1243; the next VoteChoice then stores those updated public values at lines 1248-1255. Why it matters: Truth or Consequences and Elrond both instruct players to vote secretly and then reveal the votes. This implementation exposes each prior vote before all players have voted, so it does not preserve the Oracle instruction even though exact-head CI is green. Suggested fix: add a typed secret/open vote axis or separate waiting state so secret ballots are collected without public tallies/choice events until the reveal/completion step, or leave the secret-vote cards unsupported until that engine seam exists.
[MEDIUM] Gemini's CR-annotation finding is valid. retarget_that_player_to_chosen performs the CR 608.2c/CR 120.1 player retargeting rule, but the helper's own doc/comment block at crates/engine/src/parser/oracle_vote.rs:821-827 has no CR <number>: <description> annotation. The surrounding call site has rule context, but this rule-implementing helper should carry the required local annotation before landing.
No approval, label, or enqueue from this sweep.
HIGH: Drop secret-ballot parsing from parse_each_player_votes_clause. The existing WaitingFor::VoteChoice state immediately increments public tallies and emits VoteCast events after each ballot, which would expose each player's choice before the reveal step mandated by the Oracle text of "each player secretly votes for … then those votes are revealed." Per @matthewevans review: leave secret-vote cards (Truth or Consequences, Elrond of the White Council, Orchard Elemental) as Effect::Unimplemented until a proper hidden-ballot engine seam is added. Removed satellite code introduced alongside the secret-vote arm: - take_until(", then those votes are revealed") strip in clause parser - card_name expansion (seg.replace('~', card_name)) for T/C name pun - card_name parameter on parse_vote_block / parse_each_player_votes_clause - Renamed oracle_saga.rs param back to _card_name (was unused) - Updated all callers in oracle.rs, oracle_trigger.rs, oracle_saga.rs, game/effects/vote.rs, and all tests KEPT: parse_vote_for_each_suffix_clause, pre_vote_choose hoisting, and retarget_that_player_to_chosen — these are architecturally valid building blocks for public-vote cards with suffix aggregate clauses. MEDIUM: Add mandatory CR 608.2c annotation to retarget_that_player_to_chosen per @gemini-code-assist and @matthewevans. CR 608.2c covers "later text on the card may modify the meaning of earlier text" — the "that player" anaphor in the damage clause refers back to the player chosen by the preceding "choose an opponent at random" instruction. Tests updated: - parser/oracle_vote.rs: replaced T/C secret-vote assertion tests with tests that assert None (secret votes intentionally unsupported) - game/effects/vote.rs: updated T/C runtime test to use a public-vote opener, exercising the same Choose→Vote→SourceChosenPlayer machinery Verification: - cargo fmt --all: clean - cargo clippy -p engine --all-targets -- -D warnings: clean - cargo test -p engine: all pass - ./scripts/check-parser-combinators.sh: exit 0 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
|
Current head |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head a405362 after the follow-up review and repush guard. Prior blockers are addressed: secret-ballot wording now remains unsupported instead of being treated as public voting, the that-player retarget helper has the verified CR 608.2c annotation, and the retained public-vote suffix path has parser plus runtime Choose -> Vote -> draw/damage coverage. Exact-head CI is green and GitHub reports CLEAN.
matthewevans
added
bug
2 participants
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED cluster: Player voting — secret/open votes with per-label tallies (Effect::Vote runtime exists; wire parser)
Cards corrected
Fix
Implemented cluster-36 (Truth or Consequences — secret-council vote) exactly per the approved, re-grounded plan. The card was a single
Effect::Unimplementedon upstream/main; it now parses to a fully-supported, rules-correct typed tree with ZERO Unimplemented:Choose{ChoiceType::Opponent{None}, persist:true, selection:Random}→ sub_abilityVote{choices:["truth","consequences"], tally_mode:PerVote}whose per-choice slots areDraw{count: VoteCount{0}, target: Controller}(truth) andDealDamage{amount: Multiply{3, VoteCount{1}}, target: SourceChosenPlayer}(consequences). Verified end-to-end via oracle-gen and a new e2e resolution test.NO new enum variants, NO types/ change, NO engine resolver change, NO deal_damage change — every primitive pre-existed (the plan's empirical re-grounding was correct; the prior BLOCKER findings were against a stale tree). The work is purely parser dispatch onto existing handlers, exactly matching the heterogeneous-cluster "dispatch onto existing handlers" scope_note.
Three reusable building blocks added (not card special-cases): (1) a
"each player secretly votes for "secret-ballot opener + nomtake_untilstrip of the no-op "...then those votes are revealed" reveal tail; (2)~-in-choice-list recovery viastr::replace(idempotent — expands the card-name/choice-list pun back to the option list); (3)parse_vote_for_each_suffix_clause— the trailing-suffix sibling of the existing prefixparse_aggregate_tally_clause, covering " for each vote" with an optional hoisted random "[then ]choose a(n) opponent/player at random" setup and a typedTriggeringPlayer→SourceChosenPlayerretarget. (4) imperative.rs bare-plural "draw cards"→Draw{Fixed(1)} head (only fires after the dynamic tail is stripped; strict-failure preserved for unmodeled tails). All detection/dispatch is nom combinators; all data is typed enums (no raw bools).Threaded
card_name: &strthroughparse_vote_block(+ the 3 production dispatch sites in oracle.rs/oracle_trigger.rs/oracle_saga.rs, renaming saga's_card_name→card_name) and all ~21 test call sites.Verification (run directly — this worktree is not under Tilt): cargo fmt clean; cargo clippy -p engine --all-targets clean (no warnings); full
cargo test -p enginepasses with zero failures (48 targeted vote/imperative tests green, including the new e2e);./scripts/check-parser-combinators.shflags NOTHING in my files (its EXIT=1 is entirely pre-existing committed code from other agents — its baseline is the old merge-base ff799f8); the instructions' parser diff gate passes clean; fullgen-card-data.shregenerated 35366 cards with exit 0 and Truth or Consequences shows unimpl_count:0.Risk #1 from the plan (does random Choose park on a WaitingFor?) is RESOLVED both by code inspection (
resolve_ability_chainresolvesEffect::Choose{selection:Random}inline viachoose::resolve_random_in_chain— Strax precedent, never parks) and by the new passing e2e test, which drives the full Choose→Vote chain in a 2-player game, submits both ballots, and asserts the random Choose did not park, the truth tally drove the controller's draw (+1 card), and 3×consequences-tally damage landed on the persisted chosen opponent (−3 life) and not the controller.Tests added (building-block level): oracle_vote.rs — full-card secret vote + recovery, secret-opener unit, suffix-aggregate general (no setup), suffix-aggregate with random-opponent setup + retarget; imperative.rs — bare "draw cards"/"draw card"/"draw cards."→Fixed(1) and a negative guard ("draw cards from the top of their library"→None); vote.rs — the e2e resolution test.
All 6 CR annotations grep-verified against docs/MagicCompRules.txt (CR 102.2 opponent, CR 120.1 damage, CR 121.1 draw, CR 608.2c resolution/read-whole-text, CR 608.2d effect-choice/random-override, CR 701.38 Vote). I deliberately did NOT cite CR 701.9b for "random" as the plan's prose loosely suggested — 701.9b is discard-specific; CR 608.2d is the authorizing rule for resolution-time random selection.
Files changed
CR references
Verification
cargo fmt --all— passcheck-parser-combinators.sh (scoped to upstream/main merge-base 53289c31d)— passcargo clippy -p engine --all-targets -- -D warnings— passcargo test -p engine— passoracle-gen data --filter "truth or consequences"— passCards confirmed re-parsed correctly: Truth or Consequences
🤖 Generated with Claude Code