feat(phase-ai): add draw-matters deck-feature axis + DrawPayoffPolicy - #6688
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 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.
…ayoff match main added a 128th `GameAction` variant while this branch carried a 127-arm exhaustive match, so the merge CI builds were non-exhaustive and every Rust job failed to compile. Classify it neutral: ending a continuous effect draws no card. This is the maintenance cost of an exhaustive action match living inside a single policy — see the PR discussion for centralizing it in the routing authority instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
CI went red on What happenedNot the round-11 logic. Fixed by merging The decision I'd like your call onThe exhaustive match was the [LOW] nit from your last review, and it went stale within hours of landing. That isn't bad luck; it's structural. An exhaustive 128-arm Exhaustiveness inherently means new variants break something — the question is where. Three options as I see them:
I've implemented (1) because that's what was asked and it unblocks you now. I think (2) is the right architecture and I'm happy to do it, but it touches shared routing rather than this axis, so I'm not expanding scope into it unilaterally — say the word and it's a small change. Related finding, unchangedStill worth flagging from the last round: the cast-shaped siblings ( Verification at
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 49e22f5.
[MED] Zero-quantity candidate draws are rewarded as deliverable. Evidence: crates/phase-ai/src/features/draw_matters.rs:119-123 recognizes every controller-targeted Effect::Draw without examining its count, and crates/phase-ai/src/policies/draw_payoff.rs:117-133 subsequently asks only whether the player could draw one card in general. The live resolver instead resolves that effect quantity at crates/engine/src/game/effects/draw.rs:157-162; a fixed zero or a resolved X == 0 therefore produces no CardDrawn event. Why it matters: with a nonempty library and a live draw engine, a candidate whose own instruction draws zero receives the positive draw-payoff score even though it cannot trigger that engine. Suggested fix: require a positive candidate-specific resolved draw quantity (and conservatively stay neutral until an unbound dynamic quantity is known positive), then add a registry-routed fixed-zero and bound-X-zero regression paired with the live resolver no-event assertion.
…idates (round 12)
`is_draw_source_parts` matched every controller-targeted `Effect::Draw`
without inspecting its count, while the resolver resolves that quantity
(`resolve_quantity_with_targets(..).max(0)`) and emits `CardDrawn` only per
delivered card. A candidate whose own instruction draws ZERO therefore received
the full draw-payoff bonus despite being unable to fire any engine. The
player-level delivery gate added last round does not catch this: it asks whether
the PLAYER can draw, not whether this CANDIDATE draws.
Parameterizes the one classifier with a typed `DrawQuantity` rather than forking
a second copy: `Any` for deck-time classification (a "draw X" card is still an
enabler for archetype detection, its count unknowable at deck-build time) and
`ResolvesPositive` for a live candidate. The positive check delegates to the
engine's `resolve_quantity` authority instead of re-deriving quantity semantics,
so it agrees with the resolver by construction.
X is genuinely unbound at the candidate seam — `resolve_quantity` supplies no
`chosen_x` (only `resolve_quantity_with_targets` does, from a `ResolvedAbility`
a spell still being announced does not have), so every `Variable{X}` draw
resolves to zero and stays conservatively neutral until known positive.
Tests: registry-routed fixed-zero (withheld) and fixed-positive (rewarded)
pair; an X case asserting neutrality for both unset and set `cost_x_paid`,
documenting that a stale value from an earlier activation must not be mistaken
for this candidate's X; and a live-resolver assertion in the equivalence suite
that a zero-count draw emits no `CardDrawn`, paired with a positive control.
Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed at The finding
The fixParameterized the single classifier with a typed
One correction to your suggested regressionYou asked for a bound-X-zero case. I wrote one, it failed, and the failure was correct: there is no reachable bound-X state at this seam.
Rather than force a passing assertion, the test now pins the real behavior: neutral for both an unset and a set The live-resolver assertionAdded to the equivalence suite: resolving Verification at
|
|
Review hold for current head The current source review confirms that live candidate detection now requires a positive engine-resolved draw quantity, while deck classification retains its intentional potential-draw behavior. The zero-count regression is paired with the real draw pipeline and the parser diff confirms no card blast radius. However, the paired-seed AI gate and decision-cost performance gate are still running, so this head is not approvable or queueable yet. Please let those current-head gates complete, then request another review. |
|
@matthewevans — ready for re-review at The standing Every required check is green14 pass / 3 skip. Both Rust shards, Rust lint, coverage-gate, WASM, card-data, frontend, Tauri, lobby worker, decision-cost perf gate, and the paired-seed AI gate — the last of which had never completed a run on an earlier head (cancelled once, blocked by the compile break before that), so this is its first clean pass on this work. The one red check is Findings from your last three reviews
Two things I did not silently absorbThe exhaustive match is in the wrong home. It went stale against Your suggested bound-X-zero regression has no reachable state. Local verification at this head
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved at 7b8bdd3: the current implementation uses the engine-owned quantity, draw-delivery/replacement, and trigger-legality authorities; the zero/positive quantity regression is registry-routed and paired with a real draw-pipeline assertion. Current-head required checks, paired-seed gate, and decision-cost performance gate are green; the parse-diff artifact reports no card-parse changes.
Sibling PR phase-rs#6688 (draw-matters axis) merged as 7d63341, which collided with this branch in five files. Resolutions: - phase-ai config.rs / features/mod.rs / policies/registry.rs: keep both axes. Ordering follows the files' append-chronological convention, so draw-matters (already on main) precedes cycling. - engine ability_utils.rs: keep 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 and satisfies the single-authority contract this PR was asked for. Same for the Unimplemented placeholder check: a modal root with NO modes is still a real gap, because then the placeholder itself is what would resolve. - engine triggers.rs: keep the gated `Effect::unimplemented(..)` constructor (CLAUDE.md forbids hand-built `Effect::Unimplemented` literals), plus this branch's `announced_modal_choice` field and its round-10 regression.
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.