ship/ai suggestions batch - #1661
Merged
Merged
Conversation
matthewevans
commented
May 31, 2026
Member
- fix(ai): combat correctness — attack planeswalkers, avoid commander trades, gate lifelink
- fix(ai): penalize self-protection ACTIVATIONS with no threat (not just spells)
- feat(ai): PlaneswalkerLoyaltyPolicy — use the planeswalker, don't sac it for a trick
- feat(ai): EquipmentPriorityPolicy — stop the every-turn equip shuffle
- feat(ai): LandSequencingPolicy — play a normal land before a Karoo bounce-land
- fix(engine): smarter auto-tap source order — colorless-for-generic, spare creatures
- feat(ai): don't pay for activations whose payoff is gated off (ConditionGatedActivation)
- fix(engine): make Omo's everything counter grant all land/creature types
- feat(ai): draft bots value and play nonbasic fixing lands
…rades, gate lifelink Three attack-declaration correctness fixes in combat_ai (reported in the #ai-suggestions channel). Attack declaration is decided directly by combat_ai, not the policy registry, so these are surgical heuristics there. - #8 lifelink: gate the lifelink attack bonus on free_damage || favorable_trade || attacker_survives, so it can no longer justify a pure-loss chump block (CR 702.15b: life is still gained on a simultaneous trade, so even/surviving swings are unaffected — but throwing the creature away for nothing is not worth the life). - #6 commander: don't trade the commander into a block it does not survive unless pushing lethal (CR 903.8 commander tax). Also excluded from the desperation alpha-strike fallback, computed before its cost/benefit math so a wide board cannot leak it back in. - #5 planeswalkers: consume the engine's pre-computed valid_attack_targets to redirect the fewest large attackers (largest-power-first) needed to KILL the highest-loyalty opponent planeswalker, single-opponent path only; never dilutes a lethal/near-lethal swing and never empties the face. Multiplayer pods and Battle targets deferred. Plan reviewed to convergence via /review-plan (3 rounds, perf-verified: no per-node GameState clone, projection, or O(N^2) battlefield sweep). New tests drive the chooser through the real engine target list and are mutation-checked (reverting either subtle gate fails exactly its discriminating test). clippy + test-ai green.
…t spells)
Extend ReactiveSelfProtectionPolicy to fire on ActivateAbility, not only
CastSpell. The AI repeatedly paid "discard a card: ~ gains protection from
everything" until hand-empty, and activated Sylvan Safekeeper ("sacrifice a
land: target creature you control gains shroud") on turn 1 for nothing — the
same class as casting Teferi's Protection into an empty board, which the policy
already handled for spells.
- decision_kinds() now includes ActivateAbility; the verdict guard accepts both
CastSpell and ActivateAbility.
- Classifier extension (required): a GenericEffect static scoped to ParentTarget
grants to whatever the parent ability targets, so self-scoping is decided by
the enclosing GenericEffect.target — this covers the "target creature you
control gains <defensive keyword>" class (Sylvan Safekeeper). The discard-cost
"~ gains X" class already lowered to SelfRef and was caught.
- Reuses is_self_protection_effect + any_immediate_threat unchanged; the
expensive threat check stays gated behind the cheap classifier, so there is no
per-candidate cost for non-protection activations.
Plan reviewed to convergence via /review-plan (caught that Sylvan Safekeeper's
grant is ParentTarget-scoped, which the classifier would have silently missed).
Two discriminating tests (SelfRef + ParentTarget shapes) drive the verdict with
a real ActivateAbility candidate and are mutation-checked. clippy + test-ai green.
… it for a trick
New tactical policy on ActivateAbility for planeswalker loyalty abilities
(reported in #ai-suggestions): the AI didn't use a loyalty ability the turn a
walker landed (#4d), and fired value-negative minuses that sacrificed the
walker for a single-target combat trick instead of the value plus (#4e,
Quintorius "-4: target creature gains double strike").
Blunder-only design: the ONLY penalized case is sacrificing/crippling the
planeswalker (left at <=1 loyalty) for a single-target combat trick; every
other loyalty activation — plus abilities, removal, emblems/ultimates, tokens,
draw, mass effects, steal, reanimation — earns a modest "use your planeswalker"
bonus. This avoids classifying effect value (which would misclassify the 76
CreateEmblem ultimates as low-value and suppress them); by penalizing only the
combat-trick-sacrifice shape, ultimates and removal are structurally never
penalized. #4d falls out because using a walker beats passing; #4e is the
penalty branch.
is_combat_trick = Effect::Pump gated on effect_polarity == Beneficial (excludes
negative-pump removal like Davriel's -3/-3), OR a GenericEffect granting only
beneficial keyword/+P/+T mods to the single targeted creature (affected ==
ParentTarget, distinguishing it from a Typed{You} team anthem). Reuses
effect_polarity (FreeOutletActivation precedent). CR 606.1.
Plan reviewed to convergence via /review-plan (3 rounds — caught that
effect_profile misses 76 emblem-ultimates, and that an unconditional Pump arm
would penalize negative-pump removal). 9 tests drive the verdict with a real
ActivateAbility candidate, incl. BLOCKER regression guards for emblem-ultimate
and negative-pump removal; the negative-pump guard is mutation-checked. clippy
+ test-ai green.
New tactical policy on ActivateAbility (reported in #ai-suggestions): the AI spent all its remaining mana every turn moving an Equipment between creatures for no board improvement. The AI reaches equip via ActivateAbility on the equip ability (effect Effect::Attach), where mana is committed and PassPriority is a sibling candidate — so the policy fires there and penalizes the activation when the equipment is already on the best body, making the AI keep its mana. - Unattached equip and genuine upgrades are neutral (0.0); the policy never rewards equipping (the problem is over-equipping). Only a re-equip with no bigger body to move to is penalized. - "Bigger body" compares base_power, NOT live power: the host's live power already includes this equipment's own +P buff (folded in via attached_to by the layer system), so comparing live power would make the equipped host out-power every bare upgrade target and wrongly penalize legitimate moves. base_power is the printed-baseline proxy, consistent on both sides. Guard: source has subtype "Equipment" + ability effect is Effect::Attach (CR 301.5: Equipment hosts are creatures). Performance: gated behind that cheap guard, then one own-creature base-power scan; no clone/projection. Plan reviewed to convergence via /review-plan (caught that live `power` includes the equipment's own buff, which would suppress every upgrade). The equip_upgrade_allowed test is built on a buffed host (live 4, base 2) with a base-3 target, so it fails under live-power and passes only under base_power — self-discriminating. clippy + test-ai green.
…unce-land New tactical policy on PlayLand (reported in #ai-suggestions "AI Cast & Bounce lands"): the AI played a Karoo bounce-land (Simic Growth Chamber) first when it should play a different land first, losing tempo. PlayLand was declared in BoardDevelopmentPolicy.decision_kinds() but never scored, so land choice was softmax noise; this fills the gap for the bounce-land case. When the land being played is a self-bouncing land AND a non-bouncing land is also in hand, deprioritize the bounce-land (-1.5) so the non-bounce land wins the argmax and is played first. When the bounce-land is the only land in hand, no penalty — it plays normally. The deferral is recoverable (within-turn reorder only). Detection is structural (no card names): an ETB trigger (ChangesZone, destination Battlefield, valid_card SelfRef) whose execute chain bounces a Land you control (controller == You). Reuses ability_chain::collect_chain_effects. This matches the whole Ravnica/MOM Effect::Bounce bounce-land cycle; the Mercadian Karoo/Coral Atoll sacrifice-self cycle (Effect::Sacrifice) has no sequencing downside and is deliberately not matched. Deferred (#4b, separate SelectTarget decision, documented): when the bounce ETB returns "a land you control," the AI should pick the least-useful land, not the just-played bounce-land — needs its own target-selection policy. Plan reviewed to convergence via /review-plan (confirmed trigger_definitions are populated on hand objects, so detection fires at the PlayLand step; corrected a false coverage claim and tightened the target to controller==You). Tests drive a real PlayLand candidate; the SGC-detected penalty test plus the non-bounce-ETB na test together discriminate the structural matcher. clippy + test-ai green.
…pare creatures
The auto-tap planner's card_tier sort axis ignored two cheap, in-payment
signals, so it would drain a dual land's colored capacity to pay a generic
cost (when a painland's free {C} row was available) and tap a creature mana
dork when a non-creature rock could pay the same shard.
Refine the card_tier middle key of auto_tap_mana_sources_inner's sort closure
into a 6-tier ladder (penalty axis unchanged):
0 free-colorless land row (ideal generic filler — commits no colored
production a later shard in this same payment needs)
1 other land row
2 non-land non-creature mana source (rock / Signet)
3 non-land creature mana dork — CR 509.1a: keep would-be blockers untapped
4 land-creature (animated manland) — CR 509.1a
5 deprioritized own-source
The colorless-row signal is read per-row off the existing ManaSourceOption
fields (atomic_combination + mana_type), not the per-object flexibility flag
(which is stamped identically on every row and can't tell a painland's {C}
row from its colored rows). O(1) per source, no hand lookahead — safe in the
AI cost-projection hot loop.
Two discriminating tests (mutation-checked: both fail on the old 4-tier
ladder): auto_tap_uses_colorless_row_for_generic_preserving_dual ({1}{G}) and
auto_tap_prefers_rock_over_creature_dork ({G}).
Reported via Discord #ai-suggestions ("Auto Cast Mana Selections").
…ionGatedActivation)
The AI activated hideaway payoff abilities (e.g. Mosswort Bridge's "{G},{T}:
play the exiled card if your creatures' total power is 10 or greater") every
turn even far under the threshold, burning mana for an effect that does
nothing. The engine is rules-correct here — per CR 602.5 / the Shelldock Isle
ruling the ability is legal to activate regardless; only the CastFromZone
effect is gated at resolution (CR 608.2c) — so this is purely an AI-value miss.
New ConditionGatedActivationPolicy (DecisionKind::ActivateAbility): when an
activated ability with a cost has a top-level intervening-if condition that is
currently false, apply a soft penalty so PassPriority (or a better line) wins.
Generalizes to the whole "Cost: do X if [board condition]" class — every
hideaway land and beyond, not one card. Soft (not Reject) because an ability's
cost can change the very thing its condition checks (sacrifice-then-"if you
control no creatures"); the condition is read pre-cost.
Condition evaluation is delegated to a new public engine helper
`ability_condition_currently_met`, which wraps `effects::evaluate_condition`
behind a conservative allowlist: it judges only board/controller-relative
QuantityCheck conditions and returns None for anything needing resolution-time
context (chosen targets, cast/trigger event), so the policy never guesses.
Tests: engine helper unit test (board-relative Some(false)/Some(true),
no-condition None, target-relative None) + 5 policy tests. Discriminating via
unique reason-kind sentinels.
Reported via Discord #ai-suggestions ("Mosswort Bridge").
Omo, Queen of Vesuva placed an "everything counter" that did nothing: the two
type-granting static abilities silently mis-parsed, and the trigger only
targeted a land (the "and up to one target creature" half became Unimplemented).
Three fixes:
- Trigger: parse "put a counter on each of up to one target A and up to one
target B" as two PutCounter siblings, each with its own optional target.
Also recover the PRIMARY clause's "up to one" cardinality, which
try_parse_verb_and_target dropped when lowering to ZoneCounterProxy — a
latent bug across the whole compound-PutCounter class, not just Omo.
- Land static: "is every land type" now lowers to a new
ContinuousModification::AddAllLandTypes (all 17 land subtypes per CR 205.3i),
not the no-op AddType{Land} it produced before. The new Layer-4 marker
iterates the canonical card_type::LAND_SUBTYPES; basic types among them grant
mana abilities automatically via the existing intrinsic-mana pass (CR 305.7).
Distinct from AddAllBasicLandTypes (CR 305.6, 5 basic types).
- Creature static: "each nonland creature with an everything counter on it is
every creature type" now parses (counter-conditioned subject built from the
existing TypeFilter::Non(Land) + parse_counter_suffix) and maps to the
existing AddAllCreatureTypes.
Tests: parser units (both statics + the two-target trigger), a layer test
(counter -> all types; mutation-checked that no-counter objects gain nothing),
and a full-pipeline integration test (Omo enters -> resolve trigger -> select
land + creature -> assert all land/creature types via the layer system).
Reported via Discord #ai-suggestions ("AI Cast & Bounce lands" / Omo).
Draftbots never picked or played nonbasic fixing lands: evaluate_draft_card
zeroed every non-creature land, and the deck-builder filled the manabase with
basics only, ignoring drafted duals.
- Pick value: a nonbasic land that taps for 2+ colors now scores `fixing_land`
(~card-draw value — above filler, below playables/removal), so bots take
duals mid-pack instead of wheeling them. Mono/colorless lands stay at 0.
- Deck build: suggest_deck now admits on-color drafted fixing lands into the
manabase, each replacing one basic so the 40-card total never drifts. Gated
to the standard-basics fill policy (a custom addable policy supplies its own
lands) and to drafted copies the player actually owns.
New shared `mana_colors::land_produced_color_types` (parts-based core over
subtypes + abilities) is the single "what colors does this land make" signal
for both the pick value and the manabase, unioning intrinsic basic-land-type
mana with Effect::Mana abilities (incl. filter-land ChoiceAmongCombinations).
Dedups the mulligan keepables' private copy of the same logic.
Tests: draft_eval (subtype dual, ability dual, mono, colorless-only) and
suggest (on-color admitted, off-color rejected, total conserved, no-db noop).
Reported via Discord #ai-suggestions ("Nonbasic Lands in Draft").
matthewevans
enabled auto-merge
May 31, 2026 14:33
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements full-pipeline support for Omo, Queen of Vesuva, introducing the parsing, layer evaluation, and integration tests for everything counters that grant all land and creature types. It also delivers major AI enhancements, including a shared mana_colors module for identifying fixing lands in draft and mulligan evaluation, and several new tactical policies: planeswalker loyalty-ability optimization, equipment re-equip prevention, bounce-land sequencing, and condition-gated activation prevention (e.g., for hideaway lands). Additionally, the combat AI is upgraded to support planeswalker attack redirection, prevent unfavorable commander trades, and avoid throwing lifelinkers into pure-loss blocks. I have no feedback to provide as no review comments were submitted.
…in became QuantityExpr MultiTargetSpec.min was parameterized from i32 to QuantityExpr on main (CR 115.1d up-to-N targeting). The Omo dual-target parser test compared s.min == 0 against the old integer type, which no longer compiles against the new QuantityExpr min (E0308). Use a variant match for the Fixed(0) case, matching how the rest of the engine constructs/handles QuantityExpr.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.