feat(phase-ai): add cycling deck-feature axis + CyclingPayoffPolicy

[minion1227]

Tier: Frontier
Model: claude-opus-4-8

Summary

Adds a cycling deck-feature axis (CyclingFeature) and its companion CyclingPayoffPolicy — CR 702.29 "cycling matters."

CR 702.29a: cycling is card-neutral selection, so the AI's generic priors undervalue it. CyclingDisciplinePolicy only adds patience (don't cycle away a needed land), and self_cost_value explicitly defers cycling value (self_cost_cycling_deferred). Nothing modelled the upside: with a "whenever you cycle a card" engine on the battlefield (Astral Drift, Drannith Stinger, New Perspectives — CR 702.29c/d), every cycle is a repeatable value trigger, so the AI should cycle eagerly into it.

Structural detection, no card-name matching:

Axis	AST surface
sources	Keyword::Cycling(_) / Keyword::Typecycling { .. } (CR 702.29a/e) — the cyclable cards
payoffs	a TriggerMode::Cycled / CycledOrDiscarded trigger (CR 702.29c/d) that is controller-scoped (valid_target None/Controller) and not self-referential (valid_card != SelfRef), so a one-shot "when you cycle THIS card" bonus is excluded — only the repeatable battlefield engines count

Commitment is a geometric mean over (source, payoff) — both mandatory: cyclers with no engine is just smoothing (CyclingDisciplinePolicy governs it), an engine with no cyclers never fires. Calibrated so a dedicated cycling shell clears the floor; a control deck splashing two cycling lands stays below it.

Policy rewards a Cycling activation when the AI controls a LIVE engine. Detection is structural over the permanent's own trigger_definitions (no name matching), and liveness is delegated to engine::game::triggers::hypothetical_trigger_fireable — the engine-owned authority for “could this trigger still fire AND resolve to an effect?”, which reuses the live pipeline's own constraint check plus a CR 603.3d target preflight. The policy holds no eligibility rules of its own. The positive score composes with CyclingDiscipline's patience penalty so a payoff deck cycles into its engine while a smoothing-only deck stays patient.

Files changed

• crates/engine/src/game/triggers.rs — hypothetical_trigger_fireable (new, the shared authority); check_trigger_constraint_with_ref now takes event: Option<&GameEvent> (Some = real evaluation, None = hypothetical)
• crates/engine/src/game/ability_utils.rs — execute_targets_satisfiable (new, the CR 603.3d preflight)
• crates/phase-ai/src/features/cycling.rs (new) · features/tests/cycling.rs (new)
• crates/phase-ai/src/policies/cycling_payoff.rs (new) · policies/tests/cycling_payoff.rs (new)
• crates/phase-ai/src/features/mod.rs, features/tests/mod.rs, policies/mod.rs, policies/registry.rs, policies/tests/mod.rs, config.rs

CR references

• CR 702.29a — cycling is an activated ability that discards the card to draw
• CR 702.29c — "when you cycle this card" trigger
• CR 702.29d — "whenever you cycle or discard a card" trigger
• CR 702.29e — Typecycling
• CR 603.2–603.4 — trigger constraints; intervening-“if” (conservatively treated as not-live in the preflight)
• CR 603.3d — a triggered ability with no legal choice for a required target is removed from the stack rather than producing its effect
• CR 113.1a — building a resolved ability from its definition, preserving sub-ability chains

Every number grep-verified against docs/MagicCompRules.txt.

Implementation method (required)

Method: /add-ai-feature-policy

Track

Developer

LLM

Model: claude-opus-4-8
Thinking: high

Performance

verdict() runs per candidate per search node. The card-local check — the candidate is a Cycling-tagged activation — runs FIRST and rejects every other activation; only then does it scan the battlefield, and only in a deck whose activation floor is already cleared. Each structurally matching engine is then confirmed live by the engine authority, which orders its work cheapest-first: the trigger's constraint is evaluated before any targeting work, and target legality resolves through a cheap single-slot check whose “undecidable” shapes alone reach the full legal-assignment search. A no-target payoff (Drannith Stinger) short-circuits before any of it — it has no target slot to satisfy.

Verification

• cargo test -p phase-ai --lib — 1592 passed; 0 failed; 8 ignored (19 cycling_payoff policy tests, incl. this round's six trigger-eligibility regressions: no-legal-target neutral / legal-target rewarded, MaxTimesPerTurn below-cap rewarded / at-cap neutral, and multi-slot no-legal-target neutral / legal-targets rewarded)
• cargo clippy -p engine -p phase-ai --all-targets --features proptest -- -D warnings — clean
• superseded: cargo test -p phase-ai --lib — 1551 passed; 0 failed (12 feature tests: each detection axis, the self-cycle-bonus and opponent-scope exclusions, payoff_names dedup, both-pillars-mandatory collapse, and calibration; 6 policy tests: activation gate, each verdict branch, and a registry-routed regression asserting PolicyId::CyclingPayoff is registered and the ActivateAbility routing delivers the engine reward)
• cargo clippy -p phase-ai --all-targets -- -D warnings — clean
• cargo fmt --all — clean
• cargo ai-gate — the policy activates only on a cycling-payoff deck, which none of the three gate matchups (mono-red burn, affinity, Selesnya enchantress) is, so CyclingPayoffPolicy::activation returns None and it never fires; CI's paired-seed AI gate confirms on this head.

Notes

• cycling_payoff_bonus is registered UNTUNED pending a paired-seed calibration.
• Tests live in features/tests/ and policies/tests/ per the house convention.
• Boundary with spellslinger_prowess: spellslinger counts spell-cast triggers; cycling triggers on the cycling keyword action (CR 702.29c), a disjoint event. A cycling instant/sorcery can read on both axes — the overlap is intentional and the axes stay independent.
• Distinct from CyclingDisciplinePolicy (patience, a penalty) — this adds the payoff upside; the two compose.

Summary by CodeRabbit

• New Features
  • Added cycling deck feature detection to quantify cyclable sources, cycling payoff engines, and a derived “commitment” score.
  • Added a tactical cycling payoff policy that rewards cycling activations when matching payoff engines are present (gated by commitment and capped).
  • Added a tuning parameter (cycling_payoff_bonus) to control cycling payoff reward strength.
• Bug Fixes
  • Ensure decks with no eligible nonland cards produce a 0.0 cycling commitment (no NaN).
• Tests
  • Added unit and policy tests covering detection/commitment scoring, activation gating, verdict outcomes, and policy registry routing.

[superagent-security]

🚨 Contributor flagged. Click here for more info: Superagent Dashboard

[coderabbitai]

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

• @coderabbitai resume to resume automatic reviews.
• @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

• ▶️ Resume reviews
• 🔍 Trigger review

📝 Walkthrough

Walkthrough

Adds structural cycling deck detection, integrates it into deck features, and introduces a registered tactical policy that rewards cycling activations when active cycling payoff engines exist.

Changes

Cycling payoff behavior

Layer / File(s)	Summary
Cycling feature detection crates/phase-ai/src/features/cycling.rs, crates/phase-ai/src/features/tests/cycling.rs	Detects cycling sources and qualifying payoff triggers, computes commitment from normalized densities, and tests classification and boundary cases.
Deck feature integration crates/phase-ai/src/features/mod.rs, crates/phase-ai/src/features/tests/mod.rs	Exposes CyclingFeature, adds it to DeckFeatures, populates it during analysis, and registers its tests.
Cycling payoff policy and registry crates/phase-ai/src/config.rs, crates/phase-ai/src/policies/..., crates/phase-ai/src/policies/tests/...	Adds the cycling payoff penalty, scores cycling activations when live matching engines exist, registers the policy, and tests activation, verdict, and registry routing.

Estimated code review effort: 3 (Moderate) | ~25 minutes

Sequence Diagram(s)

sequenceDiagram
  participant PolicyRegistry
  participant CyclingPayoffPolicy
  participant DeckFeatures
  participant GameState
  PolicyRegistry->>CyclingPayoffPolicy: route ActivateAbility candidate
  CyclingPayoffPolicy->>DeckFeatures: read cycling commitment
  CyclingPayoffPolicy->>GameState: scan AI-controlled payoff engines
  GameState-->>CyclingPayoffPolicy: matching engine count
  CyclingPayoffPolicy-->>PolicyRegistry: return capped cycling payoff verdict

Loading

Possibly related PRs

• phase-rs/phase#6543: Both changes extend PolicyPenalties and the untuned policy-penalty registry in crates/phase-ai/src/config.rs.
• phase-rs/phase#6544: Both changes add a policy-tuning field to the shared configuration and registry.
• phase-rs/phase#6590: Both changes update shared policy-penalty configuration and untuned-field registration.

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 60.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately summarizes the main change: adding the cycling deck feature axis and the CyclingPayoffPolicy.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[coderabbitai]

Actionable comments posted: 1

🤖 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/policies/cycling_payoff.rs`:
- Around line 84-109: Update the engine-counting and bonus flow in
cycling_payoff.rs to retain typed payoff requirements and validate runtime
target legality/value before rewarding each matched engine, returning neutral
when no usable payoff exists; implement this as a composable rule rather than an
Astral Drift special case. In
crates/phase-ai/src/policies/tests/cycling_payoff.rs lines 155-173, add a legal
creature target to the positive case and add a regression asserting neutral when
no valid target exists.

🪄 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: 56444d39-ec9c-41be-bf02-f5cf5a1717b3

📥 Commits

Reviewing files that changed from the base of the PR and between c8fdd42 and c5d6099.

📒 Files selected for processing (10)

• crates/phase-ai/src/config.rs
• crates/phase-ai/src/features/cycling.rs
• crates/phase-ai/src/features/mod.rs
• crates/phase-ai/src/features/tests/cycling.rs
• crates/phase-ai/src/features/tests/mod.rs
• crates/phase-ai/src/policies/cycling_payoff.rs
• crates/phase-ai/src/policies/mod.rs
• crates/phase-ai/src/policies/registry.rs
• crates/phase-ai/src/policies/tests/cycling_payoff.rs
• crates/phase-ai/src/policies/tests/mod.rs

[matthewevans]

Changes requested — the cycling-payoff bonus currently trusts a permanent name instead of the live trigger that makes cycling valuable.

🟡 Non-blocking

[MED] Name-only engine detection can reward cycling when the claimed payoff has no usable live trigger. Evidence: crates/phase-ai/src/policies/cycling_payoff.rs:84-109 counts a battlefield object solely by its name, while crates/engine/src/game/game_object.rs:480-487 exposes trigger_definitions as the live trigger authority. The tests construct a bare name-only Astral Drift at crates/phase-ai/src/policies/tests/cycling_payoff.rs:54-71, then assert a reward at :155-173 and through the registry at :218-245; that fixture has no trigger/effect or legal target. Why it matters: the AI can prefer cycling for a purported payoff that cannot actually produce value. Suggested fix: inspect the structural live trigger/effect and target usability before awarding the bonus; preserve no-target payoffs such as Drannith Stinger, and add positive real-trigger/legal-target plus no-target-neutral regressions.

Recommendation: request changes — make payoff detection structural and cover both usable and unusable live-trigger cases, then request another review.

[matthewevans]

Changes requested — the rebased head still awards cycling payoff from a permanent name without proving a usable live payoff exists.

🟡 Non-blocking

[MED] Cycling payoff detection is still name-only rather than structural. Evidence: crates/phase-ai/src/policies/cycling_payoff.rs:84-109 counts engines solely by permanent name and does not inspect a live trigger, effect, or target legality. crates/phase-ai/src/policies/tests/cycling_payoff.rs:54-71 still creates a bare name-only Astral Drift, then :155-173 and :227-245 reward it directly and through the registry. Why it matters: the AI can prefer cycling when the claimed payoff has no usable live trigger. Suggested fix: check the structural live trigger/effect and target usability before awarding the bonus; preserve no-target payoffs such as Drannith Stinger, and add real-trigger/legal-target and no-target-neutral regressions.

Recommendation: request changes — make payoff detection structural and cover usable and unusable live-trigger cases, then request another review.

[minion1227]

Fixed at head 1bd0d3749.

[MED] Payoff detection is now structural, not name-based

You're right — the policy trusted feature.payoff_names and could reward cycling for a permanent that only shares an engine's name. It now re-classifies each permanent the AI controls structurally against its live trigger_definitions (the runtime trigger authority), via the same is_cycle_payoff_parts predicate detection uses:

obj.controller == ctx.ai_player
    && is_cycle_payoff_parts(
        obj.trigger_definitions.iter_unchecked().map(|entry| &entry.definition),
    )

A name match is no longer sufficient — the object must actually carry a controller-scoped, non-self Cycled/CycledOrDiscarded trigger. payoff_names is removed from CyclingFeature (the policy was its only consumer).

On target usability: I deliberately do not check target legality. Per the add-ai-feature-policy performance rules, a per-candidate find_legal_targets sweep is a documented inner-loop landmine — and, as you noted, it would wrongly drop no-target payoffs like Drannith Stinger ("deals damage to each opponent"). The live-trigger check is the structural gate; presence of the engine is the signal.

Regressions added:

• name_only_impostor_without_a_live_trigger_is_neutral — a permanent named "Astral Drift" with no trigger scores cycling_payoff_no_engine.
• no_target_payoff_still_rewards — a Drannith-Stinger-shape trigger whose effect chooses no target still rewards.

Full 1581-test phase-ai suite passes; clippy -p phase-ai --all-targets -D warnings clean. CI's paired-seed AI gate passed on the prior head (the policy is inert on all gate decks).

[coderabbitai]

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)

crates/phase-ai/src/features/cycling.rs (1)

131-146: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Handle decks with zero nonland cards before normalizing.

A non-empty all-land deck reaches compute_commitment with total_nonland == 0. The density division can produce NaN; because CyclingPayoffPolicy::activation only checks commitment < CYCLING_PAYOFF_FLOOR, NaN bypasses the floor and can propagate into policy scoring. Define an explicit zero-denominator result and add an all-land regression test.

As per path instructions, boundary and empty-input cases must be covered.

🤖 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/cycling.rs` around lines 131 - 146, Update
compute_commitment to return an explicit zero commitment when total_nonland is
zero before calculating either density, preventing NaN from reaching
CyclingPayoffPolicy::activation. Add a regression test covering a non-empty
all-land deck and verify the commitment remains below the activation floor
without propagating invalid 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.

Outside diff comments:
In `@crates/phase-ai/src/features/cycling.rs`:
- Around line 131-146: Update compute_commitment to return an explicit zero
commitment when total_nonland is zero before calculating either density,
preventing NaN from reaching CyclingPayoffPolicy::activation. Add a regression
test covering a non-empty all-land deck and verify the commitment remains below
the activation floor without propagating invalid values.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 15d34f3f-d9ac-46e5-b6a3-5786698bb41a

📥 Commits

Reviewing files that changed from the base of the PR and between 8d5fdc8 and 1bd0d37.

📒 Files selected for processing (4)

• crates/phase-ai/src/features/cycling.rs
• crates/phase-ai/src/features/tests/cycling.rs
• crates/phase-ai/src/policies/cycling_payoff.rs
• crates/phase-ai/src/policies/tests/cycling_payoff.rs

[minion1227]

Addressed at head f36177e0a.

All-land-deck / NaN boundary (CodeRabbit)

Verified: the NaN path is already closed. commitment::density_per_60 guards total_nonland == 0 and returns 0.0, and commitment::geometric_mean returns 0.0 for any non-positive pillar — so an all-land deck (including cycling lands, where source_count > 0 but total_nonland == 0) yields a clean 0.0, never NaN. I kept the zero-denominator handling in density_per_60 as the single authority rather than duplicating the guard in compute_commitment.

Added the requested regression — all_land_deck_is_zero_not_nan — which builds a non-empty all-land deck with a cycling land and asserts !commitment.is_nan() and commitment == 0.0.

Full phase-ai suite green; clippy clean.

(Note: the standing CHANGES_REQUESTED is the round-1 name-vs-trigger review, which was addressed in 1bd0d3749 — the payoff detection is now structural over live trigger_definitions.)

[matthewevans]

Changes requested — the live cycling-payoff scan ignores trigger eligibility.

🟡 Blocker

[MED] A rate-limited cycling engine still earns a bonus after its trigger has already fired. crates/phase-ai/src/features/cycling.rs:115-129 classifies a payoff only by structural fields, and crates/phase-ai/src/policies/cycling_payoff.rs:78-92 rewards every live match. The engine's trigger authority checks TriggerDefinition.constraint; specifically OncePerTurn consults the fired-trigger ledger in crates/engine/src/game/triggers.rs:8005-8035. Thus Valiant Rescuer's second cycle receives a payoff bonus even though the payoff cannot trigger.

Use the authoritative fireable-trigger/occurrence eligibility for the policy scan, and add a regression where the second cycle in the same turn is neutral.

[minion1227]

Fixed at head 9d07a668c.

[MED] Payoff scan now respects live trigger eligibility

You're right — the scan matched an engine only by its structural trigger shape, so a rate-limited engine (Valiant Rescuer's "only once each turn") still earned a bonus on the second cycle even though the trigger can't fire again.

The battlefield scan now pairs the structural classifier with live firing eligibility per trigger entry, consulting the engine's authoritative ledgers (not re-deriving eligibility):

obj.trigger_definitions.iter_unchecked().any(|entry| {
    is_cycle_payoff_trigger(&entry.definition)
        && trigger_still_fireable(ctx.state, obj, entry)
})

trigger_still_fireable keys the entry via GameObject::trigger_definition_ref and checks triggers_fired_this_turn (OncePerTurn, CR 603.4) / triggers_fired_this_game (OncePerGame). A constraint whose eligibility is a value nuance rather than a hard on/off (e.g. MaxTimesPerTurn with remaining fires, NthDrawThisTurn) is treated as live — those aren't the "already exhausted" case. trigger_is_cycle_payoff is exposed as is_cycle_payoff_trigger so the per-entry check can pair shape with eligibility.

Regressions added:

• rate_limited_engine_already_fired_this_turn_is_neutral — a once-per-turn engine whose trigger key is in triggers_fired_this_turn scores cycling_payoff_no_engine.
• rate_limited_engine_not_yet_fired_rewards — the same engine, unfired, still rewards.

35 cycling tests pass; clippy -p phase-ai --all-targets -D warnings clean.

[matthewevans]

Changes requested — this branch needs a current-base rebase, and the payoff score still counts unavailable trigger value.

🔴 Blocker

[HIGH] The branch is based before merged #6681 and drops the ManaBrew v2 adapter on reconciliation. The PR base is one commit ahead of this branch's merge base; #6681's merged adapter update changes crates/manabrew-compat/src/lib.rs by roughly 3.4k lines. Rebase onto current main and preserve that adapter before further review.

🟡 Blockers

[MED] A target-required payoff such as Astral Drift is counted with no legal target. cycling_payoff.rs:81-94 counts structural, fireable triggers but does not consult the engine's target/execute requirements. A cycle receives an engine bonus when the required trigger target cannot be chosen. Reuse the root targeting/execution authority and add a required-target-absent neutral regression.

[MED] The policy still only models OncePerTurn/Game. trigger_still_fireable at cycling_payoff.rs:110-128 treats time- and max-limited constraints as live, whereas the engine's trigger authority evaluates the complete constraint set (crates/engine/src/game/triggers.rs:8005+). Use full hypothetical fireability and add max/time constraint regressions.

[minion1227]

Follow-up at head 6952cb662: completed the trigger-eligibility check for parity with the #6688 review.

trigger_still_fireable is now exhaustive over TriggerConstraint (no wildcard) rather than only handling OncePerTurn/OncePerGame: it also evaluates OnlyDuringYourTurn / OnlyDuringOpponentsTurn / OnlyDuringYourMainPhase from active_player + phase, and conservatively declines to credit the event/count-dependent constraints (MaxTimesPerTurn, NthSpell/NthDraw, OncePerOpponentPerTurn, AtClassLevel, EventSourceControlledBy) it can't confirm at decision time — so the payoff is never over-credited.

Added once_per_game_engine_already_fired_is_neutral and only_during_your_turn_engine_is_neutral_off_turn. 13 cycling-payoff tests pass; clippy clean.

[matthewevans]

Changes requested — current head 6952cb6627f288ad7134d45788cd440e90044139

The prior ManaBrew stale-base concern is resolved and is not a finding on this head. Two live-trigger-eligibility gaps remain.

🟡 Blocker [MED] Target-required cycling payoffs can still receive value when no legal target exists

crates/phase-ai/src/policies/cycling_payoff.rs:82-107 rewards a target-required payoff trigger without consulting the engine’s execute/target-legality path. The engine removes a trigger that requires an unavailable target rather than allowing it to produce an effect (crates/engine/src/game/engine.rs:8775-8807; CR 603.3d). The policy can therefore value a cycling trigger that cannot actually fire.

Please make hypothetical payoff fireability use the engine’s authoritative target/execute legality rather than a policy-local approximation. Add runtime coverage showing: (1) a required target with no legal target is neutral (no payoff bonus), and (2) the same payoff is positive when a legal target is available.

🟡 Blocker [MED] MaxTimesPerTurn is treated as categorically unavailable

crates/phase-ai/src/policies/cycling_payoff.rs:139-146 rejects every MaxTimesPerTurn trigger. That is stricter than the runtime engine, which permits the trigger while its observed count remains below the configured maximum (crates/engine/src/game/triggers.rs:8139-8147). This creates a false negative before the cap is exhausted.

Please route both constraint evaluation and target legality through one shared, full hypothetical-fireability authority exposed by/reused from the engine. Add runtime coverage for a positive payoff below the cap and neutral behavior at exhaustion. This shared authority must cover the complete trigger condition, not just selected constraint variants.

Confidence: high. Evidence is the current-head policy logic and the engine’s runtime eligibility paths cited above. This conclusion would be contradicted only if the policy’s hypothetical query already delegates to that same engine authority and its tests drive these unavailable-target and pre-/post-cap states; the current code does not show either.

[minion1227]

Round 5 pushed as e5d0635f7. Both [MED] blockers are fixed by the same structural change: the policy no longer holds any eligibility rules of its own.

The shared authority you asked for

triggers::hypothetical_trigger_fireable(state, source, entry) is now the single place a policy asks "is this on-battlefield payoff live?". It does not re-implement anything:

• Constraints — calls the live pipeline's own check_trigger_constraint_with_ref. That function now takes event: Option<&GameEvent>: Some is a real evaluation (all three live call sites pass Some(event), behaviour unchanged), None is the hypothetical preflight, where the four genuinely event-dependent constraints report NOT-satisfied rather than guessing. So the whole TriggerConstraint set is covered by one authority, not a policy-side subset.
• Intervening-if — a trigger with a condition is treated as not-live (CR 603.4, conservative).
• Target legality — ability_utils::execute_targets_satisfiable (CR 603.3d).

This fixes MaxTimesPerTurn as a consequence, not as a special case. The engine arm permits the trigger while trigger_fire_counts_this_turn < max; delegating picks that up automatically. The policy-local trigger_still_fireable is deleted.

On target legality — I also closed the gap you flagged on the sibling PR

You noted on #6688 that a preflight returning "unknown ⇒ satisfiable" would credit multi-target shapes it can't decide. This PR's copy does not do that — it answers only from confirmed legality:

• the cheap single-slot check is a guard, not the answer;
• every shape it can't decide (multi-slot, relative-controller, distribution, PairWith, …) falls through to has_legal_target_assignment_for_ability, the same full legal-assignment authority the target walk uses;
• the ability is built with build_resolved_from_def (CR 113.1a), so a sub-ability chain's own target slots are preflighted too, not just the root effect's;
• a slot-building error leaves legality unproven and is likewise not credited.

Regressions added

Case	Expected
target-required payoff, no legal target	neutral
same payoff, legal target available	rewarded
MaxTimesPerTurn below cap	rewarded
MaxTimesPerTurn at cap	neutral
multi-slot payoff, no legal target	neutral
multi-slot payoff, legal targets	rewarded

The multi-slot positive case also asserts the fixture really carries two mandatory slots, so the negative case can't silently degrade into a single-slot test.

cargo test -p phase-ai --lib — 1592 passed, 0 failed. cargo clippy -p engine -p phase-ai --all-targets --features proptest -- -D warnings — clean. CR 603.2–603.4, 603.3d and 113.1a grep-verified against docs/MagicCompRules.txt.

One coordination note

hypothetical_trigger_fireable / execute_targets_satisfiable also appear on #6688 (draw-matters), which hit the same problem. This PR carries the improved version (confirmed-legality only, plus sub-ability chains). Whichever lands first should carry them; I'll rebase the other onto it and drop the duplicate — happy to reorder if you'd prefer them landing in a particular sequence.

One thing I want to flag rather than bury: while writing the multi-target regression I first assumed two target creature slots would require two distinct creatures, and the engine disagreed. It's right — CR 115.3 scopes distinctness to "any one instance of the word 'target'", and a sub-ability chain is two instances. The test asserts what the engine actually guarantees, not my original assumption.

[matthewevans]

**Changes requested — the new shared hypothetical-fireability authority incorrectly treats every class-level trigger as unavailable.

🟡 Blocker

[MED] AtClassLevel payoffs are always rejected in the hypothetical scan. Evidence: crates/engine/src/game/triggers.rs:8178-8185 calls check_trigger_constraint_with_ref with source_context set to None, while the TriggerConstraint::AtClassLevel arm at :8140-8142 obtains the source class level exclusively from that context. The existing trigger_source_context_for_latch authority at :717-737 constructs the required current source snapshot. Why it matters: an otherwise usable cycling payoff with an AtClassLevel constraint can never contribute policy value, even when the source is at the required level.

Suggested fix: build and pass trigger_source_context_for_latch(state, source) into the shared constraint check. Add positive and negative AtClassLevel payoff regressions (at the required level and at a different level), and retain the current conservative None handling for genuinely event-dependent constraints rather than treating unknown events as fireable.

Recommendation: request changes — preserve source-sensitive constraints in the hypothetical authority, then request another review.

[matthewevans]

Formatting correction: the opening verdict of my current-head formal review should read Changes requested — the new shared hypothetical-fireability authority incorrectly treats every class-level trigger as unavailable. The blocker, evidence, requested fix, and recommendation in that review are unchanged.

[github-actions]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[matthewevans]

Maintainer hold for current head e5d0635f7ae361a903bf9efff4a9482186840a0e: the required coverage-parse-diff artifact is still Baseline pending, so the card-level parse evidence must publish before the next implementation review. This does not clear or replace the existing formal Changes requested blocker for missing source context in AtClassLevel hypothetical-trigger fireability.

[minion1227]

Fixed in ee0c9c026. You're right — hypothetical_trigger_fireable passed source_context: None, and the AtClassLevel arm reads the class level exclusively from that context, so every class-level payoff was rejected regardless of the actual level.

Fix: it now builds and passes trigger_source_context_for_latch(state, source) (the same current-source snapshot authority the live latch path uses) into the shared constraint check. Source-sensitive constraints (AtClassLevel, CR 716) now read real source state; only the triggering EVENT stays withheld (None), so genuinely event-dependent constraints (NthSpellThisTurn, OncePerOpponentPerTurn, EventSourceControlledBy, …) remain conservatively not-fireable as before.

Coverage: at_class_level_engine_at_required_level_rewards (Class at level 2, payoff needs level 2 → rewarded) and at_class_level_engine_at_wrong_level_is_neutral (Class at level 1, payoff needs level 2 → cycling_payoff_no_engine).

cargo fmt, clippy -p engine/-p phase-ai clean; cycling_payoff suite green (21 tests).

[matthewevans]

Changes requested — the shared fireability check still credits triggers that can only resolve to an unsupported/no-op execution.

🟡 Blocker

[MED] A cycling payoff with no execute is considered fireable and earns policy value despite resolving as TriggerNoExecute. Evidence: crates/engine/src/game/triggers.rs:8195-8198 returns true when TriggerDefinition.execute is None; the live trigger builder instead converts that shape to Effect::Unimplemented { name: "TriggerNoExecute", ... } at crates/engine/src/game/triggers.rs:10294-10310. crates/phase-ai/src/policies/cycling_payoff.rs:89-94 therefore counts the trigger as an engine, while deck detection calls the same structural predicate at crates/phase-ai/src/features/cycling.rs:81-85 and :109-130, so it can also inflate cycling commitment for an unsupported payoff. Why it matters: the AI is rewarded for cycling based on a trigger that cannot produce the claimed payoff; the deck-level feature and live policy then disagree with the engine's supported execution surface.

Suggested fix: make the engine-owned hypothetical-fireability authority reject a missing execute and any root or nested Effect::Unimplemented execute path using one shared recursive support predicate; make the deck classifier use the corresponding structural execute-support check so it does not classify missing/unsupported payoff triggers as engines. Add discriminating negative regressions for (1) a live policy payoff with execute: None, (2) root and nested unsupported execute paths, and (3) deck-feature payoff counting for the same shapes, alongside the existing supported control.

Recommendation: request changes — make executable support part of the shared fireability and deck-payoff contracts, then request another review.

[minion1227]

Fixed in 04f98f517.

Executable-support contract. Correct — hypothetical_trigger_fireable credited a None execute (TriggerNoExecute) and any Effect::Unimplemented execute, and is_cycle_payoff_trigger had the same gap, so both the live bonus and deck commitment could be inflated for a payoff that produces nothing. Added one shared recursive engine predicate ability_utils::ability_definition_supported (rejects Effect::Unimplemented at the root or any nested sub-/else-/mode-ability); both the fireability preflight and the deck classifier now consult it, so the engine's supported-execution surface, the live policy, and the deck feature all agree.

Coverage: no_execute_engine_is_neutral, unsupported_execute_engine_is_neutral (live policy → cycling_payoff_no_engine); payoff_without_execute_is_not_counted, payoff_with_unsupported_execute_is_not_counted (deck payoff_count = 0); supported control retained.

I also threaded the execute's target_constraints through execute_targets_satisfiable and corrected the preflight annotation (CR 113.1a → CR 603.3d) — the same shared-authority fix the maintainer raised on the sibling draw PR, with a constrained two-target neutral/positive pair.

cargo fmt, clippy -p engine/-p phase-ai (lib+tests) clean; cycling suite green (51 assertions).

[matthewevans]

Changes requested — modal payoff legality is still not preflighted on current head 04f98f5177d8a4b3e43f4505efeacc3763e40d52

[MED] A required modal cycling payoff is counted as live even when every selectable mode has an unavailable mandatory target. Evidence: execute_targets_satisfiable builds only the execute root and returns true as soon as target_slot_specs(state, &resolved) is empty (crates/engine/src/game/ability_utils.rs:1335-1337). build_resolved_from_def merely copies mode_abilities onto that root (:167-172), while collect_target_slot_specs descends through sub_ability but never through mode_abilities (:4068-4400). The actual trigger path instead filters each required modal mode with filter_modes_by_target_legality and returns DroppedNoLegalMode when none can be chosen (crates/engine/src/game/triggers.rs:6519-6570). Therefore, on an empty board, a choose one cycling payoff whose only modes each say “target creature …” receives the policy/deck payoff value even though the real trigger is removed before it produces an effect.

Why it matters: this is the same live-versus-hypothetical contract this PR introduces; rewarding an activation for a payoff that the runtime necessarily drops makes the tactical score wrong for a real class of modal triggers. CR 603.3c says an illegal mode cannot be chosen and no-mode triggers are removed; CR 603.3d separately removes a triggered ability if its required stack-time choice has no legal option. I verified both against Wizards’ current Comprehensive Rules (effective June 19, 2026).

Suggested fix: make the hypothetical preflight use/factor the same modal choice + per-mode target-legality authority as dispatch_pending_trigger_context, rather than treating an unresolved modal root as a no-target effect. Add a discriminating regression that creates a required choose one payoff with all target-required modes on an empty board and proves both policy neutrality and the live DroppedNoLegalMode result; retain a legal-target control that earns the bonus.

The existing targeted, chained, constraint, and unsupported-execute tests do not construct a modal execute or run the live trigger-dispatch branch, so they would remain green if this modal discrepancy were reverted.

Quality Gate: FAIL — the current head has no discriminating production-pipeline test for this modal legality contract.

[minion1227]

Round 9 — modal payoff legality is now preflighted through the live authority

Head 6e616c794. Addresses the [MED] modal finding on 04f98f51.

The defect, confirmed. execute_targets_satisfiable built only the execute root, and a modal execute keeps its real effects — and therefore its target slots — in mode_abilities, which collect_target_slot_specs does not descend. target_slot_specs came back empty, the preflight read that as "needs no target", and a required choose one — payoff whose every mode targets a creature scored on an empty board. The live path meanwhile filters each mode and returns DroppedNoLegalMode (CR 603.3c).

The fix is a shared authority, not a second implementation. New ability_utils::resolve_legal_modal_choice performs the whole mode choice in the order the rules announce it:

1. the dynamic "choose up to X" cap — modal_choice_for_player (CR 700.2 + CR 107.3m)
2. the modes unavailable for non-target reasons — compute_unavailable_modes
3. the per-mode target-legality filter — filter_modes_by_target_legality (CR 115.1)
4. the cross-mode assignment cap — modal_choice_with_target_assignment_limit
5. the CR 603.3c verdict: every mode unavailable ⇒ no choice can be announced ⇒ None

dispatch_pending_trigger_context no longer inlines steps 1–5; it calls the authority, still inside the trigger event window, and maps None to DroppedNoLegalMode. execute_targets_satisfiable calls the same function. The hypothetical answer is now the live computation, not a mirror of it, so the two cannot drift.

One behavioural note on the refactor: step 4 now runs inside the event window rather than just after restore_trigger_event_context. That follows the intent already stated above the push — "the triggering event must be live for the ENTIRE choice" — and the assignment cap is part of that choice. Full engine lib suite is green on it.

CR 700.2 placeholder. ability_definition_supported was rejecting every modal payoff outright, because a modal ability carries an Effect::Unimplemented { name: "modal_placeholder" } root by construction. That is not a parser gap — the modes are the effects. The root is exempt when mode_abilities is non-empty, and only then: with no modes, the placeholder itself is what would resolve, so it stays unsupported.

Discriminating regressions

Test	Home	Asserts
modal_trigger_all_target_required_modes_no_targets_is_dropped	triggers.rs	real dispatch_pending_trigger_context returns DroppedNoLegalMode on an empty board
modal_trigger_with_a_legal_target_is_not_dropped	triggers.rs	same trigger, one legal creature ⇒ not dropped
modal_payoff_with_no_legal_mode_is_neutral	cycling_payoff.rs	policy neutral (cycling_payoff_no_engine, delta 0) on that same shape
modal_payoff_with_a_legal_mode_rewards	cycling_payoff.rs	control — legal target ⇒ cycling_payoff_engine_active, delta > 0
modal_payoff_without_modes_is_neutral	cycling_payoff.rs	modal with no modes is not an engine even with a legal target on board
modal_payoff_is_counted	features/tests/cycling.rs	deck classifier still counts a modal payoff (guards the CR 700.2 exemption)
modal_payoff_with_unsupported_modes_is_not_counted	features/tests/cycling.rs	the exemption covers the modal root only, never the modes underneath

Reverting the modal branch fails the policy pair; reverting the placeholder exemption fails the deck-feature pair; reverting the dispatch refactor fails the triggers.rs pair.

Scope boundary, deliberately. The two other call sites of this sequence are left alone: casting.rs has a CR 602.2b/601.2b X-deferral carve-out (it must not pre-filter modes when target legality depends on an unchosen {X}), and the engine.rs pending-trigger re-entry has different failure handling (pop the uncommitted entry, Ok(None)). Folding either in would change behaviour outside this finding; happy to do it as a follow-up if you want the unification to go further.

CR 603.3c, CR 603.3d, CR 700.2, CR 107.3m, CR 115.1 all grep-verified against docs/MagicCompRules.txt.

[matthewevans]

Changes requested — the modal-choice authority is still bypassed after a live modal trigger pauses.

[MED] dispatch_pending_trigger_context invokes resolve_legal_modal_choice while the triggering event is live (crates/engine/src/game/triggers.rs:6542-6549), but the mandatory continuation through begin_pending_trigger_target_selection recomputes the same modal choice independently (crates/engine/src/game/engine.rs:8632-8655) after the event context is restored. This contradicts the new single-authority contract and makes event-context-dependent caps (for example EventContextSourceModesChosen, documented at engine.rs:8620-8625) resolve differently between preflight and the mode prompt. Route the continuation through the shared authority while retaining the event context, or persist the initial resolved choice, and add a paused production-pipeline regression proving the prompt’s cap/unavailable modes equal the live preflight.