feat(phase-ai): add poison-clock deck-feature axis + PoisonClockPolicy#6543
Conversation
CR 104.3d makes ten poison counters a loss condition entirely independent of
life total, tracked in a dedicated `Player.poison_counters` field. Nothing in
the AI scored progress along it, so an infect/toxic deck's whole plan read as
doing nothing: a proliferate taking an opponent from 9 to 10 poison scored the
same as one taking them from 0 to 1.
Feature (`features/poison.rs`) — three structural axes, no card names:
* sources: Keyword::Toxic(N) / Infect / Poisonous(N) on a creature face
(CR 702.164 / 702.90 / 702.70 — all three are combat-damage abilities, so
a noncreature face carrying the keyword is rejected)
* direct: Effect::GivePlayerCounter { counter_kind: Poison, target } whose
target can resolve to an opponent (CR 122.1f). A Controller/SelfRef scope
is rejected so a self-poison drawback is not read as a payoff
* proliferate: Effect::Proliferate | ProliferateTarget (CR 701.34a)
Commitment is a weighted sum, not a geometric mean: a dedicated Infect deck
runs zero direct-poison spells and often zero proliferate and must still read
as fully committed. Calibrated to Modern Infect (12 infect creatures + 2
proliferate over 37 nonland) > 0.85, with a superfriends anti-calibration
(2 proliferate, no source) staying below the policy floor.
Policy (`policies/poison.rs`) scores CastSpell + ActivateAbility by clock
progress, critical band when the action reaches ten. The branch structure is
driven by a rules detail: CR 701.34a defines proliferate over permanents and
players that ALREADY have a counter, so proliferating with no poisoned
opponent advances nothing and scores zero rather than a nudge — the inverse
would push the AI to durdle before the clock has started.
Every predicate is card-local or a plain u32 field read; no board-wide sweep,
no affordability call, nothing on the documented inner-loop landmine list.
`poison_clock_pressure` is registered UNTUNED: it is a CR 104.3d win-detector
weight whose magnitude is load-bearing for correctness, not taste.
Boundary with plus_one_counters: that axis already counts Proliferate as a
+1/+1 enabler. The overlap is intentional and the axes stay independent — one
proliferate card genuinely serves both decks, and a Hardened Scales deck
scores high there while scoring ~0 here.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
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 poison deck analysis, commitment calibration, and a registered poison-clock tactical policy. The policy evaluates direct poison, proliferate, and combat poison actions, detects lethal progress, applies configurable pressure, and includes feature and policy tests. ChangesPoison clock AI support
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant DeckFeatures
participant PoisonDetection
participant PolicyRegistry
participant PoisonClockPolicy
participant GameState
DeckFeatures->>PoisonDetection: analyze deck poison commitment
PoisonDetection->>DeckFeatures: provide PoisonFeature
PolicyRegistry->>PoisonClockPolicy: route candidate action
PoisonClockPolicy->>GameState: inspect poison counters and targets
PoisonClockPolicy-->>PolicyRegistry: return tactical verdict
Possibly related PRs
Suggested labels: 🚥 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: 5
🧹 Nitpick comments (1)
crates/phase-ai/src/policies/tests/poison.rs (1)
1-54: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing direct test coverage for
PoisonClockPolicy::verdict().Existing tests cover
reaches_lethal,activation, andmost_poisoned_opponent, but none exerciseverdict()itself — the lethal-critical branch, the "no counters to proliferate" neutral branch, and the progress-scaled preference branch are all untested here.As per CLAUDE.md, "ensure new behaviors are backed by unit tests colocated with the feature/policy module."
🤖 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/poison.rs` around lines 1 - 54, Add direct unit tests for PoisonClockPolicy::verdict covering the lethal-critical branch, the neutral result when no poison counters can be proliferated, and the progress-scaled preference branch. Reuse the existing DeckFeatures, GameState, and PlayerId setup patterns, and assert each verdict’s expected outcome through the public verdict method.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/config.rs`:
- Around line 466-469: Update the PolicyPenalties field poison_clock_pressure
with serde(default = "default_poison_clock_pressure"), define or reuse the
default_poison_clock_pressure helper, and have the PolicyPenalties Default
implementation call that same helper so older serialized values deserialize
consistently.
In `@crates/phase-ai/src/features/poison.rs`:
- Around line 198-207: Update filter_can_hit_opponent to handle
TargetFilter::Not explicitly instead of relying on the wildcard fallback.
Evaluate the negated inner filter so cases such as Not { SelfRef } and Not {
Typed(controller: You) } correctly report whether they can hit an opponent,
while preserving the existing Or and And behavior.
In `@crates/phase-ai/src/policies/poison.rs`:
- Around line 95-104: Update most_poisoned_opponent to exclude eliminated
opponents by filtering on !player.is_eliminated before mapping poison_counters.
Preserve the existing ai_player exclusion and unwrap_or(0) fallback.
- Around line 117-119: Update PoisonClockPolicy::decision_kinds to include
DecisionKind::DeclareAttackers so poison-source attack decisions receive the
policy’s activation scoring; if attacker scoring is intentionally owned by
another policy, route poison-source attackers there instead while preserving
existing spell and ability decision handling.
- Around line 60-65: Update the GameAction::CastSpell handling in the abilities
lookup to resolve the selected modal branch before poison scoring. Use the
chosen mode’s abilities rather than the full printed obj.abilities list, so
DirectPoison is scored only when that mode adds poison; preserve existing
behavior for non-modal spells.
---
Nitpick comments:
In `@crates/phase-ai/src/policies/tests/poison.rs`:
- Around line 1-54: Add direct unit tests for PoisonClockPolicy::verdict
covering the lethal-critical branch, the neutral result when no poison counters
can be proliferated, and the progress-scaled preference branch. Reuse the
existing DeckFeatures, GameState, and PlayerId setup patterns, and assert each
verdict’s expected outcome through the public verdict method.
🪄 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: 2bc1119d-ceef-4aca-80e0-3f23029f2a7b
📒 Files selected for processing (10)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/poison.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/features/tests/poison.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/poison.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/poison.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested
Reviewed current head d8ce4aaf0c167de33e93186389f057f5b9931ef4.
Blocking correctness
-
Preserve existing
PolicyPenaltiesartifacts.PolicyPenaltiesderivesDeserialize, and persisted tuning artifacts deserialize thepolicy_penaltiessection directly incrates/phase-ai/src/bin/ai_tune.rs:294-295. The newpoison_clock_pressurefield atcrates/phase-ai/src/config.rs:466-469has no serde default, unlike the recently added fields around it, so every artifact created before this change fails to load. Add a shareddefault_poison_clock_pressure()used by both#[serde(default = ...)]andDefault, plus a compatibility test that deserializes a pre-field value. -
Handle modal abilities at the actual choice seam. Both poison classifiers call
collect_chain_effects, whose documented implementation atcrates/phase-ai/src/ability_chain.rs:16-25traverses only an ability'seffectandsub_ability. It omitsAbilityDefinition::mode_abilities, so poison-only modes are absent from deck-time detection and the live policy. Mode selection is subsequently a distinctGameAction::SelectModesunderWaitingFor::ModeChoice/AbilityModeChoice;crates/phase-ai/src/decision_kind.rs:108-113routes that decision throughActivateAbility, butPoisonClockPolicy::contributiononly handlesCastSpellandActivateAbilityatcrates/phase-ai/src/policies/poison.rs:59-77. A cast action has no selected modes yet, so it cannot safely stand in for the selected branch.Please use or introduce one mode-aware ability-tree authority, have deck detection and live classification share it, and score the selected modal branch at
SelectModesrather than treating the pre-selection cast as that branch. Cover poison and non-poison modal alternatives through the policy pipeline. This needs a coherent policy/decision-boundary change, not a local read of a chosen mode fromCastSpell. -
Make the creature poison clock affect combat choices. The feature's
source_countdeliberately classifies Toxic/Infect/Poisonous creatures as combat-damage sources (crates/phase-ai/src/features/poison.rs:142-157), butPoisonClockPolicy::decision_kindsonly includesCastSpellandActivateAbility(crates/phase-ai/src/policies/poison.rs:117-119). As written, an infect deck's primary progression action—declaring its poison source as an attacker—gets no poison-clock signal. Route a live poison-source attacker contribution throughDeclareAttackers(or demonstrate a correct existing combat authority that owns it), with positive and negative attacker tests. -
Use only live opponents for clock state.
most_poisoned_opponentfilters out the AI seat but notplayer.is_eliminated(crates/phase-ai/src/policies/poison.rs:95-104). Eliminated seats remain inGameState.players, so a dead player with the highest poison total can produce false lethal/progress pressure. Filter eliminated players and cover that multiplayer case. -
Complete opponent-target classification.
filter_can_hit_opponentdropsTargetFilter::Notthrough its wildcard (crates/phase-ai/src/features/poison.rs:198-207), even though negated player scopes can target an opponent (for example,Not { SelfRef }andNot { Typed(controller: You) }). Add a soundNotarm consistent with the engine's player-filter semantics and test both matching and non-matching negated scopes.
The existing policy tests exercise activation and helper boundaries, but not PoisonClockPolicy::verdict; the requested modal/combat work should add discriminating runtime-policy coverage for lethal, no-poison proliferate, and nonlethal progress branches as well.
Resolves the five blocking items from the maintainer review of phase-rs#6543. 1. Preserve pre-field PolicyPenalties artifacts. `poison_clock_pressure` now has `#[serde(default = "default_poison_clock_pressure")]`, sharing one default fn with the `Default` impl (matching every sibling field). `ai_tune` deserializes the `policy_penalties` section straight into the struct, so an artifact written before this field must still load — a compatibility test drives that with a pre-poison artifact. 2. Handle modal abilities at the actual choice seam. `ability_chain` gains a typed `AbilityScope` (Unconditional | Potential) and one mode-aware authority, `collect_scoped_effects`. Deck-time detection walks `Potential` (mode_abilities + else_ability included, CR 700.2 / 608.2c); the live policy walks `Unconditional`. A modal cast is no longer credited with a mode's poison — the selected branch is scored at `SelectModes` (routed via ModeChoice / AbilityModeChoice → ActivateAbility), where the mode is actually chosen (CR 601.2b). The engine's own `modal_spell_mode_ability_refs` is the shared mode-list authority. 3. Route the creature poison clock through combat. The policy now declares `DeclareAttackers` and scores an attack by the poison its sources convert (CR 702.90b infect = power, CR 702.164c toxic = summed N, CR 702.70a poisonous), summed per defending player, keyed on damage to a PLAYER only (a planeswalker/battle attack adds nothing, CR 506.4c). 4. Use only live opponents. `most_poisoned_opponent` and the new `live_opponent_poison` filter `is_eliminated` seats (CR 800.4), so a dead player's counters can't produce phantom lethal/progress pressure. 5. Complete opponent-target classification. `filter_can_hit_opponent` gains a sound `Not` arm via a `filter_matches_every_opponent` complement, so `Not { SelfRef }` / `Not { Typed(controller: You) }` read as opponent poison while `Not { Opponent }` does not. Also fixes a latent scoring-contract bug the new verdict tests surfaced: the sub-lethal delta (`pressure` scalar 6.0 × progress) escaped the preference band, tripping `score_in_band`'s debug assert and silently clamping in release. Scoring now routes through `PolicyVerdict::score` with the non-lethal ceiling held at `STRONG_MAX`, so only reaching ten lands critical. Adds direct `verdict()` coverage (lethal / no-counters-to-proliferate / progress-scaled), the modal seam, combat routing (incl. eliminated-seat and planeswalker cases), negated player scopes, and registry-level routing for the modal and combat decision kinds. cargo test -p phase-ai --lib — 1412 passed cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Addressed all five blocking items in 1 — Preserve pre-field 2 — Modal abilities at the actual choice seam. Introduced a typed 3 — Creature clock affects combat. The policy declares 4 — Live opponents only. 5 — Complete opponent-target classification. Also fixed a latent scoring-contract bug the new Verification
Paired-seed AI gate (
|
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/ability_chain.rs`:
- Around line 43-45: Update the doc comment for the Potential variant to cite CR
608.2c instead of CR 603.4 for the else_ability branch, matching the engine’s
existing else_ability references while leaving the other rule citation
unchanged.
🪄 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: 2f898e01-8c84-4dba-a17a-78ff5cdc85a9
📒 Files selected for processing (7)
crates/engine/src/game/ability_utils.rscrates/phase-ai/src/ability_chain.rscrates/phase-ai/src/config.rscrates/phase-ai/src/features/poison.rscrates/phase-ai/src/features/tests/poison.rscrates/phase-ai/src/policies/poison.rscrates/phase-ai/src/policies/tests/poison.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/config.rs
- crates/phase-ai/src/features/poison.rs
|
All five blocking items are genuinely resolved in production code — what's left is test-only. The one HIGH item is that your policy's primary seam ( Re-reviewed at head ✅ Five prior blockers → all resolved
On the serde test specifically — it clears the strict bar. It does not round-trip a Rust-constructed struct. It serializes, removes the key from the JSON object, then deserializes. 🔴 Blocker[HIGH]
Delete Fix: one 🟡 Non-blocking — unexercised arms
All other CR numbers grep-verified correct: 104.3d, 122.1f, 701.34a, 702.164b/c, 702.90b/f, 702.70a, 601.2b, 700.2, 608.2c, 508.1, 800.4, 704.5c. ✅ Clean
Recommendationrequest-changes, test-only. The production code is right; please add:
Also: your "Files changed" list is stale — it omits Heads-up on your sibling #6544: it looks like it carries the exact defect item 1 fixed here — Your CI red is not your fault: |
…xup) `AbilityScope::Potential`'s doc cited CR 603.4 for the `else_ability` branch. CR 603.4 is the intervening-"if" clause rule for triggered abilities and describes no "Otherwise, ..." leg. The repo convention for `else_ability` is uniformly CR 608.2c (resolve instructions in the order written; later text may modify earlier text) — and `push_scoped_effects` in this same file already cites CR 608.2c for that exact branch, so the two annotations contradicted each other about one construct. Also drops the CR 506.4c parenthetical from `combat_contribution`: CR 506.4c governs a planeswalker or battle being REMOVED from combat, not an attack aimed at one. The load-bearing justification — CR 702.90b / CR 702.164c / CR 702.70a each keying on combat damage dealt to a *player* — is already cited in the same doc block and stands alone. Comment-only; no behavior change. All CR numbers grep-verified against docs/MagicCompRules.txt. Co-authored-by: minion1227 <romantymkiv1999@gmail.com>
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off. All five prior blocking items verified resolved against source at the seams named, not against the response comment.
The one remaining blocker — the CR 603.4 citation on AbilityScope::Potential for the else_ability branch — is fixed in a maintainer fixup commit on this branch (c351235a), co-authored to you. CR 603.4 is the intervening-"if" clause rule for triggered abilities and describes no "Otherwise, ..." leg; the repo convention for else_ability is uniformly CR 608.2c, and push_scoped_effects ~30 lines below in the same file already cited CR 608.2c for that exact branch, so the two annotations contradicted each other about one construct. This also resolves CodeRabbit's open thread on ability_chain.rs:45.
The same commit drops the CR 506.4c parenthetical from combat_contribution: CR 506.4c governs a planeswalker or battle being removed from combat, not an attack aimed at one. The load-bearing justification — CR 702.90b / CR 702.164c / CR 702.70a each keying on combat damage dealt to a player — was already in the same doc block and stands on its own. Every CR number was grep-verified against docs/MagicCompRules.txt before it was written.
Both edits are comment-only; the reviewed logic is untouched.
Note on CI: Tauri compile check is red repo-wide and is not your fault. The release: v0.35.2 commit (current main tip) bumped the workspace version without regenerating client/src-tauri/Cargo.lock, so cargo check --locked refuses. Because the required Rust (fmt, clippy, test, coverage-gate) aggregator has needs: tauri-check, that failure transitively holds the required check red. Nothing to do on this PR — it will merge once the lockfile is regenerated on main.
Nice work on the scope discipline here: the typed AbilityScope instead of a bool, and the CR 601.2b reasoning that keeps a modal branch from being credited at CastSpell time, are both the right call.
Parse changes introduced by this PR✓ No card-parse changes detected. |
phase-rs#6543) Addresses matthewevans' second review. The five original blockers were confirmed resolved in production; this round closes the HIGH test-gap and the recommended follow-ups, plus two small correctness fixes he flagged. HIGH — the policy's primary seam was declared but never registry-tested. Every direct-poison test called `verdict()` directly, so `DecisionKind::Cast\ Spell` could be deleted from `decision_kinds()` with the whole suite green. Add `registry_routes_cast_spell_to_the_policy` (poison cast scores through the pipeline, inert cast routes-but-scores-nil), and companion routed coverage: * `registry_routes_activate_ability_to_the_policy` — exercises the `obj.abilities.get(index)` lookup, the per-index discrimination, and the out-of-range → neutral fallback. * `registry_routes_modal_spell_mode_choice_to_the_policy` — drives a real `WaitingFor::ModeChoice` + `PendingCast`, the sole production consumer of the new `modal_spell_mode_ability_refs` engine API. Calibration and predicate coverage: * Or/And fixtures for BOTH filter predicates, each flipping one branch's outcome under a `.any`/`.all` swap, so the opposite-quantifier inversion between `filter_can_hit_opponent` and `filter_matches_every_opponent` can no longer pass silently. * `CoreType::Land` calibration fixture — 20 lands must not move commitment; pins the nonland-denominator guard whose removal would silently rescale. * Two-sided calibration bounds (Modern Infect 0.904 ± 0.01, superfriends 0.061 ± 0.005) replacing one-sided asserts that a saturating source weight could slip past. * `MIN_CLOCK_PROGRESS` pinned by a flat-plateau equality (0 and 1 poison score identically); deleting the floor splits them. Correctness (maintainer-flagged, non-blocking): * Combat lethal is capped at `STRONG_MAX`, strictly below the critical band a guaranteed direct poison reaches — CR 509.1a: a declared attack can be blocked or prevented, so a poison swing is a strong play, not a booked win. `score_clock` gains a per-branch `ceiling`. * `source_names` had no consumer — dropped the field, its population, and its two guarding tests rather than leave dead data. * CR nits: `608.2c` (not 603.4) for the `else_ability` branch at ability_chain.rs; dropped the off-point `506.4c` from combat_contribution. Deliberately deferred (per review): the multiplayer `SelectTarget` refinement (DirectPoison scores vs the most-poisoned seat but doesn't yet own target choice in 3+ player games) and the inherited `CR 109.3` citation, which the reviewer noted belongs on its own repo-wide commit. cargo test -p phase-ai --lib — 1420 passed (poison suite 50 → 58) cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Head branch was pushed to by a user without write access
|
Thanks for the thorough re-review and the HIGH — primary seam now registry-tested
Recommended follow-ups (all in)
Also
Deferred, per your notes
Heads-up ack on #6544: same serde-default defect at its
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-approved on current head 7d23e6c209.
The prior approval (22:20:54) was filed against c351235a57; you pushed 7d23e6c209 at 22:44:31, after it. reviewDecision still read APPROVED only because this repo's branch protection doesn't auto-dismiss on push, so the green state was covering a head nobody had reviewed. Re-reviewing the delta before it goes to the queue.
The delta is not test-only despite the test(...) subject — it carries two production changes, both sound and both maintainer-directed:
score_clockgains a per-branchceiling.DirectPoison/Proliferatemay reachCRITICAL_MAX;CombatDamageis capped atSTRONG_MAX. This is the CR 509.1a distinction I suggested — a declared attack can be blocked or prevented, so a would-be-lethal poison swing is a strong play, not a booked win. Verified all three call sites thread the new argument, and the sub-lethal path still clamps toSTRONG_MAXindependent ofceiling, matching the doc comment.PoisonFeature::source_namesdropped. Confirmed zero residue anywhere incrates/phase-ai/— field, population, and its two guarding tests all removed together. It had no consumer, so this is dead-data removal, not a capability loss.
The HIGH item from my 22:06 review is answered the right way: registry_routes_cast_spell_to_the_policy drives a direct-poison CastSpell under WaitingFor::Priority through the registry, with an inert cast as the routes-but-scores-nil control, so deleting DecisionKind::CastSpell from decision_kinds() now reds the suite instead of leaving it green at 20/20. That is a discriminating test for the exact dead-in-production risk I raised.
On the two earlier red heads: c351235a57 and 5c235a5e86 show rollup: FAILURE, but the jobs are cancelled, not failed — superseded by your next push. No real failure there.
Enqueuing with --auto, so the queue admits it only once required checks report green on this head, including Paired-seed AI gate (currently queued). Note the queue is presently ejecting batches because Tauri compile check fails on any merge-group ref built on main's release tip 21a53d50cb (its Cargo.lock still pins phase-tauri 0.35.1). That is a maintainer-side break, nothing to do with this PR; the fix is #6568, already in the queue, and this will serialize behind it.
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/features/tests/poison.rs`:
- Around line 200-204: Remove the “CR 104.3d” citation from the comment above
the land-exclusion logic, while preserving the explanation of the poison plan’s
nonland payload and the CoreType::Land guard behavior.
🪄 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: 1a58e56a-ac82-400e-9e38-0e465e2a55ce
📒 Files selected for processing (4)
crates/phase-ai/src/features/poison.rscrates/phase-ai/src/features/tests/poison.rscrates/phase-ai/src/policies/poison.rscrates/phase-ai/src/policies/tests/poison.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/phase-ai/src/policies/poison.rs
…est (phase-rs#6543) CR 104.3d is the ten-poison-counters loss rule; it says nothing about excluding lands from a commitment denominator. That exclusion is a deck-modelling choice in `detect`, not a rules requirement, so it carries no CR citation at all. Per CLAUDE.md a wrong CR number is worse than none — it creates false confidence that the code was verified against that rule. Comment-only; the guard and its assertions are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Head branch was pushed to by a user without write access
|
Fixed the one actionable CodeRabbit finding in
Re-verified the rest of the citations this branch adds:
|
|
Re-reviewed on the live head 📋 Why this needed a re-review at allMy approval was recorded against Verified, not taken on trust: ✅ Dropping CR 104.3d was correctGrep-verified at
The rule is real, but it governs the ten-poison loss SBA and says nothing about excluding lands from a commitment denominator. Citing it there was a genuine defect, and removing it rather than substituting another number is the right resolution — no CR governs a deck-modelling heuristic, and inventing a plausible-looking replacement would have been worse than none. Your replacement sentence ("this is a deck-modelling choice, not a rules requirement, so it carries no CR citation") is the correct thing to leave behind. The real 104.3d citations on ✅ Substance re-verified at full bar on the live head
The 🟡 Two non-blocking
Poison-aware target selection isn't scored — and my earlier provenance concern about this is refuted, so I want to state that plainly. 📋 Merge mechanics — sequencing matters hereThe branch is Recommendation: approve and enqueue once the branch is current and CI is green. No blocking findings. Nothing is open from CodeRabbit either — all seven inline comments across its three rounds are resolved, including the CR 104.3d one this push fixed. |
|
Branch updated to current Sequencing per your note: updated first so the approval re-anchors on a commit that is actually current, rather than approving now and re-stranding it on a superseded head.
Thanks for the refutation on the target-selection point, and agreed on |
|
CI has settled on the updated head ✅ Passing, including the two that matter most here: Paired-seed AI gate and Tauri compile check (the latter now that #6568 is on main). Also Rust lint / both test shards / coverage-gate / card data / WASM / frontend. 🟡
I can't re-trigger it — So it needs either a re-run on your side or the merge queue's own CI on enqueue. Nothing in the diff has changed since your review — head is still |
Resolves the deterministic crates/engine/tests/integration/main.rs mod-line conflict by keeping both test modules in rustfmt-sorted order. Maintainer-side port: main advanced under this PR tonight (phase-rs#6538 phase-rs#6543 phase-rs#6545 phase-rs#6554 phase-rs#6563 phase-rs#6579). The phase-tauri 0.35.1->0.35.2 client/src-tauri/Cargo.lock bump is now a no-op, since main landed the identical bump in phase-rs#6568.
Ports this PR onto phase-rs#6543 (poison-clock deck-feature axis + PoisonClockPolicy), which landed tonight and registers an axis+policy at the same wiring sites this PR uses for the graveyard type-diversity axis. All eight conflicts are both-sides-add at a registration point; every one keeps BOTH entries, with main's landed entry first: - features/mod.rs: `DeckFeatures.poison` + `.graveyard_types` fields, and both `detect(deck)` calls in `analyze()`. - policies/registry.rs: `PolicyId::{PoisonClock, GraveyardTypes}` and both `Box::new(..)` entries in `PolicyRegistry::default()`. Vec order is not load-bearing (the registry indexes `by_kind` and applies `delta * activation` once per policy). - config.rs: both `PolicyPenalties` fields, both `Default` initializers, both `UNTUNED_POLICY_PENALTY_FIELDS` entries, and the two independently-written pre-artifact serde back-compat tests split back into two whole `#[test]` fns (each pins its own field's `#[serde(default)]` wiring). No baselines refreshed.
phase-rs#6543) * feat(phase-ai): add poison-clock deck-feature axis + PoisonClockPolicy CR 104.3d makes ten poison counters a loss condition entirely independent of life total, tracked in a dedicated `Player.poison_counters` field. Nothing in the AI scored progress along it, so an infect/toxic deck's whole plan read as doing nothing: a proliferate taking an opponent from 9 to 10 poison scored the same as one taking them from 0 to 1. Feature (`features/poison.rs`) — three structural axes, no card names: * sources: Keyword::Toxic(N) / Infect / Poisonous(N) on a creature face (CR 702.164 / 702.90 / 702.70 — all three are combat-damage abilities, so a noncreature face carrying the keyword is rejected) * direct: Effect::GivePlayerCounter { counter_kind: Poison, target } whose target can resolve to an opponent (CR 122.1f). A Controller/SelfRef scope is rejected so a self-poison drawback is not read as a payoff * proliferate: Effect::Proliferate | ProliferateTarget (CR 701.34a) Commitment is a weighted sum, not a geometric mean: a dedicated Infect deck runs zero direct-poison spells and often zero proliferate and must still read as fully committed. Calibrated to Modern Infect (12 infect creatures + 2 proliferate over 37 nonland) > 0.85, with a superfriends anti-calibration (2 proliferate, no source) staying below the policy floor. Policy (`policies/poison.rs`) scores CastSpell + ActivateAbility by clock progress, critical band when the action reaches ten. The branch structure is driven by a rules detail: CR 701.34a defines proliferate over permanents and players that ALREADY have a counter, so proliferating with no poisoned opponent advances nothing and scores zero rather than a nudge — the inverse would push the AI to durdle before the clock has started. Every predicate is card-local or a plain u32 field read; no board-wide sweep, no affordability call, nothing on the documented inner-loop landmine list. `poison_clock_pressure` is registered UNTUNED: it is a CR 104.3d win-detector weight whose magnitude is load-bearing for correctness, not taste. Boundary with plus_one_counters: that axis already counts Proliferate as a +1/+1 enabler. The overlap is intentional and the axes stay independent — one proliferate card genuinely serves both decks, and a Hardened Scales deck scores high there while scoring ~0 here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(phase-ai): address review on poison-clock axis (phase-rs#6543) Resolves the five blocking items from the maintainer review of phase-rs#6543. 1. Preserve pre-field PolicyPenalties artifacts. `poison_clock_pressure` now has `#[serde(default = "default_poison_clock_pressure")]`, sharing one default fn with the `Default` impl (matching every sibling field). `ai_tune` deserializes the `policy_penalties` section straight into the struct, so an artifact written before this field must still load — a compatibility test drives that with a pre-poison artifact. 2. Handle modal abilities at the actual choice seam. `ability_chain` gains a typed `AbilityScope` (Unconditional | Potential) and one mode-aware authority, `collect_scoped_effects`. Deck-time detection walks `Potential` (mode_abilities + else_ability included, CR 700.2 / 608.2c); the live policy walks `Unconditional`. A modal cast is no longer credited with a mode's poison — the selected branch is scored at `SelectModes` (routed via ModeChoice / AbilityModeChoice → ActivateAbility), where the mode is actually chosen (CR 601.2b). The engine's own `modal_spell_mode_ability_refs` is the shared mode-list authority. 3. Route the creature poison clock through combat. The policy now declares `DeclareAttackers` and scores an attack by the poison its sources convert (CR 702.90b infect = power, CR 702.164c toxic = summed N, CR 702.70a poisonous), summed per defending player, keyed on damage to a PLAYER only (a planeswalker/battle attack adds nothing, CR 506.4c). 4. Use only live opponents. `most_poisoned_opponent` and the new `live_opponent_poison` filter `is_eliminated` seats (CR 800.4), so a dead player's counters can't produce phantom lethal/progress pressure. 5. Complete opponent-target classification. `filter_can_hit_opponent` gains a sound `Not` arm via a `filter_matches_every_opponent` complement, so `Not { SelfRef }` / `Not { Typed(controller: You) }` read as opponent poison while `Not { Opponent }` does not. Also fixes a latent scoring-contract bug the new verdict tests surfaced: the sub-lethal delta (`pressure` scalar 6.0 × progress) escaped the preference band, tripping `score_in_band`'s debug assert and silently clamping in release. Scoring now routes through `PolicyVerdict::score` with the non-lethal ceiling held at `STRONG_MAX`, so only reaching ten lands critical. Adds direct `verdict()` coverage (lethal / no-counters-to-proliferate / progress-scaled), the modal seam, combat routing (incl. eliminated-seat and planeswalker cases), negated player scopes, and registry-level routing for the modal and combat decision kinds. cargo test -p phase-ai --lib — 1412 passed cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(phase-ai): cite CR 608.2c for the else_ability leg (maintainer fixup) `AbilityScope::Potential`'s doc cited CR 603.4 for the `else_ability` branch. CR 603.4 is the intervening-"if" clause rule for triggered abilities and describes no "Otherwise, ..." leg. The repo convention for `else_ability` is uniformly CR 608.2c (resolve instructions in the order written; later text may modify earlier text) — and `push_scoped_effects` in this same file already cites CR 608.2c for that exact branch, so the two annotations contradicted each other about one construct. Also drops the CR 506.4c parenthetical from `combat_contribution`: CR 506.4c governs a planeswalker or battle being REMOVED from combat, not an attack aimed at one. The load-bearing justification — CR 702.90b / CR 702.164c / CR 702.70a each keying on combat damage dealt to a *player* — is already cited in the same doc block and stands alone. Comment-only; no behavior change. All CR numbers grep-verified against docs/MagicCompRules.txt. Co-authored-by: minion1227 <romantymkiv1999@gmail.com> * test(phase-ai): registry-route every poison seam + tighten calibration (phase-rs#6543) Addresses matthewevans' second review. The five original blockers were confirmed resolved in production; this round closes the HIGH test-gap and the recommended follow-ups, plus two small correctness fixes he flagged. HIGH — the policy's primary seam was declared but never registry-tested. Every direct-poison test called `verdict()` directly, so `DecisionKind::Cast\ Spell` could be deleted from `decision_kinds()` with the whole suite green. Add `registry_routes_cast_spell_to_the_policy` (poison cast scores through the pipeline, inert cast routes-but-scores-nil), and companion routed coverage: * `registry_routes_activate_ability_to_the_policy` — exercises the `obj.abilities.get(index)` lookup, the per-index discrimination, and the out-of-range → neutral fallback. * `registry_routes_modal_spell_mode_choice_to_the_policy` — drives a real `WaitingFor::ModeChoice` + `PendingCast`, the sole production consumer of the new `modal_spell_mode_ability_refs` engine API. Calibration and predicate coverage: * Or/And fixtures for BOTH filter predicates, each flipping one branch's outcome under a `.any`/`.all` swap, so the opposite-quantifier inversion between `filter_can_hit_opponent` and `filter_matches_every_opponent` can no longer pass silently. * `CoreType::Land` calibration fixture — 20 lands must not move commitment; pins the nonland-denominator guard whose removal would silently rescale. * Two-sided calibration bounds (Modern Infect 0.904 ± 0.01, superfriends 0.061 ± 0.005) replacing one-sided asserts that a saturating source weight could slip past. * `MIN_CLOCK_PROGRESS` pinned by a flat-plateau equality (0 and 1 poison score identically); deleting the floor splits them. Correctness (maintainer-flagged, non-blocking): * Combat lethal is capped at `STRONG_MAX`, strictly below the critical band a guaranteed direct poison reaches — CR 509.1a: a declared attack can be blocked or prevented, so a poison swing is a strong play, not a booked win. `score_clock` gains a per-branch `ceiling`. * `source_names` had no consumer — dropped the field, its population, and its two guarding tests rather than leave dead data. * CR nits: `608.2c` (not 603.4) for the `else_ability` branch at ability_chain.rs; dropped the off-point `506.4c` from combat_contribution. Deliberately deferred (per review): the multiplayer `SelectTarget` refinement (DirectPoison scores vs the most-poisoned seat but doesn't yet own target choice in 3+ player games) and the inherited `CR 109.3` citation, which the reviewer noted belongs on its own repo-wide commit. cargo test -p phase-ai --lib — 1420 passed (poison suite 50 → 58) cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(phase-ai): drop the wrong CR 104.3d cite on the land-exclusion test (phase-rs#6543) CR 104.3d is the ten-poison-counters loss rule; it says nothing about excluding lands from a commitment denominator. That exclusion is a deck-modelling choice in `detect`, not a rules requirement, so it carries no CR citation at all. Per CLAUDE.md a wrong CR number is worse than none — it creates false confidence that the code was verified against that rule. Comment-only; the guard and its assertions are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
2 participants
Summary
Adds a poison-clock deck-feature axis (
PoisonFeature) and its companionPoisonClockPolicytophase-ai.CR 104.3d makes ten poison counters a loss condition entirely independent of life total, tracked in a dedicated
Player.poison_countersfield. Nothing in the AI scored progress along it, so an infect/toxic deck's whole plan read as doing nothing — a proliferate taking an opponent from 9 to 10 poison scored exactly the same as one taking them from 0 to 1.Three structural detection axes, no card-name matching:
Keyword::Toxic(N)/Infect/Poisonous(N)on a creature faceEffect::GivePlayerCounter { counter_kind: Poison, target }whose target can resolve to an opponentEffect::Proliferate | ProliferateTargetAll three keywords are combat-damage abilities, so a noncreature face carrying one is rejected. A
Controller/SelfReftarget scope is rejected so a self-poison drawback is not read as a payoff.Commitment is a weighted sum, not a geometric mean — a dedicated Infect deck runs zero direct-poison spells and often zero proliferate, and must still read as fully committed. Calibrated to Modern Infect (12 infect creatures + 2 proliferate over 37 nonland) > 0.85, with a superfriends anti-calibration (2 proliferate, no source) staying below the policy floor.
One rules detail drives the policy's branch structure. CR 701.34a defines proliferate over permanents and players that already have a counter. Proliferating with no poisoned opponent advances nothing, so that branch scores zero rather than a nudge — the inverse would push the AI to durdle before the clock has started.
Files changed
crates/phase-ai/src/features/poison.rs(new)crates/phase-ai/src/features/tests/poison.rs(new)crates/phase-ai/src/policies/poison.rs(new)crates/phase-ai/src/policies/tests/poison.rs(new)crates/phase-ai/src/features/mod.rs,policies/mod.rs,policies/registry.rs,policies/tests/mod.rs,features/tests/mod.rs,config.rsCR references
CR 104.3d— ten or more poison counters loses the game (SBA)CR 122.1f— "poisoned" is one or more poison countersCR 701.34a— proliferate operates only on permanents/players that already have a counterCR 702.164Toxic N ·CR 702.90Infect ·CR 702.70PoisonousCR 109.3— conjunction rule for theAndarm of the target-filter walkEvery number grep-verified against
docs/MagicCompRules.txtbefore it was written.Performance
verdict()runs per candidate per search node. Every predicate here is card-local (obj.abilities) or a plainu32field read onPlayer.poison_counters. No board-wide sweep, no affordability call, nofind_legal_targets— nothing on the documented inner-loop landmine list.activation()opts the whole policy out below the commitment floor.Measurement —
cargo ai-gatewith a paired controlThose WARNs are pre-existing and not caused by this PR. I ran the identical gate on clean
upstream/main(ebd8844) as a control and it produces a byte-identical report — same three WARNs, same flip counts, same p-values.crates/phase-ai/baselines/suite-baseline.jsonwas last refreshed on 2026-06-17 (#3492), roughly 200 upstream commits ago.Consistent with that, none of the three gate matchups contains a poison source, so
PoisonClockPolicy::activationreturnsNoneand the policy never fires on them — the control simply proves it empirically rather than by argument.I deliberately did not refresh the baseline. Absorbing a pre-existing divergence into a baseline as part of an unrelated feature PR would hide upstream drift. Flagging it instead: the suite baseline looks due for a refresh on its own commit.
Verification
cargo test -p phase-ai --lib— 1379 passed; 0 failed (13 feature tests + 4 policy tests new)cargo clippy -p phase-ai --all-targets -- -D warnings— cleancargo fmt --all— cleancargo ai-gate— 0 FAIL, with the paired control aboveNotes
poison_clock_pressureis registered inUNTUNED_POLICY_PENALTY_FIELDS: it is a CR 104.3d win-detector weight whose magnitude is load-bearing for correctness rather than taste, so CMA-ES should not tune it without a paired-seed calibration.features/tests/andpolicies/tests/per the house convention (no#[cfg(test)]in source files).plus_one_counters: that axis already countsEffect::Proliferateas a +1/+1 enabler. The overlap is intentional and the axes stay independent — one proliferate card genuinely serves both decks, and a Hardened Scales deck scores high there while scoring ~0 here.Tier: Frontier
Summary by CodeRabbit