feat(ai): add enchantments-matter deck-feature axis + EnchantmentsPayoffPolicy - #3492
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces an enchantments-matter feature detector and a companion tactical policy (EnchantmentsPayoffPolicy) to reward casting enchantments in decks with enchantress or constellation payoffs. Feedback highlights two key issues: first, TargetFilter::And incorrectly uses all instead of any when matching enchantments you control, which fails to recognize payoffs with additional constraints (e.g., nontoken); second, a defensive guard is needed for when total_nonland is zero to prevent potential division-by-zero or NaN errors in density calculations.
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 109.3: every constraint of an `And` must hold for it to be an | ||
| // "enchantment you control" match (mirrors `engine::game::filter`). | ||
| TargetFilter::And { filters } => filters.iter().all(filter_matches_enchantment_you_control), |
There was a problem hiding this comment.
[HIGH] TargetFilter::And uses all instead of any when matching enchantments you control.
Why it matters: If a constellation trigger has any additional constraints combined via And (such as "nontoken" in "whenever a nontoken enchantment you control enters" or "creature" in "whenever an enchantment creature you control enters"), those additional constraints will not match filter_matches_enchantment_you_control individually. Using all will cause the entire And filter to return false, meaning the AI will completely fail to recognize these cards as constellation payoffs. Since any object matching the And filter must satisfy all sub-filters, if at least one sub-filter restricts the match to "enchantment you control", the entire conjunction is guaranteed to be an enchantment you control.
Suggested fix: Change all to any for TargetFilter::And.
| // CR 109.3: every constraint of an `And` must hold for it to be an | |
| // "enchantment you control" match (mirrors `engine::game::filter`). | |
| TargetFilter::And { filters } => filters.iter().all(filter_matches_enchantment_you_control), | |
| // If any sub-filter of an `And` restricts the match to "enchantment you control", | |
| // then the entire conjunction is restricted to "enchantment you control". | |
| TargetFilter::And { filters } => filters.iter().any(filter_matches_enchantment_you_control), |
| payoff_count = payoff_count.saturating_add(entry.count); | ||
| } | ||
| } | ||
|
|
There was a problem hiding this comment.
[MEDIUM] Missing guard for total_nonland == 0 before calling density_per_60.
Why it matters: If a deck consists entirely of lands (e.g., during testing or specific scenarios), total_nonland will be 0, which could lead to division-by-zero or NaN issues inside commitment::density_per_60.
Suggested fix: Add a defensive guard to return early with 0.0 commitment if total_nonland is 0.
| if total_nonland == 0 { | |
| return EnchantmentsFeature { | |
| enchantment_count, | |
| payoff_count, | |
| commitment: 0.0, | |
| }; | |
| } |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer review — Changes requested (one gate: activating ai-gate evidence)
The architecture and idiom here are excellent — principal-quality structural detection, correct band routing, clean registry wiring. The single blocker is that the required paired-seed cargo ai-gate evidence is vacuous: it never activates the policy.
What is right (no changes needed)
- Seam + idiom — correct. New
features/enchantments.rsdetector +policies/enchantments_payoff.rs, wired intoDeckFeatures::analyzeand the policy registry — the sanctionedadd-ai-feature-policypath. Mirrors the artifacts sibling (#3355) exactly. - Structural detection, no name matching.
is_enchantment_payoff_triggeris a single authority over the typed AST (TriggerMode::SpellCast/SpellCastOrCopy+valid_cardEnchantment filter for enchantress;ChangesZone+ Battlefield destination + enchantment-you-control filter for constellation, mirroringlandfall.rs). TheOr/Andfilter handling with the CR 109.3 rationale (intersection →any, so compound "nontoken enchantment you control" triggers still match) is correct and covers the class, not one card. - Band helpers, not raw scores.
PolicyVerdict::score(enchantment_cast_bonus=0.4, …)routes through the band contract (lands in thepreferenceband). Matches the sibling pattern; the Phase-3 lint is satisfied. - Config knob registered UNTUNED.
enchantment_cast_bonusadded toUNTUNED_POLICY_PENALTY_FIELDSalongside the lifegain sibling — correct. - Payoff-gated activation is sound.
payoff_count == 0 || commitment < COMMITMENT_FLOOR → Nonekeeps non-payoff decks unaffected.
Blocker — the ai-gate result does not activate the policy
The PR body itself states the policy is inert on the aggro suite (payoff-gated; that deck has no enchantment payoffs), and the default gate suite filters to red-mirror/aggro matchups with no enchantress/constellation deck. So across the entire cargo ai-gate run, activation() returns None and the policy never fires above COMMITMENT_FLOOR. A "0 FAIL" run where the policy never activated proves only "casting nothing extra doesn't regress unrelated decks" — it is not evidence the active policy behaves correctly or improves enchantment-deck play. AI-policy changes must ship with a paired-seed gate that actually exercises the new policy above its activation floor.
To clear this: add an enchantress/constellation deck to the gate (or duel) suite and attach a paired-seed cargo ai-gate report in which EnchantmentsPayoffPolicy is demonstrably active (non-zero activation), then confirm no significant regression. The 16 unit tests do discriminate the policy's active branches (activation gates, verdict branches, calibration anchors) and are good — but they don't substitute for the activating gate run.
Everything else is merge-ready; this is the one item standing between the PR and the bar.
|
@matthewevans I've update all. Ready to review |
matthewevans
left a comment
There was a problem hiding this comment.
Escalated AI-policy review at head 60aae99: all checks pass. Prior dormant-policy/vacuous-gate blocker resolved — enchantress-mirror wired into DEFAULT_QUICK_FILTER with a real seeded 10-game baseline, faithful structural parity with the shipped affinity-mirror/ArtifactSynergyPolicy axis. Decision-space complete, bail-paths return PolicyVerdict::neutral (no sentinels), determinism sound (commutative score-summing, ordered slice, seeded gate), knob registered UNTUNED with no-regression evidence, ai-gate paired-seed green (0 FAIL). The only residual — no in-CI end-to-end firing test for a nudge-band policy — is the identical accepted tradeoff from the merged artifacts sibling and is honestly documented. Enqueuing.
Summary
Closes #3491
Adds an enchantments-matter deck-feature axis to
phase-ai— anEnchantmentsFeaturedetector (enchantment density + enchantress/constellation payoffs; all structural, no name matching) and a payoff-gatedEnchantmentsPayoffPolicythat values casting enchantments only when the deck actually contains enchantment payoffs. This lets the AI recognize and play Enchantress / constellation decks instead of treating enchantments generically.Verified genuinely absent before building: no enchantments feature, no enchantress/constellation policy, no synergy detector, no eval dimension.
synergy::detect_spellcastis gated on instant/sorcery density (spell_count < 8), so it never fires for an enchantment deck — confirming this is a real gap, not already-covered (unlike the graveyard axis).Implementation method (required)
How was the engine/parser logic in this PR produced? Check exactly one:
/engine-implementerpipeline (plan → review-plan → implement → review-impl → commit)/engine-implementer— explain why belowIf you did not use
/engine-implementer, state why (e.g. frontend-onlychange, docs/CI/tooling change, release chore, or a fix too small to warrant
the pipeline):
Note
Any change to
crates/engine/game logic — parser, effects, resolver,targeting, rules behavior — is expected to go through
/engine-implementer.The "not used" box is for changes that genuinely fall outside that scope.
What changed
New files (impl is inline-test-free → tests live in separate files):
features/enchantments.rs—EnchantmentsFeature { enchantment_count, payoff_count, commitment }+ structuraldetect(). Payoffs detected structurally:TriggerMode::ChangesZone+destination = Battlefield+ avalid_cardfilter matching an Enchantment you control (mirrorslandfall.rs's land-ETB shape).TriggerMode::SpellCast/SpellCastOrCopy+ avalid_cardfilter referencingTypeFilter::Enchantment. (The runtimeTriggerMode::SpellCastis a unit variant; the spell type lives onvalid_card, not on the mode.)policies/enchantments_payoff.rs—EnchantmentsPayoffPolicy: payoff-gated activation (opts out entirely whenpayoff_count == 0, so non-payoff decks are unaffected); values casting an enchantment via a config-routed, band-helper-scored delta.features/tests/enchantments.rs,policies/tests/enchantments_payoff.rs— tests.Wiring edits:
features/mod.rs(pub mod+ re-export +DeckFeaturesfield +analyze()),policies/mod.rs,policies/registry.rs(PolicyId::EnchantmentsPayoff+ default registration),config.rs(newenchantment_cast_bonusknob, registered UNTUNED), and the twotests/mod.rsdeclarations.CR references
Cited in doc comments for the structural-detection rationale (no engine rules logic added):
Verification
Tilt was down, so direct cargo (the sanctioned fallback). Engine untouched; changes are additive to
phase-ai:0 FAIL(one non-significant WARN — red-mirror / AggroPressure, p=0.25, L→W=1, favoring the new code); CI's requiredPaired-seed AI gatecheck re-runs this on the PR head. The policy is correctly inert on the aggro suite (payoff-gated; that deck has no enchantment payoffs), confirming non-enchantment decks are unaffected. The newenchantment_cast_bonusknob is registered UNTUNED with this no-regression evidence, pending CMA-ES calibration.no_name_matching(structural detection, no card-name classification),score_contract(delta routed through the config knob), and thePolicyPenaltiesfield-coverage test.Note
The quick AI-gate suite contains only the red-mirror (aggro) matchup, which has no enchantment payoffs — so the gate proves no regression + correct inertness, not measurable improvement on enchantress decks (no constellation/enchantress matchup exists in the suite). The knob is UNTUNED with this evidence, pending a paired-seed calibration once such a matchup is added.