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]

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.

[matthewevans]

Maintainer hold

At current head 97fc106298a7e7c611e632fa1d72dc220068f59f, this branch has a textual conflict with current main.

Maintainer conflict resolution has been prepared in an isolated worktree (local merge commit 822efadf3e18a02511ff6f6d6e3312f7ba1bb9ce), preserving the contributor branch and current-main changes. The update could not be published because the authorized contributor-fork push credentials are unavailable; the PR head has not changed.

Next step: a maintainer with push access to minion1227/phase:minion_cycling_payoff_axis should push or recreate the update against current main. Once GitHub shows the new head and required checks run, implementation review can resume.

[minion1227]

Round 10 — the announcement is now carried to its prompt, not re-derived

Head 08d0e9db7 (round-10 fix 97fc10629 + a merge of upstream/main).

The finding was right, and it was worse than a duplicated derivation

Round 9 collapsed the continuation's six inline steps into a single resolve_legal_modal_choice call and I treated that as done. It wasn't. Deduplicating the code is not deduplicating the derivation. dispatch_pending_trigger_context asked the authority when it put the ability on the stack; begin_pending_trigger_target_selection asked the same authority again in a later re-entry. One function, two moments, two answers.

CR 700.2b: "The controller of a modal triggered ability chooses the mode(s) as part of putting that ability on the stack. If one of the modes would be illegal (due to an inability to choose legal targets, for example), that mode can't be chosen." CR 603.3c says the same for the announcement. Both bind legality to the earlier moment — and the board can move in between. The target path a few lines below already documents exactly that window: "an effect earlier in the SAME simultaneous cascade removed the only legal target."

So the prompt was answering a later game state than the announcement it was meant to present.

It is a player-visible defect, proven by reverting the fix

I bypassed the carried announcement and re-ran the new regression. It fails with got None: the re-derivation finds no legal mode, drops the trigger, and never raises a prompt at all — the controller silently loses a mode choice CR 700.2b already granted them. The ability may still leave the stack afterwards, but by CR 603.3d once a chosen mode has no legal target, which is the rule that actually governs that removal.

The fix — persist the announcement (the second option you offered)

• PendingTrigger::announced_modal_choice carries the AnnouncedModalChoice produced at the push site, set only after resolve_legal_modal_choice confirms a legal choice exists.
• begin_pending_trigger_target_selection surfaces that announcement. It falls back to the same single authority only for a trigger parked without one, and it never re-implements the mode-choice sequence.
• resolve_legal_modal_choice now returns the named AnnouncedModalChoice rather than a loose (ModalChoice, Vec<usize>), so the value crossing the pause has one type and one producer.

The field is boxed: the types::game_state_size stack guard fired, and that module's own instruction is to box a large, rarely-populated field rather than widen a ceiling. It embeds a whole ModalChoice and is populated only on the modal dispatch path. The other 36 construction sites are mechanical announced_modal_choice: None.

Regressions

• trigger_modal_choice_single_authority.rs — the paused production-pipeline test you asked for: drives the real cast → dispatch → pause → prompt path and asserts the prompt's cap and unavailable-mode set equal the live authority, for both halves.
• mode_prompt_surfaces_the_announcement_not_a_later_re_derivation — the discriminating one. The sole legal target leaves the battlefield between announcement and prompt; the test pins that the announced choice is still offered. Verified red on revert (got None), so it fails if this fix is undone.

One correction worth stating plainly: my first version of that second test was vacuous. It injected the drift by setting GameObject::zone, but battlefield membership lives in a separate zone list, so the creature stayed targetable and the test passed with the fix bypassed. It now uses zones::move_to_zone and asserts the removal actually took effect before proceeding.

Verification

• Round-10 tree: cargo fmt; clippy -D warnings across engine + phase-ai + server-core --all-targets --features proptest; engine 21,876 tests green; phase-ai + server-core 1,968 green; cargo ai-gate byte-identical to the round-9 parent run with the same card database (same suite percentages, same flip counts, same p-values), so this change is gate-neutral and the two standing WARNs are pre-existing.
• Merge commit: compiles across all three crates --all-targets and clippy -D warnings clean. I have not completed a full local test re-run on the merged tree — local builds kept getting killed by my environment, so I am relying on CI for that rather than claiming a green run I did not observe.

Merge resolution (#6688 landed underneath this branch)

7d6334185 (draw-matters axis) merged and collided in five files:

• phase-ai config.rs / features/mod.rs / policies/registry.rs — both axes kept, ordered by the files' append-chronological convention.
• ability_utils.rs — kept this branch's resolve_legal_modal_choice delegation over main's open-coded filter_modes_by_target_legality + modal_choice_with_target_assignment_limit. The authority runs those exact steps internally, so the delegation subsumes main's behaviour; main only carries the older shape because feat(phase-ai): add draw-matters deck-feature axis + DrawPayoffPolicy #6688 branched before round 9.
• triggers.rs — kept the gated Effect::unimplemented(..) constructor per CLAUDE.md, plus this branch's field and regression.

CR 603.3c, CR 603.3d, CR 700.2b verified against docs/MagicCompRules.txt.

[matthewevans]

Maintainer hold on current head 08d0e9db7819af366be889bd74e0bd8f1368ed7a: the implementation review is clean and the earlier modal-announcement findings are addressed.

The Paired-seed AI gate and Decision-cost performance gate are still running. Please leave this head unchanged while they finish; once both succeed, I will live-recheck the checks and mergeability, then approve and enqueue if the remaining maintainer gates are clear.

[matthewevans]

Additional current-head hold for 08d0e9db7819af366be889bd74e0bd8f1368ed7a: GitHub currently reports mergeStateStatus: DIRTY, so this branch also needs reconciliation with current main before it can enter the merge queue.

After the paired-seed AI and Decision-cost performance gates settle, the maintainer will reconcile/update the branch as needed and recheck the resulting head, checks, and mergeability before any approval or enqueue.

[minion1227]

Conflict resolved on new head 443ce0d63

#6718 (derive targeting intent from the announcing effect) landed at 04:30, after this branch's previous merge, and made the branch dirty. Resolved by merging current main; GitHub now reports mergeable: true.

The conflict was one import list, not a logic collision

The single conflicted hunk is the types::ability import in crates/engine/src/game/ability_utils.rs: this branch adds AnnouncedModalChoice, #6718 adds EffectKind and PtValue. Resolved as the union.

The two changes touch the same pipeline but at different stages, so neither bypasses the other:

• resolve_legal_modal_choice (this branch) decides which modes are legal when the ability is put on the stack — CR 700.2b, CR 603.3c.
• #6718 stamps EffectKind onto target slots inside collect_target_slots, which runs after a mode has been chosen.

A modal trigger therefore still gets its announced choice carried to the prompt, and the slots built from the chosen mode still get #6718's effect-kind label. I checked the continuation path at engine.rs:8661-8697 specifically: it consumes the carried AnnouncedModalChoice and emits the mode prompt; it does not construct TargetSelectionSlot values itself, so it cannot skip the new stamping.

Verified on the merged tree: cargo check -p engine -p phase-ai --all-targets clean, and both round-10 regressions still pass — prompt_cap_equals_modal_choice_authority, prompt_unavailable_modes_equal_modal_choice_authority.

The Paired-seed AI gate on the previous head was a 60-minute wall-clock timeout, not a behavioral regression

Run 30328391749 shows conclusion: cancelled, and the job ran 60m15s against ai-gate.yml:32's timeout-minutes: 60. Every step succeeded except Run quick AI gate, which was cancelled mid-matchup — there is no failing assertion or baseline delta in the log.

Measured against the last green run on this branch (30270303506, head 6e616c794), per-matchup wall clock from gate start:

matchup	last green run	timed-out run
red-mirror	25.7 min	22.4 min
affinity-mirror	38.0 min	27.0 min
enchantress-mirror	50.0 min (job total ~59m)	not finished at 50 min

Matchups 1 and 2 were faster on the timed-out head. enchantress-mirror is the long pole in both runs, and the last green run cleared the 60-minute cap with roughly one minute of headroom. That is the cliff this run went over.

Two independent signals that this is not decision-cost regression from this branch:

1. The Decision-cost perf gate passed on the same head (49m42s) — that is the gate that exists to catch this class of regression.
2. The gate passed on this branch four times on 7/27 (06:59, 07:49, 08:59, 13:26).

The seed 10592737 aborted: AI panic during suite game line in affinity-mirror is the known pre-existing panic that the suite catches; affinity-mirror still reported PASS 3/10, matching the green run's 3/10 exactly.

Checks are re-running on 443ce0d63. If the paired-seed gate lands near the cap again, that is a pre-existing wall-clock fragility in enchantress-mirror rather than something this branch introduces — happy to open a separate issue for it if you'd like.

[matthewevans]

Changes requested — current head 443ce0d63c32a53c64cd5e2ea44f06659e7330b2

🟡 Blocker [MED] The modal-continuation regression is not discriminating through the production pipeline.

The only test that changes legality between announcement and prompt hand-builds GameState, calls internal dispatch_two_mode_creature_target_modal, mutates via move_to_zone, then directly invokes begin_pending_trigger_target_selection (crates/engine/src/game/triggers.rs:16740-16783). The integration tests reach a prompt through GameScenario/GameAction::CastSpell, but only compare it with a fresh resolve_legal_modal_choice result and never change state after announcement (crates/engine/tests/integration/trigger_modal_choice_single_authority.rs:156-197, 204-262). They would therefore stay green if the continuation dropped announced_modal_choice and recomputed legality.

Please add a production-path GameScenario/GameAction continuation test where a prior simultaneous trigger/effect removes the only target after the modal trigger is announced but before AbilityModeChoice, and assert the prompt retains its announced availability/cap. That test must fail for the old recomputation/bypass behavior.

The implementation itself appears to persist/consume the field at crates/engine/src/game/triggers.rs:6551-6585 and crates/engine/src/game/engine.rs:8645-8697; this request is solely for discriminating runtime coverage. CI has Rust lint/tests and paired-seed/perf still in progress, and the parse artifact reports no parse changes.

[minion1227]

Closing this PR.

The merge conflict with #6718 is resolved on the current head and all code CI is green, but I'm not going to be able to land the discriminating modal-continuation regression the last review asks for, so I'd rather close this than leave it sitting open.

For anyone picking this up later: I confirmed the round-11 finding is correct by experiment. With the carried announcement bypassed (forcing the old recomputation path), both integration tests in trigger_modal_choice_single_authority.rs still pass — they compare the prompt against a freshly computed resolve_legal_modal_choice at unchanged state, and the continuation restores the same trigger-event window the announcement used, so only genuine board drift can separate the two. The unit test at triggers.rs:16740 does go red under that bypass.

What I could not find was a reachable production window for that drift: in every call site I traced, begin_pending_trigger_target_selection runs synchronously in the same engine step as dispatch_pending_trigger_context (triggers.rs:5895-5907, engine_priority.rs:191-196, engine_replacement.rs:2029-2041), and the one real pause — the optional-modal OptionalEffectChoice gate — admits no board-changing action. The simultaneous-cascade case the engine documents at engine.rs:8818-8820 is on the target path, which does choose targets in a later step. That may well mean I was looking in the wrong place rather than that the window doesn't exist.

Thanks for the review depth on this one.