fix(parser): UNSUPPORTED full-set one-off: The Dining Car#5651
Conversation
![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 support for transient, turn-scoped activated-ability cost reductions by introducing Effect::ReduceActivatedAbilityCost and tracking pending reductions in GameState. It also unifies the parsing of static and transient activated-ability cost reductions using a shared grammar head, and adds support for "least/lowest/smallest" superlatives mapping to AggregateFunction::Min. The review feedback recommends guarding the static cost reduction loop in casting.rs with has_static to prevent unnecessary battlefield scans when only transient reductions are active.
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.
| let has_static = static_kind_present(state, StaticModeKind::ReduceAbilityCost); | ||
| let has_transient = !state.pending_activation_cost_reductions.is_empty(); | ||
| if !has_static && !has_transient { | ||
| return; | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Guard the static cost reduction loop with has_static to avoid unnecessary battlefield scans.
Why it matters: When a transient reduction is active (e.g., from The Dining Car) but no static reductions exist on the battlefield, every activation check will perform a full O(N) battlefield scan of static abilities. Guarding the static loop with if has_static avoids this overhead, which is especially beneficial during high-frequency AI searches.
Suggested fix: Wrap the static cost reduction loop (from line 16177 to 16290) in if has_static { ... }.
Parse changes introduced by this PR · 3 card(s), 5 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked on two things: the branch doesn't compile, and GAP B builds a second cost-reduction mechanism next to one the engine already has. GAP A is genuinely good and I'd take it on its own.
🔴 Blocker 1 — the branch does not compile
Effect::ReduceActivatedAbilityCost isn't covered in two exhaustive matches, both in phase-ai:
crates/phase-ai/src/policies/effect_classify.rs:52crates/phase-ai/src/policies/redundancy_avoidance.rs:279
error[E0004]: non-exhaustive patterns:
`&engine::types::Effect::ReduceActivatedAbilityCost { .. }` not covered
This fails Rust lint, both test shards, and WASM compile check.
This is not main drift. Both files landed 2026-07-10 (9d82a1d0cc), two days before this PR's base (abc3852a5d). They were on main when you branched.
The reason it slipped past you is worth naming, because your verification was honest and still couldn't catch it:
cargo clippy -p engine --all-targets -- -D warnings — clean
cargo test -p engine — PASS (2728 passed)
Both are scoped -p engine. phase-ai is a different crate that also matches exhaustively over Effect, so -p engine structurally cannot observe it. Every claim you made is true; the commands just measured the wrong population. Adding a variant to Effect obligates --workspace — exhaustiveness is a property of every crate that matches the enum, not of the crate that declares it.
🔴 Blocker 2 — GAP B duplicates a mechanism the engine already composes
The engine already models "install a static ability for a duration":
// crates/engine/src/types/ability.rs:10472
GenericEffect {
static_abilities: Vec<StaticDefinition>, // any StaticMode, incl. ReduceAbilityCost
duration: Option<Duration>, // UntilEndOfTurn = your "this turn" axis
target: Option<TargetFilter>, // your source_filter
}and StaticMode::ReduceAbilityCost (types/statics.rs:1012) is already heavily parameterized — mode: CostModifyMode, keyword, amount, minimum_mana, dynamic_count, ActivationExemption. Its own doc comment records the discipline: "Directional parameterization (not a Raise-only sibling)."
This PR instead adds a new Effect variant + a new PendingActivationCostReduction store in game_state.rs + a second reduction loop appended to casting.rs. That is a second cost-reduction pathway for a concept that already has one, and it cuts against CLAUDE.md's "Single authority for ability costs".
You are right that it doesn't work today, and I want to credit the diagnosis. I traced it: reduce_activated_ability_cost scans battlefield_active_statics(state) (casting.rs:16183), which iterates state.battlefield only — while GenericEffect's statics are installed as "continuous effects with explicit durations, rather than being pushed onto" permanents (effects/effect.rs:16). So a resolution-generated static is genuinely invisible to the cost hook.
That is a real gap. But it argues for teaching the cost hook to also read duration-scoped continuous effects — one authority, and it fixes every future "activated abilities cost less this turn" card in one place — rather than standing up a parallel mechanism that only The Dining Car walks. Build for the class, not the card.
What would change my mind: show that a duration-scoped continuous ReduceAbilityCost cannot be made visible to the cost hook without breaking the CR 613 layer ordering or the static_kind_present O(1) presence gate. If that's true, the parallel path is justified and I'll say so — but it needs to be demonstrated, not assumed.
🟡 Non-blocking
Effect::ReduceActivatedAbilityCost vs StaticMode::ReduceAbilityCost is two names for one domain concept. Even if the parallel path survives Blocker 2, the naming should make the relationship explicit rather than reading as an unrelated sibling.
✅ Clean — GAP A is the right shape
Parameterizing the superlative-property combinator so the aggregate direction is a single alt (greatest/highest → Max, least/lowest/smallest → Min) is exactly the building-block move: it closes the whole least/lowest/smallest superlative class rather than one card, reuses the runtime's existing AggregateFunction::Min, and adds no variant. The Card data check passed and the parse-diff is real. If you split GAP A out, I'd take it as-is.
Recommendation: request-changes. Fix the two phase-ai match arms and re-verify with --workspace; then either demonstrate why the continuous-effects seam can't carry the reduction, or move GAP B onto GenericEffect { static_abilities: [ReduceAbilityCost], duration: UntilEndOfTurn } and extend the cost hook's scan. Splitting GAP A into its own PR would let the parser win land now.
2 participants
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED full-set one-off: The Dining Car
Cards corrected
Fix
Implemented the approved plan for WHO cluster #96 (The Dining Car) in /Users/ntindle/code/random/magic/phase-main-base. The card was UNSUPPORTED (Unimplemented/Unknown) on upstream/main; both root-cause gaps are now fixed and the card re-parses with 0 Unimplemented / 0 Unknown across all three triggers.
GAP A (parser-only): parameterized the superlative-property combinator in parser/oracle_target.rs so the aggregate-direction axis is a single
alt— greatest/highest = Max (existing), least/lowest/smallest = Min (new). "with the least toughness among creatures you control" now yields a Min-aggregate toughness PtComparison, so The Dining Car's upkeep "sacrifice a creature with the least toughness" resolves correctly (runtime already supported AggregateFunction::Min). This completes the whole least/lowest/smallest superlative class, not one card.GAP B (new class-level engine primitive): added Effect::ReduceActivatedAbilityCost { amount, source_filter } for the transient (this-turn) activation-cost reduction from The Dining Car's chaos ability — the resolution-generated sibling of the printed StaticMode::ReduceAbilityCost. Built across all layers per add-engine-variant: types (Effect + EffectKind + name/kind maps + compiler-enumerated exhaustive-match arms in 8 sibling matches), storage (PendingActivationCostReduction in game_state.rs), resolver (resolve_reduce_activated_ability_cost in effects/mod.rs pushing the pending vec), single-authority cost hook (widened the ReduceAbilityCost gate in casting.rs and appended a transient reduction loop reusing the same &mut AbilityCost — so AI affordability and activation both see the reduced cost with no AI change), cleanup clear (turns.rs, CR 514.2, paralleling pending_spell_cost_reductions). Per the minimalism gate I deliberately omitted exemption/minimum_mana/activator (no transient driver). Parser: extracted the shared grammar head parse_activated_ability_cost_head into oracle_static/cost_mod.rs as the single authority consumed by BOTH the static branch (dispatch.rs, behavior-preserving lift — 8 static regression tests still green) and the new transient-effect helper (imperative.rs), wired before the imperative fallback. The static/transient separation is a dispatch-site invariant (a printed line is claimed by parse_static_line before parse_imperative_effect runs), asserted at the pipeline level. Multiplayer: pending_activation_cost_reductions passes through filter_state_for_player untouched (mirrors pending_spell_cost_reductions; verified no redaction). Frontend: EffectKind is tsify-generated, no exhaustive EffectKind switch in client/src, no WaitingFor — no frontend change needed.
Added building-block-level tests: superlative direction mapping (Min/Max across all words) + card-level least-toughness assertion; effect parse to ReduceActivatedAbilityCost with load-bearing FilterProp::Token; negative arm rejection (Raise/loyalty stay Unimplemented); pipeline negative (Training Grounds stays static, no transient effect); resolver-push test; and a RUNTIME PROOF (card-test style) that a pending reduction cuts a controlled artifact TOKEN's activated ability by {2} while leaving a controlled nontoken and an opponent's token unaffected, then is cleared at cleanup.
VERIFICATION (worktree, no Tilt — cargo run directly): cargo fmt clean; cargo clippy -p engine --all-targets -D warnings clean; cargo test -p engine = 16216 lib + 6 + 9 + 2721 integration passed, 0 failed (all 8 new tests green); ./scripts/gen-card-data.sh succeeded (35366 cards, corpus-wide export clean); The Dining Car in regenerated card-data.json shows chaos→ReduceActivatedAbilityCost and upkeep-sacrifice→Min aggregate, 0 Unimplemented/Unknown. PARSER DIFF GATE: pass — check-parser-combinators.sh exits 1 but ONLY on pre-existing upstream code (baseline ff799f8 is an ancestor of HEAD; none of my added lines are flagged); the exact task-spec gate flags only three Vec::contains calls inside #[test] bodies (covered by the test exemption) — zero string dispatch in production parser code (cost_mod/dispatch/imperative/oracle_effect all pure nom combinators).
DEVIATIONS: (1) Phase 4's optional perf wrap (guarding the static loop with
if has_static) was NOT applied — the static loop runs unconditionally when any reduction is present; when only a transient reduction exists it does one extra battlefield-statics scan that finds no ReduceAbilityCost defs and no-ops (correct; the plan marked this optimization optional). (2) Added one extra negative test (Raise/loyalty rejection) beyond the plan for building-block completeness. (3) Removed redundant ..Default::default() from two TypedFilter test literals (clippy needless_update — TypedFilter has exactly 3 fields). No CR was ambiguous; no stop-and-return triggered. Did NOT commit. Generated artifacts (client/public/*.json) were regenerated as verification output; crates/engine/data/oracle-subtypes.json was already modified before this task.Files changed
CR references
Verification
cargo fmt --all— clean (no files changed, no commit needed)check-parser-combinators.sh (base=abc3852a5)— PASS (Gate A PASS head=0c7618cab base=abc3852a5)cargo clippy -p engine --all-targets -- -D warnings— clean (no warnings)cargo test -p engine— PASS (2728 passed; 0 failed; 2 ignored)oracle-gen data --filter 'the dining car'— PASS (all 3 triggers parse correctly, no Unimplemented/Unknown)Cards confirmed re-parsed correctly: The Dining Car
🤖 Generated with Claude Code