feat(phase-ai): add draw-matters deck-feature axis + DrawPayoffPolicy#6688
feat(phase-ai): add draw-matters deck-feature axis + DrawPayoffPolicy#6688minion1227 wants to merge 16 commits into
Conversation
CR 121.1: with a "whenever you draw a card" engine on the battlefield (The Locust God, Psychosis Crawler, Niv-Mizzet), every extra draw is a repeatable value trigger. card_advantage values having cards but not triggering the engine, so the AI won't lean into extra draws when it has a payoff out. New DrawMattersFeature axis (structural, no card names): - source_count: card-draw enablers (Effect::Draw scoped to Controller). - payoff_count: permanents with a controller-scoped, non-self TriggerMode::Drawn engine trigger. - commitment: geometric mean over (source, payoff) — both mandatory (draw with no engine is just card advantage; an engine with no extra draw only fires on the natural draw for turn). New DrawPayoffPolicy: on a CastSpell/ActivateAbility that draws the controller a card (its own CastFacts primary/ETB effects, or the activated ability), if the AI controls a live draw engine (structural match over trigger_definitions), score a positive per-engine bonus. Composes with card_advantage. draw_payoff_bonus registered UNTUNED. Tests: 11 feature + 6 policy (incl a registry-routed regression and a name-only-impostor neutral case) + full 1582-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Note Reviews pausedIt 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 Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds structural deck detection for controller-drawing sources and “whenever you draw” engines, aggregates the feature, and introduces a registered policy that rewards qualifying draw actions. Configuration defaults, trigger preflight support, and feature and policy tests are included. ChangesDraw Matters AI Policy
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/phase-ai/src/policies/draw_payoff.rs (1)
108-121: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPrefer an exhaustive
matchover the_ => falsefallback.Coding guidelines call for exhaustive
matchover wildcard defaults so newGameActionvariants surface at compile time rather than silently falling through tofalsehere.As per coding guidelines, "Use idiomatic Rust: prefer enums over stringly typed data, exhaustive
matchover wildcard defaults" and path instructions forcrates/**/*.rs: "wildcard_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants."🤖 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/policies/draw_payoff.rs` around lines 108 - 121, The candidate_draws_controller match currently hides newly added GameAction variants behind a wildcard false arm. Replace `_ => false` with explicit arms for every remaining GameAction variant, preserving false behavior for non-CastSpell and non-ActivateAbility actions while making the match exhaustive.Sources: Coding guidelines, Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/policies/tests/draw_payoff.rs`:
- Around line 162-286: Add verdict tests for DrawPayoffPolicy’s ActivateAbility
path using an ability activation candidate, including a modal ability whose
selected mode draws and another mode does not. Assert the drawing mode receives
the engine-active reward while the non-drawing mode remains neutral, exercising
candidate_draws_controller’s ActivateAbility branch and selected-mode scoping.
---
Nitpick comments:
In `@crates/phase-ai/src/policies/draw_payoff.rs`:
- Around line 108-121: The candidate_draws_controller match currently hides
newly added GameAction variants behind a wildcard false arm. Replace `_ =>
false` with explicit arms for every remaining GameAction variant, preserving
false behavior for non-CastSpell and non-ActivateAbility actions while making
the match exhaustive.
🪄 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: 9d9b5814-7a9a-4738-862f-3addf971a63a
📒 Files selected for processing (10)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/draw_matters.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/draw_matters.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/policies/draw_payoff.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/draw_payoff.rscrates/phase-ai/src/policies/tests/mod.rs
Parity with the phase-rs#6683 review fix: the draw-payoff scan matched an engine only by its structural trigger shape, so a rate-limited "whenever you draw ... only once each turn" engine would still earn a bonus on a second draw even though its trigger can't fire again. The battlefield scan now pairs the structural classifier with live firing eligibility per trigger entry — a OncePerTurn / OncePerGame engine that has already fired (per the engine's authoritative triggers_fired_this_turn / triggers_fired_this_game ledgers, keyed via GameObject::trigger_definition_ref) no longer counts. trigger_is_draw_payoff is exposed as is_draw_payoff_trigger so the per-entry check can pair shape with eligibility. Regressions: a once-per-turn engine already fired this turn scores neutral; the same engine unfired still rewards. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Re: the CodeRabbit nitpick on
Happy to expand it if you'd prefer the fully-exhaustive form despite the precedent — just say the word. |
There was a problem hiding this comment.
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/tests/draw_payoff.rs`:
- Around line 260-316: Add symmetric OncePerGame coverage near
once_per_turn_engine: create an once_per_game_engine helper using
TriggerConstraint::OncePerGame, then add a test that inserts its trigger key
into triggers_fired_this_game and verifies DrawPayoffPolicy returns
draw_payoff_no_engine with zero delta. Also add an unfired OncePerGame control
test confirming the engine remains rewarding.
🪄 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: 8d5d0e3f-0323-4000-8a65-d69020ad3275
📒 Files selected for processing (3)
crates/phase-ai/src/features/draw_matters.rscrates/phase-ai/src/policies/draw_payoff.rscrates/phase-ai/src/policies/tests/draw_payoff.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/policies/draw_payoff.rs
- crates/phase-ai/src/features/draw_matters.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — candidate timing and trigger eligibility are both overstated in the live draw-payoff score.
🟡 Blockers
[MED] A modal/else draw is credited before its draw mode is selected. crates/phase-ai/src/features/draw_matters.rs:98-109 scans with AbilityScope::Potential, and draw_payoff.rs:110-121 uses that classifier directly for the live candidate. Potential is appropriate for deck classification, but not an action before SelectModes: a non-draw mode can receive a draw payoff. Split runtime scanning to unconditional effects plus the definitions selected by SelectModes, and cover paired draw/non-draw modal choices.
[MED] trigger_still_fireable treats every constraint except OncePerTurn/Game as live. draw_payoff.rs:126-140 therefore rewards engines that cannot trigger under constraints such as OnlyDuringYourTurn, main-phase, nth-draw, or max-times. The engine's trigger path evaluates the full constraint authority (crates/engine/src/game/triggers.rs:8005+); use the authoritative occurrence/fireability evaluation rather than a partial policy mirror, with regressions for those unavailable cases.
…omplete trigger eligibility Round-1 review (phase-rs#6688): two ways the live draw-payoff score was overstated. [MED] A modal/else draw was credited before its mode was selected. `is_draw_source_parts` scanned with `AbilityScope::Potential`, so a modal "choose one — deal damage; OR draw a card" got a draw payoff even when the non-draw mode would be chosen. The predicate now takes the scope: deck-time detection keeps `Potential` (a modal draw still marks the card), but the live candidate scan uses `Unconditional` (CR 700.2) so only a draw that always happens is credited pre-mode-selection. [MED] `trigger_still_fireable` treated every constraint except OncePerTurn/Game as live, rewarding engines that cannot fire. It is now exhaustive over `TriggerConstraint` (no wildcard): OncePerTurn/Game via the fired-trigger ledgers; OnlyDuringYourTurn / OnlyDuringOpponentsTurn / OnlyDuringYourMainPhase via `active_player` + `phase`; and the event/count-dependent constraints (MaxTimesPerTurn, NthSpell/NthDraw, OncePerOpponentPerTurn, AtClassLevel, EventSourceControlledBy) conservatively NOT confirmed, so the payoff is never over-credited. Tests: modal-draw-not-credited (+ deck-time still counts it), OncePerGame fired/unfired, OnlyDuringYourTurn off-turn/on-turn; full 1590-test phase-ai lib suite pass; clippy -p phase-ai --lib --tests -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both blockers fixed at head [MED] Modal/else draw credited before mode selection
Tests: [MED]
|
| Constraint | Check |
|---|---|
OncePerTurn / OncePerGame |
triggers_fired_this_turn / _game ledger, keyed via trigger_definition_ref |
OnlyDuringYourTurn / OnlyDuringOpponentsTurn |
state.active_player vs the engine's controller |
OnlyDuringYourMainPhase |
your turn and state.phase ∈ {PreCombatMain, PostCombatMain} |
MaxTimesPerTurn, NthSpellThisTurn, NthDrawThisTurn, OncePerOpponentPerTurn, AtClassLevel, EventSourceControlledBy |
conservatively not confirmed at decision time (depends on the triggering event or a per-turn count) → no bonus, so the payoff is never over-credited |
Tests: once_per_game_engine_already_fired_is_neutral / _unfired_rewards, only_during_your_turn_engine_is_neutral_off_turn / _rewards_on_your_turn.
The exhaustive match also resolves the earlier CodeRabbit nitpick about wildcard arms here.
Full 1590-test phase-ai suite pass; clippy -p phase-ai --lib --tests -D warnings clean. (The same eligibility-completeness fix is going onto #6683's cycling policy for parity.)
…e constraints) Parity with the phase-rs#6688 review: trigger_still_fireable is now exhaustive over TriggerConstraint (no wildcard). OncePerTurn/Game via the fired-trigger ledgers; OnlyDuringYourTurn / OnlyDuringOpponentsTurn / OnlyDuringYourMainPhase via active_player + phase; and the event/count-dependent constraints (MaxTimesPerTurn, NthSpell/NthDraw, OncePerOpponentPerTurn, AtClassLevel, EventSourceControlledBy) conservatively NOT confirmed so the payoff is never over-credited. Regressions: once-per-game engine already fired -> neutral; OnlyDuringYourTurn engine on the opponent's turn -> neutral. cargo test -p phase-ai --lib cycling_payoff — 13 pass; clippy -p phase-ai --lib --tests -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
There was a problem hiding this comment.
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/tests/draw_payoff.rs`:
- Around line 415-448: Extend the draw payoff timing-constraint tests around
trigger_still_fireable to cover TriggerConstraint::OnlyDuringOpponentsTurn and
TriggerConstraint::OnlyDuringYourMainPhase: assert rewards on each constraint’s
valid turn/phase, zero payoff on invalid turns, and zero payoff for a non-main
phase. Reuse the existing state, engine_with_constraint, draw_spell, and verdict
setup while preserving the current OnlyDuringYourTurn cases.
🪄 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: 647fb847-c745-4c3d-9ed9-a38482fc59fd
📒 Files selected for processing (4)
crates/phase-ai/src/features/draw_matters.rscrates/phase-ai/src/features/tests/draw_matters.rscrates/phase-ai/src/policies/draw_payoff.rscrates/phase-ai/src/policies/tests/draw_payoff.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/phase-ai/src/policies/draw_payoff.rs
- crates/phase-ai/src/features/draw_matters.rs
- crates/phase-ai/src/features/tests/draw_matters.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed at head 8af6c31658cf42b10e06cd03ebefdcd894e1afc4.
[MED] Live draw-payoff detection treats a structurally matching trigger as a usable payoff without evaluating its trigger condition or resolution targets. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:83-86,146-172; the production trigger pipeline evaluates TriggerDefinition::condition with the event and source context and rejects unresolvable target sets (crates/engine/src/game/triggers.rs:435-445,1447-1473,4511-4539). Why it matters: the policy awards a draw bonus for engines that will produce no value, such as Wizard Class at level 3 with no creature you control to target. Suggested fix: use an engine-owned, event/source-aware eligibility preflight for the prospective draw event, including execution target legality, or conservatively reject every engine whose eligibility cannot be established; add Wizard Class zero-target and legal-target runtime cases.
[MED] The cast candidate's immediate-ETB draw path ignores the ETB trigger's condition. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:115-123 classifies every qualifying ETB execute body, while crates/phase-ai/src/cast_facts.rs:330-336 admits it based only on its zone-change shape and execute. Why it matters: Latchkey Faerie is credited as a draw spell even when its prowl cost was not paid, because its condition is never preflighted. Suggested fix: route immediate-ETB draw detection through the same event/source-aware preflight (or reject conditional ETBs until that exists), with runtime no-prowl and prowl-paid Latchkey Faerie cases.
[MED] Deck draw-source detection omits ETB cantrips that the runtime policy intentionally rewards. Evidence: crates/phase-ai/src/features/draw_matters.rs:82-85 scans only face.abilities, whereas crates/phase-ai/src/policies/draw_payoff.rs:115-124 also recognizes immediate ETB trigger bodies. Why it matters: a deck relying on enter-the-battlefield draw sources is undercounted and can miss the activation floor even though those same casts receive the live bonus. Suggested fix: include structurally qualifying ETB cantrip triggers in the deck feature's potential-source scan and add a detector regression that changes source_count/commitment.
[MED] The decision branches introduced by this policy still lack discriminating coverage. Evidence: crates/phase-ai/src/policies/tests/draw_payoff.rs:118-128,186-448 constructs only GameAction::CastSpell candidates and tests only OnlyDuringYourTurn; DrawPayoffPolicy::decision_kinds() also registers ActivateAbility, and trigger_still_fireable separately handles OnlyDuringOpponentsTurn and OnlyDuringYourMainPhase (crates/phase-ai/src/policies/draw_payoff.rs:49-51,125-130,158-164). Why it matters: regressions in activated-draw selection and the remaining timing paths can ship while every current assertion stays green. Suggested fix: add verdict/registry tests that reach ActivateAbility (including draw/non-draw selected branches) and positive/negative runtime cases for opponent-turn and both main-phase timing constraints.
…d 2) Addresses matthewevans' four [MED] blockers on phase-rs#6688 by moving live "whenever you draw" payoff eligibility into a single engine authority that covers the COMPLETE trigger condition, not just selected constraint variants. Engine (single authority): - `triggers::hypothetical_trigger_fireable(state, source, entry)` — the one place a policy asks "is this on-battlefield payoff live?". It reuses the live pipeline's `check_trigger_constraint_with_ref` (now `event: Option<&_>`; Some = real eval, None = hypothetical → event-dependent constraints report NOT-satisfied rather than guess), rejects an intervening-if `condition` (CR 603.4, conservative), and preflights execution target legality via `ability_utils::execute_targets_satisfiable` (CR 603.3d — a mandatory-target trigger with no legal target produces no effect). - Deleted the policy-local partial `trigger_still_fireable` reimplementation. Policy (draw_payoff.rs): - Live engine scan now `is_draw_payoff_trigger && hypothetical_trigger_fireable`. - Immediate-ETB draw path skips conditional ETBs (`condition.is_none()`), so a Latchkey Faerie prowl cantrip is not credited a draw until it will fire. Feature (draw_matters.rs): - `detect()` now also counts self-ETB draw cantrips (Elvish Visionary) as deck draw sources via `is_etb_draw_source`, matching what the live policy rewards. Tests (external, no cfg(test) in source): - Wizard-Class zero-target neutral / legal-target reward (target legality). - Latchkey conditional-ETB not credited / unconditional-ETB credited. - ActivateAbility draw reward / non-draw neutral. - OnlyDuringOpponentsTurn positive+negative; OnlyDuringYourMainPhase both main phases positive + off-phase (upkeep) negative; MaxTimesPerTurn below/at cap. - Feature: ETB-cantrip source-count regression + opponent-draw control. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — all four [MED] blockers addressed in 1. Live payoff detection ignored condition + resolution targets.
Tests: 2. Immediate-ETB draw path ignored the ETB condition (Latchkey). 3. Deck source detection omitted ETB cantrips. 4. Discriminating coverage.
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head db3fe363d886e033f762dd20494a6125c891c7aa.
[MED] The policy still grants a draw-payoff bonus when the candidate cannot actually draw. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:65-68,110-132 classifies any structural Effect::Draw { target: Controller } as a draw, but the authoritative delivery gate returns zero for CantDraw and exhausted PerTurnDrawLimit at crates/engine/src/game/effects/draw.rs:14-48. Why it matters: no CardDrawn event occurs, so a "whenever you draw" engine never triggers, yet this policy adds a positive score to the no-op draw action. Suggested fix: route live candidate qualification through an engine-owned draw preflight that resolves the requested count and applies the same allowed-draw gate; add discriminating CantDraw and exhausted-limit cases that return draw_payoff_na.
[MED] The new generic trigger preflight reports multi/complex mandatory target executions as live even when no legal target assignment exists. Evidence: crates/engine/src/game/ability_utils.rs:1290-1312 returns None unless there is exactly one simple target slot, and execute_targets_satisfiable converts that None to true at :1320-1338; hypothetical_trigger_fireable relies on it at crates/engine/src/game/triggers.rs:8188-8192. Why it matters: a draw-payoff trigger with two mandatory targets (or another unsupported target shape) is credited despite being removed under CR 603.3d when its required choices are unavailable. Suggested fix: make the preflight distinguish confirmed-legal from unknown and have this scoring policy award the bonus only for confirmed legality (or use the existing full legal-assignment authority behind an appropriate cheap guard); cover a multi-target zero-legal-target engine.
There was a problem hiding this comment.
🧹 Nitpick comments (2)
crates/phase-ai/src/policies/tests/draw_payoff.rs (1)
204-277: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winNo test exercises the
MAX_REWARDED_ENGINEScap.Every rewarded-verdict test here (single-engine, once-per-turn, targeted, activated-ability, etc.) has exactly one live engine on the battlefield.
DrawPayoffPolicy::verdictcaps the reward atengines.min(MAX_REWARDED_ENGINES)(crates/phase-ai/src/policies/draw_payoff.rsline 96), but nothing here places more engines than the cap to confirm the score saturates rather than scaling linearly, or that the reportedenginesfact still reflects the true (uncapped) count.♻️ Suggested addition
#[test] fn engine_count_above_cap_saturates_reward() { let config = AiConfig::default(); let mut st = state(); // Place more engines than MAX_REWARDED_ENGINES. for _ in 0..(MAX_REWARDED_ENGINES + 1) { engine_on_battlefield(&mut st); } let (oid, cid) = draw_spell(&mut st); let context = context(&config, session(0.9)); let candidate = cast(oid, cid); let decision = priority_decision(&candidate); let (delta, reason) = score_of(DrawPayoffPolicy.verdict(&ctx(&st, &candidate, &decision, &context, &config))); assert_eq!(reason.kind, "draw_payoff_engine_active"); let capped = config.policy_penalties.draw_payoff_bonus * MAX_REWARDED_ENGINES as f64; assert!((delta - capped).abs() < f64::EPSILON, "reward should saturate at the cap"); }🤖 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/policies/tests/draw_payoff.rs` around lines 204 - 277, Add a test near the existing DrawPayoffPolicy tests that places MAX_REWARDED_ENGINES + 1 live engines using engine_on_battlefield, then casts a draw spell and evaluates the verdict. Assert the engine-active reason, verify the reward equals the configured draw-payoff bonus multiplied by MAX_REWARDED_ENGINES rather than the true engine count, and confirm the reported engines fact remains uncapped if exposed by the verdict reason.crates/engine/src/game/triggers.rs (1)
8155-8195: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
hypothetical_trigger_fireablefails closed onAtClassLeveldue to missingsource_context, not genuine uncertainty.
check_trigger_constraint_with_refis called withsource_context: None(line 8182), but theTriggerConstraint::AtClassLevelarm (line ~8140) readssource_context.and_then(|source| source.source_read(state).class_level()). Sincesource_contextisNonehere, any Class-level-gated trigger will always evaluate as not-fireable — even thoughsource: &GameObjectis directly available to build a real context viatrigger_source_context_for_latch. Unlike the event-dependent constraints (OncePerOpponentPerTurn,EventSourceControlledBy,NthSpellThisTurn,NthDrawThisTurn),AtClassLevelhas nothing to do with the (unknown) triggering event — the source's level is fully knowable at hypothetical-evaluation time, so failing it closed here is inconsistent with the function's own documented invariant ("event-dependent constraints... conservatively report NOT satisfied").🐛 Proposed fix
let definition_ref = source.trigger_definition_ref(entry); + let source_context = trigger_source_context_for_latch(state, source); // CR 603.2-603.4: the trigger's own constraint, in hypothetical (no-event) // mode — the shared authority the live pipeline also uses. if !check_trigger_constraint_with_ref( state, def, Some(&definition_ref), - None, + Some(&source_context), source.controller, None, ) { return false; }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/triggers.rs` around lines 8155 - 8195, Update hypothetical_trigger_fireable to pass a real source context, created from the available source via trigger_source_context_for_latch, into check_trigger_constraint_with_ref instead of None. Preserve conservative failure for genuinely event-dependent constraints while allowing AtClassLevel to evaluate the source’s known class level.
🤖 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.
Nitpick comments:
In `@crates/engine/src/game/triggers.rs`:
- Around line 8155-8195: Update hypothetical_trigger_fireable to pass a real
source context, created from the available source via
trigger_source_context_for_latch, into check_trigger_constraint_with_ref instead
of None. Preserve conservative failure for genuinely event-dependent constraints
while allowing AtClassLevel to evaluate the source’s known class level.
In `@crates/phase-ai/src/policies/tests/draw_payoff.rs`:
- Around line 204-277: Add a test near the existing DrawPayoffPolicy tests that
places MAX_REWARDED_ENGINES + 1 live engines using engine_on_battlefield, then
casts a draw spell and evaluates the verdict. Assert the engine-active reason,
verify the reward equals the configured draw-payoff bonus multiplied by
MAX_REWARDED_ENGINES rather than the true engine count, and confirm the reported
engines fact remains uncapped if exposed by the verdict reason.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f04e16bb-8d59-47a8-aeb6-b7e05e8d9468
📒 Files selected for processing (6)
crates/engine/src/game/ability_utils.rscrates/engine/src/game/triggers.rscrates/phase-ai/src/features/draw_matters.rscrates/phase-ai/src/features/tests/draw_matters.rscrates/phase-ai/src/policies/draw_payoff.rscrates/phase-ai/src/policies/tests/draw_payoff.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/features/tests/draw_matters.rs
- crates/phase-ai/src/features/draw_matters.rs
Parse changes introduced by this PR✓ No card-parse changes detected. |
…legality (round 3) Addresses matthewevans' two round-3 [MED] blockers on phase-rs#6688. 1. Draw-delivery gate (CR 120.3 / CR 101.2). A structural `Effect::Draw { target: Controller }` produces no `CardDrawn` event — and fires no "whenever you draw" engine — when the controller cannot actually draw: under a `CantDraw` static, or with a `PerTurnDrawLimit` already exhausted. `candidate_draws_controller` now gates on a new engine authority, `effects::draw::can_draw_at_least_one`, which reuses the same `allowed_draw_count` gate the delivery path runs — so the bonus is never added to a no-op draw. New coverage: `CantDraw` → `draw_payoff_na`; exhausted per-turn limit → `draw_payoff_na`; limit with headroom → rewarded. 2. Multi-target legality (CR 603.3d). `execute_targets_satisfiable` previously treated the cheap single-slot check's `None` (multi-target or other complex shape) as satisfiable, crediting a mandatory multi-target engine with no legal target assignment. It now falls through to `build_target_slots` + `has_legal_target_assignment_for_ability` (the same full solver production target selection uses), so a multi-target execute with no legal assignment is correctly reported not-live. New coverage: a two-target "exchange control of two permanents" engine on an empty board → `draw_payoff_no_engine`; with two exchangeable permanents → rewarded. `cargo fmt`, `clippy -p engine`/`-p phase-ai` (lib+tests) clean; full phase-ai lib suite green (1609 passed). Engine changes ride behind existing live targeting/draw tests. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both round-3 [MED] blockers fixed in 1. Draw-delivery gate (CR 120.3 / CR 101.2). You're right that a structural 2. Multi-target legality (CR 603.3d). Correct —
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head a129a921620d117c1c0b12e9a7cbb252b914650a.
[MED] Activated-draw coverage bypasses PolicyRegistry routing. Evidence: DrawPayoffPolicy opts into both CastSpell and ActivateAbility at crates/phase-ai/src/policies/draw_payoff.rs:47-49, and priority-time GameAction::ActivateAbility is classified as DecisionKind::ActivateAbility at crates/phase-ai/src/decision_kind.rs:57-67 before registry dispatch at crates/phase-ai/src/policies/registry.rs:433-437. Yet the activated-draw assertion calls DrawPayoffPolicy.verdict directly at crates/phase-ai/src/policies/tests/draw_payoff.rs:800-817; the only registry-routed regression covers a cast at :1263-1278. Why it matters: a regression in the action classifier, decision-kind registration, or by_kind construction can leave activated draw actions unscored while every activated-draw test stays green. Suggested fix: add a priority-context PolicyRegistry::verdicts regression for a rewarded ActivateAbility draw (and, preferably, its non-draw control).
[MED] can_draw_at_least_one bypasses mandatory individual draw replacements. Evidence: crates/engine/src/game/effects/draw.rs:23-25 only checks the static allowance and whether a library card is selectable; it never proposes the draw to the replacement pipeline. Live delivery instead calls draw_through_replacement_with_applied at draw.rs:285-294, which invokes replacement::replace_event at :381-402; a mandatory QuantityModification::Prevent draw replacement returns ApplyResult::Prevented at crates/engine/src/game/replacement.rs:2772-2785. GameEvent::CardDrawn is emitted only after the unreplaced settled delivery loop at draw.rs:508-535. Why it matters: the policy awards a “whenever you draw” payoff to a candidate whose individual draw is obligatorily replaced and therefore cannot emit the triggering event. CR 614.1/614.6 define replacement effects and say a replaced event never happens; CR 614.11 says draw replacements apply even when no card can be drawn. Suggested fix: make the policy’s deliverability preflight replacement-aware (conservatively reject a mandatory individual replacement unless its resulting draw event can be established), and add a discriminating mandatory-prevented-draw policy case.
Current-head scope: prior empty-library, execute-target-constraint, unsupported-execute, and CR-citation findings are resolved. This review requests changes only for the two remaining behavior/test gaps above.
…ivated-draw coverage (round 8) Addresses matthewevans' two round-8 [MED] blockers on phase-rs#6688. 1. Replacement-aware deliverability (CR 614.1 / CR 614.6 / CR 614.11). `can_draw_at_least_one` checked draw restrictions and library delivery but not the replacement pipeline, so a candidate whose individual draw is obligatorily replaced (a mandatory `QuantityModification::Prevent` draw replacement — Living Conundrum's "skip that draw instead") was still credited even though the replaced event never happens and no `CardDrawn` fires. It now also scans `active_replacements` and conservatively blocks delivery when a mandatory prevent-draw replacement (`ReplacementEvent::Draw`/`DrawCards` + `QuantityModification::Prevent`) is active. New case: `mandatory_prevent_draw_replacement_is_a_no_op` → `draw_payoff_na`. 2. Registry-routed activated-draw coverage. The activated-draw assertions called `verdict` directly; only a cast was registry-routed. Added `registry_routes_activated_draw_to_the_policy` (a `GameAction::ActivateAbility` draw routed through `PolicyRegistry::verdicts` → rewarded) and `registry_activated_non_draw_is_not_rewarded` (control), so a regression in the action classifier / decision-kind registration / `by_kind` dispatch can't leave activated draws silently unscored. `cargo fmt`, `clippy -p engine`/`-p phase-ai` (lib+tests) clean; draw suite green (80 assertions). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both round-8 blockers fixed in 1. Replacement-aware deliverability (CR 614.6). Correct — 2. Registry-routed activated-draw coverage. Added
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Draw-preflight treats every functioning Prevent definition as a mandatory replacement of the candidate's draw. Evidence: crates/engine/src/game/effects/draw.rs:40-50 scans active_replacements only by event and QuantityModification::Prevent; the live candidate authority first runs the handler and condition gates (crates/engine/src/game/replacement.rs:6305-6355), then scopes ProposedEvent::Draw { player_id } against the replacement source (crates/engine/src/game/replacement.rs:6432-6453) and handles optional replacements (crates/engine/src/game/replacement.rs:6549-6560). DrawCards is also registered as a stub rather than a runtime draw handler (crates/engine/src/game/replacement.rs:4953). Why it matters: an unrelated/opponent-source, conditional, optional, or DrawCards-stub Prevent makes the AI classify an otherwise deliverable draw as a no-op and refuse the draw-payoff bonus. The added test does not discriminate: it puts the replacement on P1 (crates/phase-ai/src/policies/tests/draw_payoff.rs:1050-1064) while the candidate draw is P0, so the live authority would reject it by the default source-player scope at replacement.rs:6449; the scan still returns false. Suggested fix: model the candidate ProposedEvent::Draw and reuse the live applicability/choice authority (or expose a pure preflight from it), then add positive controls for an opponent-scoped replacement, a false conditional / optional replacement, and a DrawCards stub alongside the matching mandatory P0 prevent case.
…nt authority + modal-aware execute preflight (round 9) Draw preflight (review blocker). `can_draw_at_least_one` scanned `active_replacements` by event + `QuantityModification::Prevent` alone, so any functioning prevent-draw definition — opponent-scoped, conditional, optional, or a `DrawCards` stub — made the AI classify a deliverable draw as a no-op and withhold the payoff bonus. It now models the individual draw as the `ProposedEvent::Draw` the live path proposes (CR 121.2) and consults the live applicability authority, so source/player scope, the condition gate, the handler matcher, and optionality are all honored. - `replacement.rs`: parameterize `counter_placement_prevention_applies` into `mandatory_prevention_applies(state, candidates, events)` and expose a pure, read-only preflight `event_is_mandatorily_prevented(state, event)` that routes through `find_applicable_replacements` (CR 614.1 / CR 614.6). Only a MANDATORY Prevent suppresses the event — an optional one is an accept/decline choice. Modal execute preflight. A modal ability's root is an `Effect::Unimplemented` placeholder with its real effects in `mode_abilities`, so round 7's support check reported every modal payoff unsupported, and the target preflight never descended into the modes. - `ability_utils.rs`: `ability_definition_supported` treats an `Unimplemented` root as a gap only on a non-modal ability (CR 700.2); modes are still checked recursively. `execute_targets_satisfiable` mirrors the live trigger dispatch for modal executes by delegating to `filter_modes_by_target_legality` + `modal_choice_with_target_assignment_limit` (CR 603.3c / CR 603.3d). Tests. Five discriminating draw-replacement policy cases, all fail-on-revert against the old scan: the mandatory prevent now scoped to the DRAWING player, plus opponent-scoped / false-conditional / optional / `DrawCards`-stub positive controls. Registered engine dispatch tests assert the live `DroppedNoLegalMode` contract the modal preflight mirrors. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
The round-8 blocker is fixed in Draw-preflight over-blocking (CR 614.1a / CR 614.6). Correct, and the test was non-discriminating exactly as you describe — it put the replacement on P1 while the candidate draw was P0, so the live authority would have rejected it by the default source-player scope at Taking the suggested fix: model the candidate
Coverage — the four positive controls you asked for, plus the corrected mandatory case. All five are fail-on-revert against the old scan:
I confirmed the optional case is non-vacuous: Your Also in this commit — a self-found defect in my own round-7 code, not a review item. A modal ability carries an One annotation correction: I had drafted
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 3c868634621972bdea98cea56d9ee1bb6daaedee.
[MED] Draw-delivery preflight still credits mandatory non-draw substitutions. Evidence: crates/engine/src/game/effects/draw.rs:33-45 decides delivery solely via event_is_mandatorily_prevented; that helper only treats QuantityModification::Prevent definitions as blockers at crates/engine/src/game/replacement.rs:8700-8724,8740-8744. The live replacement path separately recognizes a mandatory non-Draw execute or runtime_execute (Words of Worship/Wilding, Jace/Abundance shapes), zeros the proposed draw count, and thus produces no CardDrawn at replacement.rs:7908-7939; draw.rs:441-555 emits CardDrawn only for delivered cards. Why it matters: with such a mandatory substitution already armed, DrawPayoffPolicy still awards the “whenever you draw” bonus even though the candidate’s draw event is replaced away. Suggested fix: expose/use a pure replacement outcome preflight that distinguishes a surviving positive Draw from a mandatory non-Draw substitute (while retaining the correct optional-decline and draw-count behavior), and add paired policy cases for an armed mandatory non-Draw substitution versus a normal/count-modified draw.
Recommendation: request changes — make the engine-owned draw-delivery preflight model the current live replacement outcome, then cover the mandatory non-draw substitution and surviving-draw controls through DrawPayoffPolicy.
… live pipeline (round 10) The draw-payoff preflight was a partial mirror of the live draw pipeline, and each review round surfaced one more leg the mirror failed to model. Fix the mirror rather than the leg: extract the substitution classification out of `apply_single_replacement` into `draw_is_substituted_away`, and have both the live pipeline and the read-only preflight call it — so the two cannot disagree about whether a draw survives, because there is only one of them. Replace `event_is_mandatorily_prevented` (prevention only, single caller) with `proposed_draw_survives_replacement`, routing every replacement suppression leg through the authority that owns it: - mandatory `QuantityModification::Prevent` (CR 614.6, Living Conundrum), via `mandatory_prevention_applies` - mandatory non-Draw substitute carried in `execute` (Chains of Mephistopheles, Jace Wielder of Mysteries) or `runtime_execute` (Words of Worship/Wilding), CR 614.11, via the shared `draw_is_substituted_away` - mandatory count modification resolving to zero (CR 614.11a), via `draw_replacement_count` — a leg no prior round had modeled Optional-decline and count behavior are preserved: a "you may" replacement is never assumed to apply, and an Alhammarret's Archive-style positive rescale is a surviving draw, not a substitution. Adds `draw_preflight_matches_live_pipeline`: for each shape it asks the preflight for a prediction, then drives the REAL draw through the pipeline and asserts prediction == observed delivery, with expected delivery pinned independently so a regression breaking both sides alike still fails. Covers the restriction, empty-library, prevent, both substitute slots and zero-count legs, plus unreplaced and count-modified surviving controls — preflight/pipeline drift now fails CI in either direction. Six paired `DrawPayoffPolicy` cases cover the same shapes at the policy seam. Consolidates the file's two Draw-definition producers into one parameterized helper and re-freezes the producer census accordingly.
|
Thanks — this one is on me, and the useful thing to say first is why it took ten rounds rather than just patching the tenth finding. The finding behind the findingsEvery round of this review has been the same defect wearing a different hat.
You named the root cause in round 1 — "use the authoritative evaluation rather than a partial policy mirror" — and I kept treating each finding as its own bug instead. Patching leg 10 in isolation would only have queued up leg 11, and leg 11 already existed: a mandatory count modification resolving to zero ( So this round fixes the mirror, not the leg. What changedThe classification is now shared with the pipeline instead of mirrored from it. The
Optional-decline and count behavior are preserved as you asked: a "you may" replacement is never assumed to apply, and a positive rescale ("…draw two cards instead") is a surviving draw. The old seam had exactly one caller, so it is removed rather than left orphaned. EvidenceSix paired policy cases in Beyond that, Local: engine clippy and phase-ai clippy clean under Note on the censusI consolidated the two Draw-definition producers in the AI test file into one parameterized Card textOracle text for every card named in the new comments and tests was verified against Scryfall, and two attributions in my own earlier draft were wrong until I checked: Words of Worship and Words of Wilding are activated one-shots, so they belong to the |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The hot-path policy performs the whole draw-delivery/replacement preflight before proving that the candidate draws. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:110-145 calls can_draw_at_least_one at line 118 before matching the candidate; that preflight scans active draw statics and invokes replacement::proposed_draw_survives_replacement, which calls find_applicable_replacements (crates/engine/src/game/effects/draw.rs:34-46, crates/engine/src/game/replacement.rs:8790-8811). Why it matters: verdict() runs for every CastSpell and ActivateAbility candidate at each search node, so every non-draw candidate now pays the battlefield/replacement scan despite the PR's stated card-local early-out. Suggested fix: first classify the candidate structurally from its cast facts/effective activated ability, return neutral for non-draw actions, and only then run can_draw_at_least_one for that reduced set.
[LOW] candidate_draws_controller still hides future GameAction variants behind a wildcard fallback. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:124-145 ends the known enum match with _ => false; this is the still-open CodeRabbit finding. Why it matters: a future action added to the existing routing surface can silently bypass this policy rather than forcing an intentional classification. Suggested fix: make the GameAction match exhaustive with explicit neutral arms (or route through an existing exhaustive decision/action classifier).
[LOW] The per-engine reward cap has no discriminating regression. Evidence: MAX_REWARDED_ENGINES caps the score at crates/phase-ai/src/policies/draw_payoff.rs:34,96, while crates/phase-ai/src/policies/tests/draw_payoff.rs has no scenario with more than one live engine. Why it matters: the policy's bounded-score contract can regress to unbounded scaling without a test failing. Suggested fix: add a registry- or policy-routed case with MAX_REWARDED_ENGINES + 1 live engines and assert the capped delta (and uncapped engines fact, if that observability contract is intentional).
Clippy `type_complexity` on the new equivalence test's `scenario` parameter. The tuple is `(card name, shaper)`; the shaper takes the permanent's ObjectId because a `runtime_execute` substitute binds its own source. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head fd61fd76aec345295e14fe313216218f065dea71.
[MED] The policy still performs draw-delivery/replacement preflight before it knows that the candidate draws. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:110-145 calls can_draw_at_least_one before the GameAction match; that helper scans active draw statics and calls proposed_draw_survives_replacement, which runs find_applicable_replacements (crates/engine/src/game/effects/draw.rs:34-46, crates/engine/src/game/replacement.rs:8790-8811). Why it matters: verdict() runs for every cast and activation candidate at every search node, so non-draw candidates still pay a battlefield/replacement scan despite the policy's stated card-local early-out. Suggested fix: first classify the cast facts/effective activated ability structurally, return neutral for non-draw candidates, and invoke can_draw_at_least_one only after that classifier confirms an unconditional controller draw.
[LOW] The bounded-score contract has no discriminating cap regression. Evidence: MAX_REWARDED_ENGINES limits the reward at crates/phase-ai/src/policies/draw_payoff.rs:38-40,96, while the policy suite has no case with more than one live engine (crates/phase-ai/src/policies/tests/draw_payoff.rs). Why it matters: changing or removing the cap can produce an unbounded hot-path score while every current reward test stays green. Suggested fix: exercise MAX_REWARDED_ENGINES + 1 live engines through the policy or registry, assert the capped delta, and preserve/assert the uncapped engine-count fact if that is intended observability.
[LOW] candidate_draws_controller still uses a wildcard GameAction arm. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:124-145 ends the known enum match with _ => false; this is the unresolved CodeRabbit review finding. Why it matters: a future action variant can silently bypass intentional classification instead of forcing it at compile time. Suggested fix: make this match exhaustive with explicit neutral arms, or delegate to an existing exhaustive action classifier.
The prior draw-delivery/replacement correctness findings are resolved at this head: the shared substitution classifier is now used by the preflight and live replacement pipeline, and the integration test drives both paths. The parse-diff sticky comment is current and reports no card-parse changes.
…ded-score regression (round 11) Addresses the three round-11 findings. [MED] hot-path ordering: `candidate_draws_controller` ran the delivery/ replacement preflight BEFORE proving the candidate draws, so every non-draw CastSpell/ActivateAbility candidate paid a battlefield-static scan and `find_applicable_replacements` at every search node — the opposite of the module doc's stated card-local early-out. Split into `candidate_draws_ structurally` (card-local, reads only the candidate AST) and `draw_is_deliverable` (the expensive, candidate-independent engine authority), evaluated in that order. Behavior is unchanged; only non-draw candidates get cheaper, and the module doc is now true. [LOW] exhaustive `GameAction` match: the wildcard `_ => false` let a future action silently bypass the policy. Now enumerates all 127 variants, so a new one fails to compile here and forces an intentional classification. Cast-shaped siblings (madness, miracle, foretell, ninjutsu, free/copy casts) are grouped into their own arm documenting WHY they are neutral — `cast_facts` is populated only for the plain `CastSpell` seam — so that gap is visible rather than silent. [LOW] bounded-score regression: `MAX_REWARDED_ENGINES` had no test. Adds a capped case (MAX+1 engines must not out-score MAX) plus a below-cap scaling control so the pair pins a CAP rather than a constant, and asserts the `engines` fact still reports the true uncapped count. The constant is `pub(crate)` so the assertion tracks it instead of a copied literal. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
All three addressed at [MED] Preflight ran before the card-local testCorrect, and worse than a missed optimization: the module doc claimed the card-local check "runs FIRST and rejects every non-draw action" while the code did the opposite. The stated contract and the implementation disagreed. Split into two gates evaluated cheapest-discriminator-first:
[LOW] Wildcard
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 1a411e1c2e73fed826867ed68c7bff0bdc0dfa15.
[HIGH] The new exhaustive GameAction match is not exhaustive against current main. Evidence: crates/phase-ai/src/policies/draw_payoff.rs:146 has no GameAction::EndContinuousEffect { .. } arm, while current main defines it at crates/engine/src/types/actions.rs:922; the current-head Rust lint job fails with E0004 at draw_payoff.rs:146. Why it matters: phase-ai does not compile, so the required Rust lint/tests, paired-seed AI gate, decision-cost perf gate, and WASM check cannot run. Suggested fix: merge/rebase current main, classify EndContinuousEffect { .. } explicitly as neutral in candidate_draws_structurally, and push a head for which the required checks are green.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Summary
Adds a draw-matters deck-feature axis (
DrawMattersFeature) and its companionDrawPayoffPolicy— CR 121.1 "whenever you draw."With a "whenever you draw a card" engine on the battlefield — The Locust God (make an Insect), Psychosis Crawler / Niv-Mizzet (ping each opponent), Chulane — every extra draw is a repeatable value trigger.
card_advantagevalues having cards but not triggering the engine, so the AI won't lean into an extra-draw spell or ability when it has a payoff out. This axis lets a policy see that engine.Structural detection, no card-name matching:
Effect::Draw { target: Controller }(CR 121.1) — the card-draw enablers that feed the engineTriggerMode::Drawntrigger (CR 121.1) that is controller-scoped (valid_targetNone/Controller) and not self-referential, so a "when this card is drawn" hand trigger is excluded — only repeatable battlefield engines countCommitment is a geometric mean over (source, payoff) — both mandatory: card draw with no engine is just card advantage (
card_advantagegoverns it), an engine with no way to draw extra only fires on the natural draw for turn.Policy rewards a
CastSpell/ActivateAbilitythat draws the controller a card — detected via the action's ownCastFactsprimary/ETB effects (a cast permanent's activated draw ability doesn't fire on cast) or the activated ability — when the AI controls a live draw engine (structural match overtrigger_definitions, the runtime trigger authority). Composes withcard_advantagethe same wayCyclingDiscipline(patience) composes with a payoff policy.Files changed
crates/phase-ai/src/features/draw_matters.rs(new) ·features/tests/draw_matters.rs(new)crates/phase-ai/src/policies/draw_payoff.rs(new) ·policies/tests/draw_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.rsCR references
CR 121.1— a card is drawn (theTriggerMode::Drawnevent and theEffect::Drawenabler)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 — does this action draw the controller a card (its ownCastFactsprimary/ETB effects, or the activated ability) — runs FIRST and rejects every non-draw action; only then does it scan the battlefield for a live engine, and only in a deck whoseactivationfloor is already cleared. No affordability sweep, nofind_legal_targets.Verification
Round-11 head
1a411e1c2:cargo test -p engine --lib— 17817 passed; 0 failed (covers the inlinecfg(test)suites inreplacement.rsanddraw.rs, the two files this round changes)cargo test -p phase-ai --lib— 1636 passed; 0 failed (feature-detection axes + policy verdict branches, incl. registry-routedCastSpellandActivateAbilityregressions, and six paired replacement cases: mandatoryexecutesubstitution, mandatoryruntime_executesubstitution and zero-count rescale all withhold; optional substitution, opponent-scoped substitution and positive count rescale all still pay)cargo test -p engine --test integration draw_preflight_matches_live_pipeline— 8 passed; 0 failed — the preflight-vs-pipeline equivalence suite: each shape askscan_draw_at_least_onefor a prediction, then drives the REAL draw and assertspredicted == observed, with expected delivery pinned independently so a regression breaking both sides alike still fails. Covers all five suppression legs (draw restriction, empty library,Prevent, both substitute slots, zero-count) plus unreplaced and count-modified surviving controls.cargo clippy -p engine --all-targets --features proptest -- -D warnings— cleancargo clippy -p phase-ai --all-targets -- -D warnings— cleancargo fmt --all— cleanscripts/draw_replacement_census.py --producers --check— PASS (the file's two Draw-definition producers are consolidated into one parameterized helper; single renamed row, re-frozen with--write)cargo ai-gate— the policy activates only on a draw-engine deck, which none of the three gate matchups (mono-red burn, affinity, Selesnya enchantress) is, soDrawPayoffPolicy::activationreturnsNoneand it never fires; CI's paired-seed AI gate is authoritative on this head.Gate A
./scripts/check-parser-combinators.shagainst the committed head:Notes
draw_payoff_bonusis registeredUNTUNEDpending a paired-seed calibration.trigger_definitionsat decision time (not a name lookup), the pattern adopted for the cycling axis.card_advantage/spellslinger_prowess:card_advantagescores the card itself; this axis adds the extra value a draw carries when it fires an engine.spellslinger_prowesscounts spell-cast triggers; a draw event is a disjoint trigger. A card can read on both axes — the overlap is intentional and the axes stay independent.Summary by CodeRabbit
draw_payoff_bonustuning) that prioritizes drawing when qualifying draw-payoff engines are active.