feat(phase-ai): add graveyard type-diversity axis + GraveyardTypesPolicy#6544
Conversation
CR 207.2c lists delirium, descend and threshold as ABILITY WORDS with no rules
meaning — the mechanical content is entirely the underlying "N or more card
types among cards in your graveyard" condition (CR 205.2a). 95 cards in the
corpus read that quantity, and nothing in the AI modelled the graveyard's type
spread: a self-mill that supplied the fourth distinct card type, switching
every delirium payoff on, scored exactly the same as one that added a
redundant fifth creature.
Feature (`features/graveyard_types.rs`) — three structural axes, no card names:
* threshold payoffs: a `QuantityComparison` condition whose lhs is
`DistinctCardTypes { Zone { Graveyard, Controller } }`. `StaticDefinition`
and `TriggerDefinition` carry DISTINCT condition enums that happen to share
the shape, so each gets its own extractor (Backwoods Survivalists puts the
clause on the static, Autumnal Gloom on the trigger)
* scaling payoffs: the same quantity read as a dynamic magnitude with no
threshold (Consuming Blob, Tarmogoyf). A threshold payoff is deliberately
NOT also counted here, or delirium cards would be double-weighted
* enablers: Mill / DiscardCard / Discard / Surveil scoped to the controller.
An opponent-scoped mill is excluded — filling their graveyard does nothing
for this deck's threshold
`highest_threshold` is tracked so a descend 8 deck does not think it is done at
four card types.
Commitment is a geometric mean over (payoff, enabler): unlike the poison axis,
BOTH pillars are mandatory. Payoffs with no enablers never turn on reliably and
enablers with no payoff are just self-mill — neither is this archetype, and
both collapse to 0.0. Calibrated to a Modern delirium shell (8 threshold + 2
scaling + 8 enablers over 37 nonland) > 0.85, with a lone-Tarmogoyf
anti-calibration below the floor.
Policy scores CastSpell + ActivateAbility by threshold deficit — strong band
when the action supplies the LAST missing type, scaled down as the deficit
grows. Once the count is at or above the deck's highest threshold the branch
scores zero: every payoff is already live, so more diversity buys nothing, and
scoring it would make the AI durdle with self-mill after delirium is on.
Predicate order is deliberate for the search inner loop: the card-local AST
check runs first and rejects almost every candidate; only a confirmed
graveyard-filler pays for the graveyard scan, which walks one zone and never
touches the battlefield, mana affordability, or find_legal_targets.
`graveyard_types_progress` is registered UNTUNED pending a paired-seed
calibration.
Boundary with `reanimator`: that axis detects graveyard RECURSION TARGETS —
what is worth bringing back. This one measures the graveyard's TYPE SPREAD. A
graveyard of four creatures is excellent for reanimator and useless for
delirium; the axes are independent by design.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Warning Review limit reached
Next review available in: 39 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (6)
📝 WalkthroughWalkthroughThe PR adds structural graveyard card-type detection, exposes the resulting deck feature, introduces a tactical policy for graveyard-filling actions, registers configuration and policy identifiers, expands shared graveyard-fill recognition, and adds feature and policy tests. ChangesGraveyard type-diversity support
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DeckFeatures
participant GraveyardTypesFeature
participant CardAST
DeckFeatures->>GraveyardTypesFeature: analyze(deck)
GraveyardTypesFeature->>CardAST: inspect card abilities and effects
CardAST-->>GraveyardTypesFeature: return graveyard type signals
GraveyardTypesFeature-->>DeckFeatures: store commitment and threshold
sequenceDiagram
participant CandidateAction
participant GraveyardTypesPolicy
participant GameState
CandidateAction->>GraveyardTypesPolicy: evaluate action
GraveyardTypesPolicy->>GameState: count owned graveyard CoreTypes
GameState-->>GraveyardTypesPolicy: distinct type count
GraveyardTypesPolicy-->>CandidateAction: neutral, strong, or scaled verdict
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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
🤖 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/graveyard_types.rs`:
- Around line 239-244: Update the QuantityRef::DistinctCardTypes handling for
CardTypeSetSource::Zone at ZoneRef::Graveyard so CountScope::All is not
classified as own-graveyard progress; retain only the controller-scoped behavior
unless the feature and distinct_graveyard_types runtime counting are changed
together to honor the selected scope.
- Around line 195-211: Update static_graveyard_type_threshold’s
QuantityComparison and Not handling to accept only comparisons whose direction
rewards reaching at least a positive threshold; reject equality, less-than, and
other fewer-types predicates, and normalize strict greater-than bounds to the
equivalent inclusive threshold before returning the value. Ensure negated
threshold conditions are not treated as positive payoffs, while preserving the
existing And/Or aggregation behavior.
- Around line 145-149: Update the highest_threshold representation so absence of
any threshold is distinct from an actual delirium threshold; do not substitute
DEFAULT_DELIRIUM_THRESHOLD when no threshold cards are present. Propagate this
distinction through the policy logic so scaling-only decks continue scaling
without a ceiling, while mixed decks remain bounded only by their real threshold
value.
- Line 211: Replace the wildcard fallback arms in
static_graveyard_type_threshold, trigger_graveyard_type_threshold,
modification_quantity, and filter_is_controller_scoped with explicit matches for
every currently unsupported enum variant, preserving their existing None/false
behavior while making future variants compiler-visible.
In `@crates/phase-ai/src/policies/graveyard_types.rs`:
- Around line 88-103: Update the ability selection in the graveyard-progress
evaluation around the CastSpell and ActivateAbility match arms so CastSpell
resolves only the spell ability/effect chain actually executed by the cast,
rather than passing the permanent’s full abilities slice. Preserve the existing
ActivateAbility selection and ensure self-mill or discard abilities that are
merely available on a cast permanent do not receive credit.
🪄 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: 7b61e711-e3b3-4c8b-9229-38f7a4841d9b
📒 Files selected for processing (10)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/graveyard_types.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/graveyard_types.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/policies/graveyard_types.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/graveyard_types.rscrates/phase-ai/src/policies/tests/mod.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the focused implementation. I need a revision before we can merge this current head.
-
PolicyPenaltiesis deserialized as a complete value from existing tuning artifacts (ai_tune.rs), but the newgraveyard_types_progressfield has no serde default. Every artifact produced before this PR therefore fails to load with a missing-field error instead of retaining the shipped default. Please add the same field-level default pattern used by the neighboring recently-added penalties, and cover deserializing an artifact that predates this field. -
The scaling-only case is given
DEFAULT_DELIRIUM_THRESHOLDin the feature even though it has no threshold. The policy then returns neutral as soon as the graveyard has four types, so it stops rewarding progress for a payoff that continues to scale. Model the absence of a threshold distinctly and add a verdict-level test that a scaling-only deck still receives the intended progress signal above four types. -
Threshold extraction ignores
Comparator, treatsNotas positive, and takes the maximum threshold under bothAndandOr. This misclassifies conditions such as “fewer than four types” or negated comparisons as delirium payoffs;Oralso does not make every child a mandatory gate. Please normalize only genuinely positive mandatory comparisons (including the correct strict->boundary) and reject/infer the rest according to the condition's truth semantics. Tests need to discriminateGE,GT, negative comparators, negation, and compound conditions. -
quantity_reads_own_graveyard_typesacceptsCountScope::All, whiledistinct_graveyard_typesdeliberately scans only the AI player's owned graveyard. Consequently an all-graveyards payoff is detected as an own-graveyard plan but the policy computes a different quantity (e.g. the parsed all-graveyards Tarmogoyf-class shape). Either keep the feature/policy strictly controller-scoped or carry the source scope through and resolve it consistently at decision time. -
The candidate lookup does not use the action's authoritative ability semantics. On
CastSpellit walks every ability on the object, so casting a permanent with an activated self-mill/discard ability is rewarded even when the spell itself does not fill a graveyard. OnActivateAbilityit indexesGameObject::abilitiesdirectly even thoughcast_facts::effective_activated_abilitydocuments that this is not the engine's authoritative enumerated index space for runtime-granted abilities. Please use the established cast/activated-ability fact helpers (and the spell-only cast effect chain) and add regression coverage for both decisions.
I agree with the substantive current CodeRabbit reports on the threshold, scope, and cast-action issues and am leaving those threads intact. Its wildcard-exhaustiveness observation is a non-blocking style concern; the semantic fixes above are required first.
…-rs#6544) Resolves the five blocking items from the maintainer review of phase-rs#6544. 1. Preserve pre-field PolicyPenalties artifacts. `graveyard_types_progress` now carries `#[serde(default = "default_graveyard_types_progress")]`, and both the `Default` impl and the serde attribute call that one shared fn — the same pattern as the neighbouring recently-added penalties. `ai_tune` deserializes the `policy_penalties` section straight into the struct, so a pre-field artifact previously failed with a missing-field error; a compatibility test now strips the key from a serialized artifact, tunes a neighbour, and asserts the tuned value survives while the new one falls back to the shared default. 2. Model "no threshold" distinctly. `highest_threshold` becomes `Option<u32>`; the fabricated `DEFAULT_DELIRIUM_THRESHOLD` fallback (and the const) are gone. A scaling-only deck (Consuming Blob, Tarmogoyf) has no threshold to finish, so the policy no longer goes neutral at four types: a threshold deck races to its gate, and any deck with a scaling payoff keeps earning a diminishing-but-nonzero signal past it. Covered by verdict-level tests, including the required "scaling-only deck still gets progress above four types". 3. Respect comparison truth semantics. Threshold extraction now normalizes only genuinely positive mandatory gates: `>=` / `>` (with the strict-bound off-by-one, `> 3` ⟺ `>= 4`), in either orientation (`N <= types` flips to `types >= N`). `<`, `<=`, `=`, `!=` are rejected — they reward fewer or an exact count. `Not` no longer passes through as positive (negating "N or more" is a "fewer than N" condition). `And` takes the max mandatory threshold; `Or` yields one only when EVERY branch is a graveyard threshold (then the minimum), since a single non-graveyard branch lets the payoff fire without delirium. Tests discriminate GE, GT, each negative comparator, negation, mirrored orientation, and both compounds. 4. Keep the axis strictly controller-scoped. `quantity_reads_own_graveyard_types` no longer accepts `CountScope::All`: the policy counts only the AI's owned graveyard, so an all-graveyards payoff would be detected as an own-graveyard plan while the policy measured a different quantity. Scope match is explicit (Controller/Owner accepted per CR 108.3 + CR 109.5). 5. Use the action's authoritative ability semantics. `CastSpell` now inspects only the spell's own resolution chain via `CastFacts::primary_effects`, so casting a permanent that merely HAS an activated self-mill ability is no longer credited; `ActivateAbility` resolves through `cast_facts::effective_activated_ability`, the engine's enumerated index space (correct for runtime-granted abilities where `GameObject::abilities` is not). Regression coverage for both decisions, plus the negative case. Also routes the verdict through `PolicyVerdict::score` so a tuned-up scalar auto-bands instead of tripping a band assertion. cargo test -p phase-ai --lib — 1401 passed (graveyard suite 12 → 39) 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 — Model the absence of a threshold distinctly. 3 — Respect comparator direction and negation. Extraction now normalizes only genuinely positive mandatory gates:
Tests discriminate 4 — Keep the axis strictly controller-scoped. 5 — Use the action's authoritative ability semantics. Also: the verdict now routes through Verification
On CodeRabbit's wildcard-exhaustiveness note: I left the large |
|
All five prior blockers are genuinely fixed — I re-verified each independently rather than taking the summary. Two new blockers remain, and they're about reach, not correctness: as built, the axis switches itself off for the flagship delirium shell. ✅ The five prior blockers — verified fixed
The serde fix is the real thing rather than a half-fix: 🔴 Blocker 1 — enabler detection never reads
|
…alk (phase-rs#6544) Addresses the two reach blockers from the second review. They were mirror images of one omission — enabler detection read `abilities` but not `triggers`; threshold detection read `triggers` but not `abilities` — so this is one change that gives both detectors the same three-carrier, chain-walking scan. Blocker 1 — enabler detection never read `face.triggers`, which zeroed the axis. `compute_commitment` ends in a geometric mean, so `enabler_count == 0` forces `commitment == 0.0`, which drops the deck under `GRAVEYARD_TYPES_FLOOR` and switches the policy off entirely. Stitcher's Supplier — the archetypal delirium enabler — is `abilities: []` with mill triggers, so the flagship shell scored nothing at all. `fills_own_graveyard_parts` now takes `(abilities, triggers)` and walks `triggers[*].execute` too, mirroring `tokens_wide::is_token_generator_parts`. Blocker 2 — threshold extraction missed the third condition carrier, `AbilityDefinition.condition`. On Traverse the Ulvenwald the gate is not at `abilities[0].condition` but at `abilities[0].sub_ability.condition` (the "…instead search…" clause lowers onto the sub-ability, wrapped in `ConditionInstead`), so folding only the top level in would still have missed it. Added `ability_graveyard_type_threshold` (QuantityCheck / ConditionInstead / And-max / Or-all-then-min / Not-None, matching the two existing extractors) plus `ability_chain_graveyard_type_threshold`, which walks the sub_ability and else_ability chain. All three carriers now feed one `.max()`. Also, per the same review: * `Effect::Dig { rest_destination: Graveyard }` is now an enabler (CR 701.20e) — the densest type-spread filler on the axis. Needed the three-carrier walk too: Satyr Wayfinder carries it in triggers, Grisly Salvage in abilities. * The `CastSpell` gate no longer discards `CastFacts::immediate_etb_triggers`. Excluding *activated* abilities was right (CR 601.2); excluding ETB triggers overshot and made casting Stitcher's Supplier return `graveyard_types_na`. * Single authority restored: `reanimator::effect_fills_own_graveyard` is extended (DiscardCard, Surveil, rest-to-graveyard Dig; SelfRef / Typed-You / Or / And scopes) and both graveyard axes now call it, instead of two "single authorities" disagreeing about one question. * Docs: Tarmogoyf is `CountScope::All` and correctly rejected, so Consuming Blob is the example throughout; the zone-scan claim now matches the implementation; the enabler list includes Dig. * Dead branch removed — `scalar / deficit` already peaks at `scalar` when the deficit is 1. Fixtures were the reason all of this shipped green: every enabler fixture was an ability-borne Mill. Added trigger-borne mill, Dig-borne (both carriers) with a rest-to-library negative, ability-chain threshold (incl. participation in `highest_threshold`), a commitment-path guard that trigger-only enablers clear the floor, and ETB-trigger cast coverage with a non-filling control. Left alone per the review: the two inherited `CR 109.3` annotations, which are a repo-wide cleanup rather than this PR's. cargo test -p phase-ai --lib — 1409 passed (graveyard suite 39 → 47) cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Both reach blockers fixed in 🔴 Blocker 1 — enabler detection now reads
|
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/phase-ai/src/features/tests/graveyard_types.rs (1)
658-683: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing Not/And/Or regression coverage for the new ability-chain threshold carrier.
The static- and trigger-condition carriers each got dedicated tests for negation and compound-condition semantics (lines 280-390, 391-479), but the new ability-chain carrier (
ability_graveyard_type_threshold) only has positive-path coverage here. Since that function explicitly special-casesNotto reject inverted "fewer types" semantics, a regression test mirroring the existingNot-rejection tests forAbilityConditionwould close the gap and match this PR's stated goal of end-to-end building-block coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/features/tests/graveyard_types.rs` around lines 658 - 683, The ability-chain threshold carrier lacks regression coverage for negated conditions. Add a test alongside threshold_in_the_ability_chain_is_detected using ability_graveyard_type_threshold through the ability-chain fixture, asserting that a Not-wrapped “fewer types” condition is rejected and does not contribute to threshold_payoff_count or highest_threshold; mirror the existing AbilityCondition Not-rejection test pattern.
🤖 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/reanimator.rs`:
- Around line 281-289: Make target_fills_own_graveyard exhaustive by replacing
the wildcard arm with explicit matches for every remaining TargetFilter variant.
Preserve the existing behavior for Controller, Any, SelfRef, Typed, Or, and And,
while ensuring future controller-scoped variants cannot be silently treated as
false.
---
Nitpick comments:
In `@crates/phase-ai/src/features/tests/graveyard_types.rs`:
- Around line 658-683: The ability-chain threshold carrier lacks regression
coverage for negated conditions. Add a test alongside
threshold_in_the_ability_chain_is_detected using
ability_graveyard_type_threshold through the ability-chain fixture, asserting
that a Not-wrapped “fewer types” condition is rejected and does not contribute
to threshold_payoff_count or highest_threshold; mirror the existing
AbilityCondition Not-rejection test pattern.
🪄 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: 04640db3-94d5-4e16-b19f-2e861e551b0c
📒 Files selected for processing (5)
crates/phase-ai/src/features/graveyard_types.rscrates/phase-ai/src/features/reanimator.rscrates/phase-ai/src/features/tests/graveyard_types.rscrates/phase-ai/src/policies/graveyard_types.rscrates/phase-ai/src/policies/tests/graveyard_types.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/phase-ai/src/policies/graveyard_types.rs
| fn target_fills_own_graveyard(filter: &TargetFilter) -> bool { | ||
| matches!(filter, TargetFilter::Controller | TargetFilter::Any) | ||
| match filter { | ||
| TargetFilter::Controller | TargetFilter::Any | TargetFilter::SelfRef => true, | ||
| TargetFilter::Typed(typed) => matches!(typed.controller, Some(ControllerRef::You)), | ||
| TargetFilter::Or { filters } => filters.iter().any(target_fills_own_graveyard), | ||
| // Every constraint of a conjunction must hold for the match. | ||
| TargetFilter::And { filters } => filters.iter().all(target_fills_own_graveyard), | ||
| _ => false, | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
rg -n "pub enum TargetFilter" -A 60 crates/engine/src/types/ability.rsRepository: phase-rs/phase
Length of output: 3818
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== target_fills_own_graveyard ==\n'
rg -n "fn target_fills_own_graveyard|target_fills_own_graveyard\\(" crates/phase-ai/src/features/reanimator.rs -A 20 -B 8
printf '\n== TargetFilter match sites ==\n'
rg -n "TargetFilter::" crates -g '*.rs' | sed -n '1,220p'
printf '\n== TargetFilter enum tail ==\n'
sed -n '4640,4725p' crates/engine/src/types/ability.rsRepository: phase-rs/phase
Length of output: 32435
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/phase-ai/src/features/reanimator.rs')
for i, line in enumerate(p.read_text().splitlines(), 1):
if 260 <= i <= 305:
print(f"{i}:{line}")
PYRepository: phase-rs/phase
Length of output: 2642
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== Effect definitions ==\n'
rg -n "pub enum Effect|Mill \\{|Discard \\{|DiscardCard \\{|Surveil \\{|Dig \\{" crates/engine/src/types -A 8 -B 3
printf '\n== Conversions feeding mill/discard/surveil/dig ==\n'
rg -n "Effect::Mill|Effect::DiscardCard|Effect::Discard|Effect::Surveil|Effect::Dig" crates -g '*.rs' -A 4 -B 4 | sed -n '1,260p'
printf '\n== TargetFilter variants handling in convert ===\n'
rg -n "TargetFilter::(Not|SpecificObject|SpecificPlayer|StackAbility|StackSpell|Opponent|GrantingObject|SourceOrPaired|LastCreated|AttachedTo|TriggeringSource|ParentTarget|ExiledBySource|Player|Controller|SelfRef|Any)" crates/mtgish-import/src/convert crates/engine/src -g '*.rs' | sed -n '1,260p'Repository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path('crates/engine/src/types/ability.rs')
lines = p.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 4700 <= i <= 4795:
print(f"{i}:{line}")
PYRepository: phase-rs/phase
Length of output: 6106
Make target_fills_own_graveyard exhaustive (crates/phase-ai/src/features/reanimator.rs:281-289) TargetFilter is a closed enum here, so _ => false can hide a new controller-scoped variant and silently undercount self-enabling mill/discard effects. Match the remaining variants explicitly.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@crates/phase-ai/src/features/reanimator.rs` around lines 281 - 289, Make
target_fills_own_graveyard exhaustive by replacing the wildcard arm with
explicit matches for every remaining TargetFilter variant. Preserve the existing
behavior for Controller, Any, SelfRef, Typed, Or, and And, while ensuring future
controller-scoped variants cannot be silently treated as false.
Source: Path instructions
…er (phase-rs#6544) The static- and trigger-condition carriers each have negation and compound-condition regression tests; the new `AbilityCondition` carrier shipped with positive-path coverage only, so its `Not => None` rejection and its And-max / Or-all-then-min rules were unguarded. Adds, through the Traverse-shaped sub_ability fixture: * negated "N or more types" is not a payoff (the inverted "fewer than N" gate), matching the other two carriers, * a non-positive comparator on the same carrier is likewise rejected, * `And` takes the highest mandatory threshold, `Or` yields one only when every branch is a graveyard threshold (else the payoff can fire without delirium). cargo test -p phase-ai --lib — 1412 passed (graveyard suite 47 → 50) 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 the CodeRabbit round in ✅ Taken — Not/And/Or coverage for the new ability-chain carrier. Fair catch: the static and trigger carriers each had negation and compound tests, but the third carrier shipped positive-path only, so its ⏭️ Skipped — making
On the red
That pattern points at runner/concurrency exhaustion rather than anything in the diff, so I've left it alone per the "external CI → note, don't touch" rule. Locally this branch is green on the full suite and clippy; happy to re-run the gate once the runners drain. |
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.
|
Confirming your merge Timing overlap: #6543 landed while this loop was mid-cycle, so I resolved the same eight conflicts locally. Before pushing, my pre-push divergence check reported DIVERGED against Our resolutions agreed on every hunk, including the two non-obvious ones:
Verified on your merged tree:
Agreed the registry |
|
@matthewevans — requesting a re-review when you have a moment. Everything from both of your rounds is addressed and the branch is green on your merge head
CI is 15/15 green (Paired-seed AI gate, Decision-cost perf gate, Tauri compile check, full Rust suite). Locally: The |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-reviewed at 3bb5eefa. All five findings from the prior round are resolved, and each is now pinned by a test that discriminates the exact failure it fixes. Approving.
Findings, closed
- serde default —
#[serde(default = "default_graveyard_types_progress")]on the field, wired intoDefault, with the pre-existing-artifact rationale in the doc comment.ai_tuneartifact load is green in CI. - threshold absence —
highest_threshold: Option<u32>; the verdict routes aNone-threshold scaling deck into thehas_scalingbranch (scalar / (current+1), diminishing but never zero), so a Consuming Blob deck keeps being rewarded past four types. Pinned byverdict_still_rewards_scaling_only_deck_above_four_typesanddescend_eight_tracks_highest_threshold. - comparator / Not / And / Or —
positive_graveyard_thresholdhandles both orientations (flip_comparator), the strict->off-by-one (GT→bound+1), and rejects</<=/=/!=with an exhaustiveComparatormatch.Not→None;And→ max (mandatory conjunction, CR 109.3);Or→ threshold only when every branch is a graveyard gate, elseNone. Pinned bynon_positive_comparators_are_not_thresholds,mirrored_orientation_normalizes_to_lower_bound,negated_threshold_is_not_a_payoff,and_takes_the_highest_mandatory_threshold,or_is_a_gate_only_when_every_branch_is_graveyard— and the ability-chain twins. - CountScope consistency —
quantity_reads_own_graveyard_typesnow matches onlyController | Owner, so an all-graveyards (Tarmogoyf-class) payoff is no longer misread as an own-graveyard plan against the controller-scopeddistinct_graveyard_types. Pinned byall_graveyards_scope_is_not_own_graveyard. - authoritative ability semantics —
candidate_fills_own_graveyardusescast_facts()(primary_effects+immediate_etb_triggers, correctly excluding activated abilities on a cast per CR 601.2) andeffective_activated_ability()(the runtime-enumerated index per CR 602.2) instead of walkingGameObject::abilities. Pinned byverdict_ignores_cast_of_a_body_with_an_activated_mill,verdict_credits_the_activated_mill_ability, and the ETB-trigger pair.
Gates
Paired-seed AI gate and Decision-cost perf gate both green — the live registry addition doesn't regress AI strength or the per-candidate verdict() cost, and no baseline bump was required. The condition combinators are decomposed into typed helpers with CR annotations rather than card-name matching, and the extractor is shared across all three condition carriers (static / trigger / ability-chain) — the right seam. Nice work.
|
Status on head One CodeRabbit thread is still open — the exhaustiveness nit on
Given that, and that the PR is already approved and the paired-seed gate is expensive to re-run, I'd rather tighten this to an explicit exhaustive match in a small follow-up than push a new commit onto an approved head now. Happy to do that immediately if you'd prefer it in-PR instead — just say the word. |
2 participants
Summary
Adds a graveyard card-type-diversity axis (
GraveyardTypesFeature) and its companionGraveyardTypesPolicy— the delirium / descend / threshold / Goyf axis.CR 207.2c lists delirium, descend and threshold as ability words with no rules meaning, so the mechanical content is entirely the underlying "N or more card types among cards in your graveyard" condition (CR 205.2a). 95 cards in the corpus read that quantity, and nothing in the AI modelled the graveyard's type spread: a self-mill that supplied the fourth distinct card type — switching every delirium payoff on — scored exactly the same as one that added a redundant fifth creature.
Three structural axes, no card-name matching:
QuantityComparisonwhose lhs isDistinctCardTypes { Zone { Graveyard, Controller } }Mill/DiscardCard/Discard/Surveilscoped to the controllerTwo details worth flagging for review:
StaticDefinition.conditionandTriggerDefinition.conditionare distinct enums that happen to share theQuantityComparisonshape. Each gets its own extractor rather than a forced conversion — Backwoods Survivalists puts the delirium clause on the static, Autumnal Gloom puts it on the trigger.highest_thresholdis tracked so a descend 8 deck does not think it is finished at four card types. Opponent-scoped counts and opponent-scoped mill are both rejected — punishing an opponent's diverse graveyard, or filling it, does nothing for this deck's own threshold.Commitment is a geometric mean over (payoff, enabler) — unlike the poison axis, both pillars are mandatory. Payoffs with no enablers never turn on reliably; enablers with no payoff are just self-mill. Neither is this archetype, and both collapse to
0.0(two tests pin the collapse). Calibrated to a Modern delirium shell (8 threshold + 2 scaling + 8 enablers over 37 nonland) > 0.85, with a lone-Tarmogoyf anti-calibration below the floor.Policy scores by threshold deficit: strong band when the action supplies the last missing type, scaled down as the deficit grows. Once the count is at or above the deck's highest threshold the branch scores zero — every payoff is already live, so more diversity buys nothing, and scoring it would make the AI durdle with self-mill after delirium is already on.
Files changed
crates/phase-ai/src/features/graveyard_types.rs(new)crates/phase-ai/src/features/tests/graveyard_types.rs(new)crates/phase-ai/src/policies/graveyard_types.rs(new)crates/phase-ai/src/policies/tests/graveyard_types.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 207.2c— delirium / descend / threshold / undergrowth are ability words with no rules meaningCR 205.2a— the card typesCR 404.1— a graveyard is its owner's discard pileCR 109.2a+CR 400.1— zone scoping for the countCR 109.3— conjunction rule for theAndarms of the condition and filter walksCR 701.17Mill ·CR 701.25SurveilEvery number grep-verified against
docs/MagicCompRules.txtbefore it was written.Performance
verdict()runs per candidate per search node, so predicate order is deliberate: the card-local AST check (fills_own_graveyard_partsover just this candidate's abilities) runs first and rejects almost every candidate. Only a confirmed graveyard-filler pays for the graveyard scan, which walks one zone's objects and never touches the battlefield, mana affordability,find_legal_targets, or anySimulationFilter-cloning path.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 — this branch's exact base) 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 2026-06-17 (#3492), roughly 200 upstream commits ago.Consistent with that, none of the three gate matchups is a graveyard-diversity deck, so
GraveyardTypesPolicy::activationreturnsNoneand the policy never fires on them — the control proves it empirically rather than by argument.I deliberately did not refresh the baseline. Absorbing 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— 1381 passed; 0 failed (16 feature tests + 3 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
graveyard_types_progressis registered inUNTUNED_POLICY_PENALTY_FIELDSpending a paired-seed calibration.features/tests/andpolicies/tests/per the house convention (no#[cfg(test)]in source files).reanimator: that axis detects graveyard recursion targets — what is worth bringing back. This one measures the graveyard's type spread. A graveyard of four creatures is excellent for reanimator and useless for delirium; the axes are independent by design.Tier: Frontier
Summary by CodeRabbit
graveyard_types_progresscalibration weight for threshold progress scoring (backward-compatible deserialization).