fix(parser): UNSUPPORTED one-off: Clockspinning#4595
Conversation
…-unsupported-one-off-clockspinning
![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 the ChooseCounterAdjustment effect to support cards like Clockspinning, in compliance with CR 122.1, CR 608.2d, and CR 702.62b. It introduces the ChooseCounterAdjustment effect variant and CounterAdjustment enum, implements the resolution logic by delegating to the existing ChooseOneOf machinery, and adds parser support for target designations and adjustment clauses using nom combinators. The changes are integrated across the engine, including coverage, printed cards, trigger index, and AI policies, and are accompanied by comprehensive unit tests. There are no review comments to address, and we have no additional feedback to provide.
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.
Parse changes introduced by this PR · 18 card(s), 22 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The suspended-card target path is not covered through real target selection. Evidence: crates/engine/src/game/effects/choose_counter_adjustment.rs:281 creates an exiled time-counter object and crates/engine/src/game/effects/choose_counter_adjustment.rs:339 injects it as an already-selected target, while the real parser target leg at crates/engine/src/parser/oracle_target.rs:833 requires the exiled card to satisfy the suspended-card predicate (Exile + Suspend + time counter). Why it matters: Clockspinning can parse and the resolver can work, but a broken cast-time legal-target/revalidation path for suspended cards would still pass these tests. Suggested fix: add a scenario/cast-path test with a real suspended exiled card carrying Suspend + time counter, verify it is offered/accepted as a legal target, then resolve the remove/add branch.
[LOW] The operation axis is broader than the parser currently supports. Evidence: crates/engine/src/types/ability.rs:1076 adds Add, Remove, and AddOrRemove, but crates/engine/src/parser/oracle_effect/imperative.rs:9917 and crates/engine/src/parser/oracle_effect/imperative.rs:9922 recognize only the exact remove-or-put-another form and always return AddOrRemove at crates/engine/src/parser/oracle_effect/imperative.rs:9929. Why it matters: this presents a reusable class while shipping dead variants and a Clockspinning-specific parser seam, so future counter-adjustment forms can appear supported without parser/test coverage. Suggested fix: either narrow the enum/effect to the supported AddOrRemove shape or add parser branches and tests for standalone add/remove and reasonable anaphor/order variants.
[MED] Add real-targeting test for the suspended-card filter path.
The prior tests injected targets directly, bypassing `find_legal_targets`.
The new `suspended_card_is_a_legal_target_through_filter` test creates a
game state with a card in exile that carries both `Keyword::Suspend` (in
`base_keywords`, as required by `off_zone_has_keyword_kind`) and a time
counter, then calls `find_legal_targets` with the parsed Or-filter to
verify the full predicate stack: InZone{Exile} + HasKeywordKind{Suspend} +
Counters{Time ≥ 1}. A bare exiled card without Suspend is also checked as
a negative case. This exercises the real cast-path targeting / revalidation
code that the direct-injection tests bypassed.
[LOW] Make all three CounterAdjustment variants reachable from the parser.
`try_parse_choose_counter_adjustment` previously only returned `AddOrRemove`
(Clockspinning), leaving `Add` and `Remove` as dead variants. The function
is restructured with nom combinators to also recognize:
- Remove-only: "remove that counter [from <anaphor>]" → CounterAdjustment::Remove
- Add-only: "put another of those counters on <anaphor>" → CounterAdjustment::Add
AddOrRemove is tried first (longer form sharing the remove prefix), then
Remove-only, then Add-only. Two new test functions exercise each form
across all four object anaphors ("it", "that permanent", "that card",
"that permanent or card") plus trailing-punctuation variants and negative
cases that must remain None.
Addressed comments:
- matthewevans [MED]: crates/engine/src/game/effects/choose_counter_adjustment.rs:281 / :339
- matthewevans [LOW]: crates/engine/src/types/ability.rs:1076 / imperative.rs:9917/9922/9929
Verification: cargo fmt --all + clippy -p engine -D warnings + cargo test -p engine
(14,151 tests, 0 failures) + check-parser-combinators.sh gate pass.
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
2 participants
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED one-off: Clockspinning
Cards corrected
Fix
Implemented Cluster 68 (Clockspinning) as an in-scope new engine primitive, fully rules-correct, 0 Unimplemented. Buyback already worked (synthesized); the gap was the counter clause.
WHAT SHIPPED
CounterAdjustment { Add, Remove, AddOrRemove }(allows_add/allows_remove helpers, no raw bools) + new slot-lessEffect::ChooseCounterAdjustment { adjustment, count }mirroringChooseOneOfat all 6 registration sites (target_filter/count_expr/count_expr_mut None groups, effect_name, EffectKind, From) — notargetfield (target arrives via the propagatedability.targetschain, so it is NOT surfaced by collect_target_slots).ability.targets, reads kinds present viapositive_counter_types(sorted by as_str for determinism), builds a FLATEffect::ChooseOneOfof concrete PutCounter/RemoveCounter ParentTarget branches (one per present kind x allowed op), and delegates tochoose_one_of::resolve— reusing the entire ChooseOneOfBranch interactive surface (WaitingFor/GameAction/AI/multiplayer/frontend) with ZERO new surface. Zone-agnostic counter mutation confirmed for suspended cards in exile.try_parse_choose_counter_adjustment("remove that counter [from X] or put another of those counters on Y" -> AddOrRemove), wired before the generic remove gate; sentence-1try_parse_choose_counter_on_target("[a|one|any] counter on " -> TargetOnly). ast.rs adds ZoneCounterImperativeAst::ChooseCounterAdjustment + lowering. Dispatch arm in effects/mod.rs; exhaustive-match registrations in coverage.rs, printed_cards.rs, trigger_index.rs, ability_graph.rs, sequence.rs.KEY DEVIATION (MATERIAL, empirically forced): The plan removed the dedicated sentence-1 handler, asserting
parse_targetwould yield the Or filter once the target arm landed. A diagnostic test proved this FALSE:parse_target("a counter on target permanent or suspended card")returns Any — the existing is_choose_as_targeting->parse_target dispatch does NOT forward-scan past the "a counter on " lead-in. I re-addedtry_parse_choose_counter_on_target(strips the lead-in via nom tag, then delegates to parse_target), mirroring try_parse_proliferate_target. It only fires when parse_target yields non-Any and nothing remains, so it is strictly downstream of all higher-priority handlers.JUDGEMENT CALLS: (1) The sentence-2 combinator emits only AddOrRemove (Clockspinning's exact form) to avoid widening the existing remove/put dispatch; the CounterAdjustment type keeps all three variants, exercised by the resolver + unit tests, ready for future single-direction cards. (2) Plan's "coverage.rs map EffectKind=>Handled" was stale — coverage classifies support via !matches!(Unimplemented), so the variant is auto-supported; only the exhaustive detail-extraction match needed the arm.
REGRESSION CHECK: 4 other cards share "choose a counter on" phrasing (Animation Module, Aven Courier, Ichormoon Gauntlet, The Caves of Androzani). Verified against the pre-change data/card-data.json that their sentence-1 target was already Any; my helper improves them to the correct target pool while their separate "give/put another of that kind" sub-clauses remain Unimplemented exactly as before (that broader class is out of scope). No regression — confirmed by reasoning AND the 14142-test suite passing AND the actual before-state.
VERIFICATION: fmt clean; clippy -p engine --all-targets exit 0 no warnings; test -p engine --lib 14142 passed/0 failed (incl. 8 new tests); oracle-gen Clockspinning 0 Unimplemented with TargetOnly{Or} + ChooseCounterAdjustment{AddOrRemove,1} sub + Buyback keyword + Optional additional_cost intact; gen-card-data.sh exit 0 across ~35366 cards (no serialization panic). Parser combinator gate: my added lines clean of string-literal dispatch (added .contains() calls are Vec membership in test assertions, not parsing dispatch; script's overall EXIT=1 is environmental — this worktree's origin/main resolves to ancient PR #904, flagging pre-existing history). Reverted gen-card-data's destructive known-tokens.toml side effect (was clean at session start); left the pre-existing oracle-subtypes.json modification untouched. No commit made.
Files changed
CR references
Verification
cargo fmt --all— clean (no changes)check-parser-combinators.sh (merge-base 656260f7)— clean (exit 0)cargo clippy -p engine --all-targets -- -D warnings— clean (exit 0, no errors in changed files)cargo test -p engine— clean (all tests passed, 0 failures)oracle-gen clockspinning— confirmed fixedCards confirmed re-parsed correctly: clockspinning
🤖 Generated with Claude Code