feat(phase-ai): add cost-reduction deck-feature axis + CostReductionPolicy - #6743
feat(phase-ai): add cost-reduction deck-feature axis + CostReductionPolicy#6743minion1227 wants to merge 3 commits into
Conversation
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
📝 WalkthroughWalkthroughAdds context-free card-face filter evaluation, deck-time cost-reduction analysis, and a registered tactical policy that rewards deploying reducers before discounted spells. ChangesCost Reduction Decision Support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeckFeatures
participant CostReductionFeature
participant EngineFilter
participant PolicyRegistry
participant CostReductionPolicy
DeckFeatures->>CostReductionFeature: analyze deck reducers and spell coverage
CostReductionFeature->>EngineFilter: match filters against CardFace
EngineFilter-->>CostReductionFeature: context-free admission
CostReductionFeature-->>DeckFeatures: commitment score
PolicyRegistry->>CostReductionPolicy: evaluate CastSpell candidate
CostReductionPolicy-->>PolicyRegistry: deploy bonus, defer penalty, or neutral verdict
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…olicy
`features::mana_ramp` explicitly deferred this shape — "`StaticMode::ModifyCost`
is deliberately out of scope — cost reducers are a follow-up feature". This is
that follow-up.
A Goblin Electromancer / Baral / Foundry Inspector / Medallion effect is
acceleration that never taps for mana: every later spell costs less for as long
as the permanent survives (CR 601.2f). The engine already applies the discount
at cast time, so the AI is never overcharged — but nothing valued *deploying*
the reducer. `mana_ramp` only sees effects that add mana, so a deck whose whole
acceleration plan is cost reduction read as having no ramp at all.
Feature (`features/cost_reduction.rs`), structural over `CardFace` AST:
- `reducer_count` — cards carrying a board-wide `ModifyCost { Reduce }` that
applies to spells YOU cast. Mirrors the eligibility tests
`casting::collect_cost_modifiers` applies at cast time, so deck-time
classification and the resolver agree by construction: `Reduce` only (`Raise`
is Thalia, `Minimum` is Trinisphere), never a `SelfRef` self-cost reduction
(CR 113.6), never opponent-scoped.
- `total_discount` — summed generic discount. CR 118.7a: a generic reduction
affects only the generic component, so the magnitude is `generic`, not the
amount's full mana value.
- `discounted_count` / coverage — deck cards a reducer's `spell_filter`
actually admits, delegating every type leaf to the engine's CR 205 authority
(`matches_type_filter_against_face`, widened to `pub`) rather than re-deriving
type semantics. An unverifiable filter reports no coverage, so the axis fails
OFF rather than claiming a discount the deck may not get.
- `commitment` — geometric mean over (reducer density, coverage): both pillars
mandatory. Reducers that discount nothing are blanks; spells with no reducer
are just spells.
Policy (`policies/cost_reduction.rs`, `CastSpell`):
- `cost_reduction_deploy_engine` — credit for deploying a reducer, scaled by
capped saved mana across the remaining grip.
- `cost_reduction_defer_to_engine` — nudge against casting past an unplayed,
cheaper reducer (the `RampTimingPolicy::defer_to_ramp` shape).
- The card-local static check runs first, so every non-reducer candidate is
rejected after reading one card's AST; only a confirmed reducer pays for the
hand walk. No battlefield sweep, no `find_legal_targets`, no affordability
query in the search inner loop.
Both scoring scalars land in `UNTUNED_POLICY_PENALTY_FIELDS` pending a
paired-seed calibration.
33 tests: per-axis detection, both calibration anchors, every exclusion
(`Raise`/`Minimum`/`SelfRef`/opponent-scope/zero-generic/unverifiable-filter),
format-size neutrality, the bounded-score ceiling, and a registry-routed
regression covering registration + `CastSpell` routing.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
…two elided assumptions Self-review of the cost-reduction axis before final submission: - The doc comments named `casting::collect_cost_modifiers`; the actual engine authority is `casting::collect_battlefield_cost_modifiers` (crates/engine/src/game/casting.rs:7179). A citation that does not resolve is worse than none — it claims a verification that never happened. - `your_spell_discount` silently ignored `dynamic_count`. Record that this is deliberate: a scaling reducer's multiplier is unknowable at deck-analysis time, so the per-unit `generic` is reported, understating rather than inventing a value. - `castable_cards_in_hand` counts remaining casts without narrowing to the reducer's `spell_filter`. Record why that is correct rather than sloppy: the narrowing is applied once per game through `commitment`, which `activation` multiplies in, instead of per candidate in the search inner loop. Documentation only — no behavior change; the 33 tests are untouched and green. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/phase-ai/src/features/cost_reduction.rs`:
- Around line 157-208: Expose the Reduce-mode, non-SelfRef, non-Opponent-scoped
eligibility logic used by casting::collect_cost_modifiers as a shared engine
helper for StaticDefinition/CardFace-compatible inputs, following the existing
matches_type_filter_against_face pattern. Update both
casting::collect_cost_modifiers and phase-ai's your_spell_discount to call this
authoritative helper, leaving your_spell_discount responsible only for
extracting and validating the generic amount.
- Around line 173-208: Update your_spell_discount to account for
ModifyCost::Reduce dynamic_count by resolving the multiplier and scaling the
generic discount by it, matching the engine’s amount *
resolve_quantity(dynamic_count) behavior. Preserve the existing filtering for
mode, SelfRef, opponent scope, and zero generic reductions, and return the
scaled value for saved_mana/total_discount.
In `@crates/phase-ai/src/policies/cost_reduction.rs`:
- Around line 116-154: Update castable_cards_in_hand and
hand_holds_cheaper_reducer to evaluate each reducer’s spell_filter against the
candidate spell before counting it as relevant. Thread the candidate spell or
matching context through their callers, preserve existing behavior for
spell_filter: None, and add a regression test where a reducer’s filter does not
match the candidate spell.
In `@crates/phase-ai/src/policies/registry.rs`:
- Around line 145-149: Move the CR 121.1 documentation comment so it directly
annotates DrawPayoff rather than CostReduction, and add an accurate verified CR
citation and description for CostReduction if one is required by the surrounding
policy annotations. Keep SelfBounceTarget unchanged and ensure each variant’s
documentation describes its own domain.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: cf1e186a-d035-49d1-bfd1-a81cc165ae95
📒 Files selected for processing (12)
crates/engine/src/game/filter.rscrates/phase-ai/src/config.rscrates/phase-ai/src/features/cost_reduction.rscrates/phase-ai/src/features/mana_ramp.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/cost_reduction.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/policies/cost_reduction.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/cost_reduction.rscrates/phase-ai/src/policies/tests/mod.rs
| pub(crate) fn your_spell_discount_parts<'a>( | ||
| statics: impl IntoIterator<Item = &'a StaticDefinition>, | ||
| ) -> u32 { | ||
| statics | ||
| .into_iter() | ||
| .filter_map(your_spell_discount) | ||
| .fold(0u32, u32::saturating_add) | ||
| } | ||
|
|
||
| /// CR 601.2f: the generic discount this one static gives to spells you cast, or | ||
| /// `None` when it is not a board-wide reduction of your own spells. | ||
| /// | ||
| /// Mirrors the eligibility tests `casting::collect_cost_modifiers` applies at | ||
| /// cast time, so deck classification and the resolver agree by construction: | ||
| /// `Reduce` mode only, never a `SelfRef` self-cost reduction, and never an | ||
| /// opponent-scoped modifier. | ||
| fn your_spell_discount(def: &StaticDefinition) -> Option<u32> { | ||
| let StaticMode::ModifyCost { | ||
| mode: CostModifyMode::Reduce, | ||
| amount, | ||
| .. | ||
| } = &def.mode | ||
| else { | ||
| // `Raise` (Thalia) and `Minimum` (Trinisphere) are taxes, not discounts. | ||
| return None; | ||
| }; | ||
|
|
||
| // CR 113.6: a `SelfRef` reduction is "this spell costs {N} less" — resolved | ||
| // by `apply_self_spell_cost_modifiers` for the spell being cast, and never | ||
| // applied from a battlefield permanent to other spells. It is a property of | ||
| // one card, not a deck-wide engine. | ||
| if matches!(def.affected, Some(TargetFilter::SelfRef)) { | ||
| return None; | ||
| } | ||
|
|
||
| // CR 601.2f: caster scope. `Opponent` is a discount handed to the other | ||
| // side; only `You` and an unscoped modifier reduce spells you cast. | ||
| if let Some(TargetFilter::Typed(typed)) = &def.affected { | ||
| if matches!(typed.controller, Some(ControllerRef::Opponent)) { | ||
| return None; | ||
| } | ||
| } | ||
|
|
||
| // CR 118.7a: only the generic component of a cost can be reduced by a | ||
| // generic reduction, so the discount magnitude is `generic`. A reduction of | ||
| // zero generic mana (a purely colored `amount`) moves no cost here and is | ||
| // not counted as an engine. | ||
| let ManaCost::Cost { generic, .. } = amount else { | ||
| return None; | ||
| }; | ||
| (*generic > 0).then_some(*generic) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Caster/mode eligibility is hand-duplicated instead of exposed from the engine, unlike the type axis.
your_spell_discount reimplements CR 601.2f eligibility (Reduce-only, not SelfRef, not Opponent-scoped) as a parallel copy of casting::collect_cost_modifiers's rules, per this file's own doc comment ("Mirrors the eligibility tests casting::collect_cost_modifiers applies at cast time, so deck classification and the resolver agree by construction"). The type axis avoided this exact problem by having matches_type_filter_against_face elevated to pub in engine::game::filter so phase-ai calls the authority directly instead of re-deriving CR 205 semantics. The caster/mode axis didn't get the same treatment — it's a hand-copy that can silently drift from the actual resolver if collect_cost_modifiers's rules ever change (new caster-scope variant, adjusted SelfRef handling, etc.), causing deck-feature detection and policy scoring to disagree with real game behavior.
Suggested direction
Expose an authoritative, CardFace/StaticDefinition-compatible eligibility check from engine (mirroring how matches_type_filter_against_face was elevated), and have casting::collect_cost_modifiers and phase-ai's your_spell_discount both call it.
Based on learnings, the repo's CLAUDE.md explicitly states phase-ai "should orchestrate/scoring using engine-provided semantics, not duplicate game rules."
🤖 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/phase-ai/src/features/cost_reduction.rs` around lines 157 - 208,
Expose the Reduce-mode, non-SelfRef, non-Opponent-scoped eligibility logic used
by casting::collect_cost_modifiers as a shared engine helper for
StaticDefinition/CardFace-compatible inputs, following the existing
matches_type_filter_against_face pattern. Update both
casting::collect_cost_modifiers and phase-ai's your_spell_discount to call this
authoritative helper, leaving your_spell_discount responsible only for
extracting and validating the generic amount.
Source: Path instructions
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed against current head d360a8a5f55425e4b630d40f907f1bd655d1c653.
This cannot merge yet: the new feature and policy score reductions that the casting authority would not apply, while also missing supported reducer classes. The existing CodeRabbit findings are confirmed against this head; the condition gap below is an additional current-head blocker.
[MED] Deck-time filter matching discards every nonempty TypedFilter.properties list. Evidence: crates/phase-ai/src/features/cost_reduction.rs:243-249 requires typed.properties.is_empty(), even though StaticMode::ModifyCost carries the whole TargetFilter; the engine's matches_target_filter_against_face is the context-free CardFace authority at crates/engine/src/game/filter.rs:1263-1294. Why it matters: reducers for color/colorless/historic/mana-value spell classes are silently treated as discounting no deck cards, so discounted_count and commitment are wrong. Suggested fix: expose and extend/reuse the engine's full static face matcher for every context-free TargetFilter property, rather than reimplementing the type axis here; add property-filter regressions, including an unsupported/live-only property failing closed.
[MED] The tactical policy counts and rewards every nonland card rather than the spells each reducer actually discounts. Evidence: crates/phase-ai/src/policies/cost_reduction.rs:79-96 derives only a generic amount, then castable_cards_in_hand at :121-138 counts all nonlands; hand_holds_cheaper_reducer at :143-153 likewise never inspects spell_filter. Why it matters: a narrow reducer is rewarded for irrelevant future cards and causes an incorrect sequencing penalty for a spell it cannot reduce. Suggested fix: retain/evaluate each qualifying reducer's spell_filter against the candidate and hand spell through an engine-owned matcher; cover both deploy-credit and defer-penalty mismatches, while preserving the unfiltered case.
[MED] dynamic_count reductions are valued as a fixed base amount. Evidence: crates/phase-ai/src/features/cost_reduction.rs:173-207 returns only amount.generic, while the cast authority resolves dynamic_count and applies it as a multiplier in crates/engine/src/game/casting.rs:7307-7324. Why it matters: the policy credits a reduction when the live multiplier is zero and under-credits it when the multiplier exceeds one. Suggested fix: expose an engine-owned live modifier evaluation for policy use (or conservatively fail this feature off when a live multiplier cannot be evaluated); add zero and greater-than-one multiplier regressions.
[MED] Conditional reducers are treated as unconditional. Evidence: the feature's qualifying path at crates/phase-ai/src/features/cost_reduction.rs:173-221 never examines StaticDefinition.condition, but collect_cost_modifiers gates every modifier with evaluate_cost_mod_static_condition at crates/engine/src/game/casting.rs:7278-7290. Why it matters: the AI can reward deploying a reducer during a turn/state in which it provides no discount. Suggested fix: consume the same engine eligibility/evaluation authority, or conservatively fail off when a deck-time condition cannot be evaluated; add a regression for an own-turn candidate with a negated own-turn condition.
[MED] Board-wide ModifyCost eligibility is duplicated in phase-ai instead of having one authority. Evidence: your_spell_discount recreates Reduce/SelfRef/opponent checks at crates/phase-ai/src/features/cost_reduction.rs:169-207, whereas the casting authority additionally owns functioning-zone, player-scope, condition, selected-target, filter, and dynamic-count checks at crates/engine/src/game/casting.rs:7222-7324. Why it matters: the two paths have already diverged in conditions and dynamic counts and will continue to drift for future reducer forms. Suggested fix: move/expose structured eligibility facts and live evaluation from the engine, then make both casting and phase-ai consume that authority rather than parallel predicates.
[LOW] The CR 121.1 doc comment is attached to CostReduction rather than DrawPayoff. Evidence: crates/phase-ai/src/policies/registry.rs:145-149; CR 121.1 in docs/MagicCompRules.txt:1142 defines drawing a card. Why it matters: the enum documentation states the wrong rule/domain and leaves DrawPayoff undocumented. Suggested fix: move the verified CR 121.1 comment directly above DrawPayoff; add only a separately verified, accurate comment for CostReduction if the surrounding policy IDs require one.
The parse-diff evidence for this head reports no card-parse changes. CI is green; AI gates remain in progress and are not used as a blocker for this review. Please address the correctness and ownership gaps above, then request re-review.
d360a8a to
ef0fa8c
Compare
github-actions
Bot
added
the
needs-maintainer
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed against current head ef0fa8c9b70fbd1419ce115fd224cf0b8b0c8b03.
The 21-line documentation follow-up does not resolve the implementation blockers from the prior review, so this remains changes requested.
[HIGH] The phase-AI feature duplicates only part of the engine's cost-modifier authority. Evidence: crates/phase-ai/src/features/cost_reduction.rs:173-215 recognizes a Reduce static based on its definition, while crates/engine/src/game/casting.rs:7217-7324 additionally selects functioning statics and applies source zone, caster scope, condition, spell-filter, and dynamic-multiplier semantics before a modifier exists. Why it matters: the AI can award a cost-reduction payoff to a static that cannot reduce the candidate player's future spells in the live game. Suggested fix: expose/reuse an engine-owned classification or evaluation seam that agrees with cost collection, rather than maintaining a partial phase-AI copy.
[MED] Dynamic reductions and static conditions are deliberately ignored by the feature/policy but are live casting semantics. Evidence: crates/phase-ai/src/features/cost_reduction.rs:205-215 credits one fixed generic unit and does not resolve dynamic_count; crates/phase-ai/src/policies/cost_reduction.rs:79-96 scores that fixed value; crates/engine/src/game/casting.rs:7278-7324 gates modifiers on condition and resolves dynamic_count. Why it matters: the policy can favor deploying a reducer that is currently disabled or value a zero-multiplier reducer as if it saved mana. Suggested fix: make the scoring path consume the same condition and quantity semantics as the casting authority, with tests for an inactive condition and a zero/nonzero dynamic multiplier.
[MED] The policy counts every nonland card in hand even when the reducer's spell_filter admits only a subset. Evidence: crates/phase-ai/src/policies/cost_reduction.rs:83,122-162 calls castable_cards_in_hand, which filters only lands, whereas the engine applies the modifier's spell_filter at crates/engine/src/game/casting.rs:7293-7305. Why it matters: an instant-and-sorcery or artifact-only reducer is rewarded for unrelated future spells, producing incorrect sequencing incentives. Suggested fix: evaluate the specific reducer's filter for each candidate future spell through a shared engine authority; deck-level commitment is not a substitute for the live hand's composition.
[MED] Deck-time filter matching rejects static property filters that the engine can evaluate against a card face. Evidence: crates/phase-ai/src/features/cost_reduction.rs:249-265 requires typed.properties.is_empty(), while crates/engine/src/game/filter.rs:1263-1294 supports context-free property evaluation, including HasSupertype. Why it matters: valid reducers such as filters restricted by a printed supertype are silently treated as covering no spells. Suggested fix: delegate the full context-free filter match to matches_target_filter_against_face instead of reproducing only the type axis.
[LOW] The CR annotation is attached to the wrong policy reason. Evidence: crates/phase-ai/src/policies/registry.rs:147-149 documents drawing-card payoff semantics immediately above CostReduction, not DrawPayoff. Why it matters: rule annotations are part of the engine's audit trail and this one misstates the concern represented by the enum member. Suggested fix: move the CR 121.1 comment to DrawPayoff and annotate CostReduction with the relevant cost-modification rule if an annotation is intended.
The parser parse-diff reports no changes, and pending AI gates are not blockers for this review outcome; the blockers above are current-head behavioral and ownership defects.
Addresses the review on phase-rs#6743. Every MED shared one root cause — phase-ai MIRRORED `collect_battlefield_cost_modifiers`'s eligibility instead of sharing it — so this fixes the ownership rather than patching each symptom. Engine (new single authority): - `StaticDefinition::board_wide_cost_modifier()` + `CostModifierCasterScope` (types/ability.rs) is now the one structural definition of "is this a board-wide cost modifier and what are its terms": rejects `Minimum` and `SelfRef` (CR 113.6), and names the caster scope so a caller WITH a PlayerId (the casting pipeline) and one WITHOUT (deck analysis) ask the same question. `collect_battlefield_cost_modifiers` now consumes it, so the two paths cannot drift again. - `matches_target_filter_against_face_scoped` + `context_free_prop_matches_face` (game/filter.rs) extend the CR 205 face authority to every context-free `FilterProp` — mana value (CR 202.3), color (CR 105.2), keyword, supertype, token-ness — and return `Option<bool>` so a live-only property is *unanswerable* and fails closed by construction rather than by a caller remembering to. `FaceControllerScope` names the previously-implicit controller assumption. phase-ai (consumes, no longer mirrors): - `your_spell_discount` reads the engine authority; the hand-rolled type-axis matcher is deleted. Color-, mana-value- and keyword-scoped reducers now produce coverage instead of silently reading as discounting nothing. - New `live_your_spell_discounts` resolves what a structural read cannot: `condition` fails off (a card in hand has no truthful "as long as" answer), and `dynamic_count` is resolved through the engine's `resolve_quantity`, so a multiplier of zero earns no credit and a multiplier above one scales it. - The policy counts and penalizes only spells a reducer actually discounts, matched through the engine's live `matches_target_filter` with the reducer as filter source. - `PolicyId::DrawPayoff` gets its CR 121.1 doc comment back; `CostReduction` gets its own verified CR 601.2f one. Tests 33 -> 44. Six new regressions, each proven RED by breaking the specific invariant it guards: color/mana-value/negated-color coverage, live-only property failing closed, controller-scoped filter admitted, filter-narrowed deploy credit and defer penalty, conditional reducer earning nothing, and zero vs positive dynamic multipliers. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Addressed at [MED] Board-wide [MED] [MED] [MED] Conditional reducers treated as unconditional. [MED] Policy rewarded/penalized every nonland card. [LOW] Misplaced CR 121.1 comment. Verification (head
Re-running |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/types/ability.rs (1)
20885-20989: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo unit tests for the new shared cost-modifier authority.
cost_modifier_caster_scope,board_wide_cost_modifier,admits, andadmits_own_controllerare new public building blocks that both the casting pipeline and phase-ai's deck feature detection now depend on (per PR objectives), but this file'stestsmodule has no coverage for any of them — not even the basic You/Opponent/Any scope resolution or theMinimum/SelfRefexclusion inboard_wide_cost_modifier.As per path instructions: "Reuse existing shared building blocks before adding utility or inline extraction logic; test the building block and its parameter range rather than a single card case." A single-authority function consumed by two different subsystems is exactly the case where a small parameter-range unit test (You/Opponent/Any × with/without spell_filter × Minimum/other modes × SelfRef/non-SelfRef
affected) pays for itself.🤖 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/types/ability.rs` around lines 20885 - 20989, The new shared cost-modifier helpers lack unit coverage. Add focused parameterized tests in the existing tests module for cost_modifier_caster_scope and CostModifierCasterScope::admits/admits_own_controller across You, Opponent, and Any, and for StaticDefinition::board_wide_cost_modifier covering accepted modes, spell_filter presence, and rejection of Minimum and SelfRef affected values.Source: Path instructions
🤖 Prompt for all review comments with 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.
Inline comments:
In `@crates/engine/src/game/filter.rs`:
- Around line 1346-1393: Update the FilterProp::AnyOf branch in
context_free_prop_matches_face to short-circuit and return Some(true) as soon as
any operand evaluates to Some(true), without evaluating or propagating later
None results. Preserve Some(false) when all operands are answerable and false,
and return None only when no true operand exists and at least one operand is
unanswerable.
---
Nitpick comments:
In `@crates/engine/src/types/ability.rs`:
- Around line 20885-20989: The new shared cost-modifier helpers lack unit
coverage. Add focused parameterized tests in the existing tests module for
cost_modifier_caster_scope and
CostModifierCasterScope::admits/admits_own_controller across You, Opponent, and
Any, and for StaticDefinition::board_wide_cost_modifier covering accepted modes,
spell_filter presence, and rejection of Minimum and SelfRef affected values.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 916b0b3c-ac6f-448e-985e-6370ad70653f
📒 Files selected for processing (8)
crates/engine/src/game/casting.rscrates/engine/src/game/filter.rscrates/engine/src/types/ability.rscrates/phase-ai/src/features/cost_reduction.rscrates/phase-ai/src/features/tests/cost_reduction.rscrates/phase-ai/src/policies/cost_reduction.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/cost_reduction.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/policies/registry.rs
- crates/phase-ai/src/policies/cost_reduction.rs
| /// CR 205 + CR 202: Evaluate one `FilterProp` against a bare `CardFace`. | ||
| /// | ||
| /// `Some(true)` / `Some(false)` = the property has a context-free reading and | ||
| /// this is it. `None` = the property needs a live object (battlefield state, | ||
| /// counters, combat, zone, a value chosen earlier in a resolution) and therefore | ||
| /// has NO answer for a face — callers must fail closed rather than guess. | ||
| /// | ||
| /// Kept as an explicit allowlist, not a wildcard `_ => true`: a newly added | ||
| /// `FilterProp` lands in the `None` arm and fails closed until someone decides | ||
| /// whether it is context-free, which is the safe direction. | ||
| pub fn context_free_prop_matches_face(face: &CardFace, prop: &FilterProp) -> Option<bool> { | ||
| match prop { | ||
| // CR 205.4a: printed supertype line. | ||
| FilterProp::HasSupertype { value } => Some(face.card_type.supertypes.contains(value)), | ||
| FilterProp::NotSupertype { value } => Some(!face.card_type.supertypes.contains(value)), | ||
| // CR 202.3: mana value is printed on the face. Only a fixed comparison | ||
| // is context-free — a dynamic quantity needs game state. | ||
| FilterProp::Cmc { | ||
| comparator, | ||
| value: QuantityExpr::Fixed { value }, | ||
| } => Some(comparator.evaluate( | ||
| i32::try_from(face.mana_cost.mana_value()).unwrap_or(i32::MAX), | ||
| *value, | ||
| )), | ||
| // A dynamic mana-value comparison needs game state to resolve. | ||
| FilterProp::Cmc { .. } => None, | ||
| // CR 105.2 + CR 202.2: printed color, via the face's own color authority. | ||
| FilterProp::HasColor { color } => Some(face_colors(face).contains(color)), | ||
| FilterProp::NotColor { color } => Some(!face_colors(face).contains(color)), | ||
| FilterProp::ColorCount { comparator, count } => Some(comparator.evaluate( | ||
| i32::try_from(face_colors(face).len()).unwrap_or(i32::MAX), | ||
| i32::from(*count), | ||
| )), | ||
| // CR 702: printed keyword line. | ||
| FilterProp::WithKeyword { value } => Some(face.keywords.contains(value)), | ||
| FilterProp::WithoutKeyword { value } => Some(!face.keywords.contains(value)), | ||
| // CR 111.1 + CR 108.2: a bare face is a card definition, never a token. | ||
| FilterProp::Token => Some(false), | ||
| FilterProp::NonToken | FilterProp::RepresentedByCard => Some(true), | ||
| // Recursive combinators inherit their operands' answerability. | ||
| FilterProp::Not { prop } => context_free_prop_matches_face(face, prop).map(|m| !m), | ||
| FilterProp::AnyOf { props } => props.iter().try_fold(false, |acc, inner| { | ||
| context_free_prop_matches_face(face, inner).map(|m| acc || m) | ||
| }), | ||
| // Everything else reads live state and has no face-level answer. | ||
| _ => None, | ||
| } | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
FilterProp::AnyOf can turn a proven true into a fail-closed None.
try_fold discards the accumulator the instant the closure returns None. So if an earlier disjunct resolves Some(true) and a later disjunct in the same Vec is unanswerable (None), the whole AnyOf incorrectly returns None instead of Some(true) — losing the already-proven match. Correct OR semantics: once any disjunct is definitely true, the composite is true regardless of the answerability of the rest.
This is pub and used outside AI heuristics (module doc cites Effect::CreateTokenCopyFromPool pool hydration), so a reducer/filter combining an answerable-true FilterProp with an unanswerable one inside AnyOf can be silently mismatched in real gameplay, and undercounts phase-ai's cost-reduction coverage downstream (filter_admits_face). No existing test exercises FilterProp::AnyOf through this path.
🐛 Proposed fix — short-circuit on the first proven `true`
- FilterProp::AnyOf { props } => props.iter().try_fold(false, |acc, inner| {
- context_free_prop_matches_face(face, inner).map(|m| acc || m)
- }),
+ FilterProp::AnyOf { props } => {
+ let mut any_unknown = false;
+ for inner in props {
+ match context_free_prop_matches_face(face, inner) {
+ Some(true) => return Some(true),
+ Some(false) => {}
+ None => any_unknown = true,
+ }
+ }
+ if any_unknown { None } else { Some(false) }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// CR 205 + CR 202: Evaluate one `FilterProp` against a bare `CardFace`. | |
| /// | |
| /// `Some(true)` / `Some(false)` = the property has a context-free reading and | |
| /// this is it. `None` = the property needs a live object (battlefield state, | |
| /// counters, combat, zone, a value chosen earlier in a resolution) and therefore | |
| /// has NO answer for a face — callers must fail closed rather than guess. | |
| /// | |
| /// Kept as an explicit allowlist, not a wildcard `_ => true`: a newly added | |
| /// `FilterProp` lands in the `None` arm and fails closed until someone decides | |
| /// whether it is context-free, which is the safe direction. | |
| pub fn context_free_prop_matches_face(face: &CardFace, prop: &FilterProp) -> Option<bool> { | |
| match prop { | |
| // CR 205.4a: printed supertype line. | |
| FilterProp::HasSupertype { value } => Some(face.card_type.supertypes.contains(value)), | |
| FilterProp::NotSupertype { value } => Some(!face.card_type.supertypes.contains(value)), | |
| // CR 202.3: mana value is printed on the face. Only a fixed comparison | |
| // is context-free — a dynamic quantity needs game state. | |
| FilterProp::Cmc { | |
| comparator, | |
| value: QuantityExpr::Fixed { value }, | |
| } => Some(comparator.evaluate( | |
| i32::try_from(face.mana_cost.mana_value()).unwrap_or(i32::MAX), | |
| *value, | |
| )), | |
| // A dynamic mana-value comparison needs game state to resolve. | |
| FilterProp::Cmc { .. } => None, | |
| // CR 105.2 + CR 202.2: printed color, via the face's own color authority. | |
| FilterProp::HasColor { color } => Some(face_colors(face).contains(color)), | |
| FilterProp::NotColor { color } => Some(!face_colors(face).contains(color)), | |
| FilterProp::ColorCount { comparator, count } => Some(comparator.evaluate( | |
| i32::try_from(face_colors(face).len()).unwrap_or(i32::MAX), | |
| i32::from(*count), | |
| )), | |
| // CR 702: printed keyword line. | |
| FilterProp::WithKeyword { value } => Some(face.keywords.contains(value)), | |
| FilterProp::WithoutKeyword { value } => Some(!face.keywords.contains(value)), | |
| // CR 111.1 + CR 108.2: a bare face is a card definition, never a token. | |
| FilterProp::Token => Some(false), | |
| FilterProp::NonToken | FilterProp::RepresentedByCard => Some(true), | |
| // Recursive combinators inherit their operands' answerability. | |
| FilterProp::Not { prop } => context_free_prop_matches_face(face, prop).map(|m| !m), | |
| FilterProp::AnyOf { props } => props.iter().try_fold(false, |acc, inner| { | |
| context_free_prop_matches_face(face, inner).map(|m| acc || m) | |
| }), | |
| // Everything else reads live state and has no face-level answer. | |
| _ => None, | |
| } | |
| } | |
| FilterProp::AnyOf { props } => { | |
| let mut any_unknown = false; | |
| for inner in props { | |
| match context_free_prop_matches_face(face, inner) { | |
| Some(true) => return Some(true), | |
| Some(false) => {} | |
| None => any_unknown = true, | |
| } | |
| } | |
| if any_unknown { None } else { Some(false) } | |
| } |
🤖 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/game/filter.rs` around lines 1346 - 1393, Update the
FilterProp::AnyOf branch in context_free_prop_matches_face to short-circuit and
return Some(true) as soon as any operand evaluates to Some(true), without
evaluating or propagating later None results. Preserve Some(false) when all
operands are answerable and false, and return None only when no true operand
exists and at least one operand is unanswerable.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — two blockers remain on head f8d3e4d.
🔴 Blocker
[HIGH] Raw keyword membership bypasses the engine keyword authority. Evidence: crates/engine/src/game/filter.rs:1379-1380 adds face.keywords.contains calls; CI run 30410592730 fails the Engine authority gate on those exact lines. Why it matters: this creates a second keyword-evaluation path that can diverge from the established authority. Suggested fix: use the existing keyword authority, or add the gate-prescribed structural annotation with a justification.
🟡 Blocker
[MED] FilterProp::AnyOf does not implement three-valued OR. Evidence: crates/engine/src/game/filter.rs:1387-1389 uses try_fold, so [Some(true), None] produces None; the caller at filter.rs:1314-1320 then rejects it. Why it matters: a matching alternative can be discarded whenever a later alternative is unknown. Suggested fix: implement three-valued OR and add cases covering [Some(true), None], [None, Some(true)], and all-unknown inputs.
The parse-diff is current and reports no card changes. Relevant CR citations were verified and are not blockers.
Recommendation: address both findings, then request re-review.
2 participants
Summary
Adds a
cost_reductiondeck-feature axis and its companionCostReductionPolicytophase-ai, so the AI values deploying a CR 601.2f cost reducer before the spells it discounts.features::mana_rampexplicitly deferred this shape in its module docstring — "StaticMode::ModifyCostis deliberately out of scope — cost reducers are a follow-up feature" — and this is that follow-up.The engine already applies the discount at cast time, so the AI was never overcharged; what was missing is any reason to sequence the reducer first.
RampTimingPolicysupplies exactly that signal for permanents that add mana and structurally cannot see a cost reducer, so a deck whose entire acceleration plan is cost reduction read as having no ramp at all.Files changed
crates/phase-ai/src/features/cost_reduction.rs(new)crates/phase-ai/src/features/tests/cost_reduction.rs(new)crates/phase-ai/src/policies/cost_reduction.rs(new)crates/phase-ai/src/policies/tests/cost_reduction.rs(new)crates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/mana_ramp.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/config.rscrates/engine/src/game/filter.rsTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — this change adds AI heuristics under
crates/phase-ai/, notcrates/engine/game logic. The singlecrates/engine/edit widens one existing function's visibility (pub(crate)→pubonmatches_type_filter_against_face) with no behavior change, so there is no parser, effect, resolver, targeting, or rules-behavior change for/engine-implementerto own. Everything else is deck classification and candidate scoring.CR references
CR 601.2f— cost determination; reductions are subtracted from the total cost. Governs the whole axis.CR 118.7a— a generic cost reduction affects only the generic component of a cost. Fixes the discount magnitude toamount'sgeneric, not its mana value.CR 113.6— abilities of non-instant/sorcery objects function on the battlefield; aSelfRefreduction is a property of one spell, not a board-wide engine.CR 205— type-line semantics, via the engine authoritymatches_type_filter_against_face.CR 305.1— playing a land is not casting a spell, so lands are excluded from coverage and from "future casts".All five were verified against
docs/MagicCompRules.txtbefore being written.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo test -p phase-ai—1672 passed; 0 failed(lib) plus all integration binaries green, includingevery_policy_penalty_is_tuning_registered_or_explicitly_untuned, which enforces that both newPolicyPenaltiesfields are registered.cargo test -p phase-ai --lib cost_reduction—33 passed; 0 failed.cargo clippy -p phase-ai -p engine --all-targets --features proptest -- -D warnings— exit 0, no diagnostics. Re-run after the rebase onto1604a6f30.cargo fmt --all— clean../scripts/check-parser-combinators.sh— PASS (see Gate A).cargo ai-gate— run on this head and as a paired control on the pre-change parent. Diffing the two full runs, the only difference in the entire output is the nondeterministic OS thread id inside a pre-existing panic line:Every game result, deck verdict, flip count and p-value is byte-identical —
compare: 0 FAIL, 2 WARN, 1 PASS, 0 NEW, 0 REMOVEDon both sides. The two WARNs (enchantress-mirror50→10%,red-mirror70→50%), theenchantress-mirrorsuite FAIL, and theMetallic Rebukepanic (same seed10592737) all reproduce unchanged on the parent commit and are pre-existing and unrelated to this PR. No quick-filter deck clearsCOST_REDUCTION_FLOOR, so the policy is inert across the suite. No baseline refresh is included — refreshing would launder pre-existing drift this PR did not cause.cargo ai-perf-gate— not applicable. This policy adds no board-wide or affordability engine call (max_x_value,feasible_mana_capacity,find_legal_targets,SimulationFilterclone).verdict()runs a card-local static check first and only a confirmed reducer pays for a hand-bounded walk.Mutation evidence that the tests are not vacuous. Three invariants were individually broken in the implementation — the
SelfRefexclusion, the caster-scope check, and the coverage pillar (forced to full) — and 9 tests failed, each in the branch it guards. Restored to green afterwards.Gate A
Gate A PASS head=ef0fa8c9b70fbd1419ce115fd224cf0b8b0c8b03 base=1604a6f3023413a34740d6d268d77872b3150b0f
Anchored on
crates/phase-ai/src/features/draw_matters.rs:75— the established axis shape this mirrors:detect()folding per-DeckEntryCardFaceAST into counts,pub(crate)parts-based predicates shared with the policy, and acommitmentscalar;:248is the same two-pillarcommitment::geometric_meanformula with a calibration anchor in its docstring.crates/phase-ai/src/policies/draw_payoff.rs:45— the sameTacticalPolicyshape:activation()as a single floor-plus-scale knob returningOption<f32>, and:67the cheap-to-expensiveverdict()ordering where a card-local AST check rejects non-members before any wider scan (MAX_REWARDED_ENGINESis the cap constant this PR'sMAX_REWARDED_FUTURE_CASTS/MAX_REWARDED_DISCOUNTfollow).crates/engine/src/game/casting.rs:7179—collect_battlefield_cost_modifiers, the runtime authority whose eligibility tests the deck-time classifier mirrors exactly:Reducemode only,SelfRefskipped,ControllerRef::Opponentskipped.crates/phase-ai/src/policies/ramp_timing.rs:102— thedefer_to_rampverdict this PR'scost_reduction_defer_to_enginebranch follows.Final review-impl
Final review-impl PASS head=ef0fa8c9b70fbd1419ce115fd224cf0b8b0c8b03
Self-review before submission raised and fixed three items in commit
ef0fa8c9b(documentation only, no behavior change):casting::collect_cost_modifiers; corrected tocollect_battlefield_cost_modifiers.dynamic_countwas silently ignored — now documented as a deliberate conservative choice (a scaling reducer's multiplier is unknowable at deck-analysis time, so the per-unitgenericis reported, understating rather than inventing).castable_cards_in_handdoes not narrow to the reducer'sspell_filter— now documented, with the reason: the narrowing is applied once per game throughcommitment, whichactivationmultiplies in, rather than per candidate in the search inner loop.One candidate finding was refuted:
PolicyIdderives noSerialize/Deserialize, so inserting theCostReductionvariant mid-enum carries no serialized-surface risk.Claimed parse impact
None. No parser, card-data, or Oracle-text code is touched; the axis reads already-parsed AST.
Scope Expansion
None. One axis and one policy in the scope the upstream
mana_rampdocstring named. The onecrates/engine/edit is a visibility widening sophase-aicalls the existing CR 205 authority instead of duplicating type semantics — no logic was copied into the AI crate.Validation Failures
None.
CI Failures
None outstanding from this change. Noted for transparency: the
Contributor trustcheck reportedaction_required(score 35/100) on the first push — that is an account-reputation scan, not a code signal, and requires maintainer action rather than a code change.🤖 Generated with Claude Code
Summary by CodeRabbit