feat(phase-ai): add devotion deck-feature axis + DevotionPolicy#6590
feat(phase-ai): add devotion deck-feature axis + DevotionPolicy#6590minion1227 wants to merge 3 commits into
Conversation
CR 700.5: devotion to a color is the number of that color's mana symbols
among the mana costs of permanents you control. It is the payoff currency for
the Theros gods (not creatures below their DevotionGE threshold), Gray Merchant
drains, and X = devotion scalers — 43 cards in the corpus read it. The AI
models mana value and board presence but not pip density, so between two
comparable permanents it could not prefer the double-pip one, and it could not
see that casting one more colored permanent flips a dormant god into a body.
Feature (features/devotion.rs) — structural, no card names:
* threshold payoffs: StaticCondition::DevotionGE { colors, threshold },
walked through the Not/And/Or tree (Erebos gates its "isn't a creature" on
Not{DevotionGE})
* scaling payoffs: QuantityRef::Devotion anywhere in an effect's magnitude,
reached via the engine's own Effect::count_expr authority (so drains,
damage, token counts, draw counts, dynamic P/T are all covered without
hand-listing effect variants) or a continuous modification
* pip density: ManaCost::count_colored_pips, the single CR 700.5 counting
authority (hybrid {G/W}{G/W} = 2), over permanent faces only
`primary_color` is the deck's most-devoted color among the colors its payoffs
read; a ChosenColor payoff (Nykthos) makes every color eligible so the deck's
own densest color wins. `highest_threshold` is Option<u32> so a scaling-only
deck is never handed a fabricated god ceiling (mirrors graveyard_types).
Commitment is a geometric mean over (payoff, pip): both mandatory — pips with
no payoff is just a mono deck, a payoff with no pips never turns on. Calibrated
to Mono-Black Devotion > 0.85, with a lone-splashed-payoff anti-calibration
below the floor.
Policy scores CastSpell of a permanent by primary-color pips added, with a
distinct god-activation spike when the cast crosses a DevotionGE threshold
(turning a non-creature enchantment into a body — the "last missing piece"
structure graveyard_types uses). The card-local pip check runs first; only a
confirmed primary-color permanent pays for the count_devotion battlefield scan.
No affordability sweep, no find_legal_targets.
Both penalty fields registered UNTUNED pending a paired-seed calibration.
Known safe undercounts (documented): mana-production ramp (Nykthos lives in a
mana-ability production shape, not an Effect magnitude) and a spell's own
"costs {X} less where X is devotion" self-discount (already applied by the
engine, no per-cast decision to make). Both only lower commitment.
Boundary with tribal/mana_ramp: devotion counts colored pips, not creatures of
a type or mana sources — a five-Forest ramp deck has high mana_ramp and zero
devotion; a {B}{B} Gray Merchant deck is the reverse.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
📝 WalkthroughWalkthroughAdds CR 700.5 devotion deck analysis, exposes devotion data through ChangesDevotion analysis and policy
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant PolicyRegistry
participant DevotionPolicy
participant DeckFeatures
participant GameState
PolicyRegistry->>DevotionPolicy: evaluate CastSpell candidate
DevotionPolicy->>DeckFeatures: read devotion commitment and primary color
DevotionPolicy->>GameState: count battlefield devotion
GameState-->>DevotionPolicy: current devotion
DevotionPolicy-->>PolicyRegistry: scoring verdict
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
crates/phase-ai/src/features/devotion.rs (1)
67-71: 🎯 Functional Correctness | 🔵 Trivial
highest_thresholdcollapses multiple DevotionGE thresholds in the same color to a single max.
ColorThresholds::raise(Line 364-374) only keepscurrent.max(threshold), andDevotionFeature.highest_thresholdexposes just that one value. If a primary color ever had twoDevotionGEgates at different thresholds (the engine'sStaticCondition::DevotionGEallows anyu32),DevotionPolicy::verdict(policies/devotion.rs Lines 100-110) can only ever detect crossing the higher one — a cast that turns on the lower-threshold god silently falls through to the smooth pip-progress branch instead of the god-activation spike. Every currently-modeled Theros god uses threshold 5, so this doesn't bite with today's card pool, but the module's stated goal is to handle "a CLASS of cards, not one special case" — tracking a sorted set of thresholds (and finding the specific one this cast crosses) instead of just the max would generalize correctly.Also applies to: 363-385
🤖 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/devotion.rs` around lines 67 - 71, Replace the scalar highest_threshold tracking in ColorThresholds::raise and DevotionFeature with a sorted, deduplicated collection of all DevotionGE thresholds read for the primary color. Update DevotionPolicy::verdict to identify the specific threshold crossed by the cast, including lower thresholds when higher ones also exist, and trigger the god-activation spike for that crossing while preserving normal pip progress otherwise.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 476-481: Annotate the new Config fields devotion_pip_progress and
devotion_god_activation with serde default attributes matching the existing
policy penalty pattern, using defaults of 0.35 and 2.5 respectively. Ensure
older policy_penalties payloads that omit these fields deserialize successfully
with those fallback values.
In `@crates/phase-ai/src/features/devotion.rs`:
- Around line 180-194: Replace both local is_permanent* helpers in
crates/phase-ai/src/features/devotion.rs:180-194 and
crates/phase-ai/src/policies/devotion.rs:122-136 with the shared
CoreType::is_permanent_type() helper, updating each caller accordingly. Revise
both doc comments to cite CR 110.4 instead of CR 205.2a; no other behavior
changes are needed.
---
Nitpick comments:
In `@crates/phase-ai/src/features/devotion.rs`:
- Around line 67-71: Replace the scalar highest_threshold tracking in
ColorThresholds::raise and DevotionFeature with a sorted, deduplicated
collection of all DevotionGE thresholds read for the primary color. Update
DevotionPolicy::verdict to identify the specific threshold crossed by the cast,
including lower thresholds when higher ones also exist, and trigger the
god-activation spike for that crossing while preserving normal pip progress
otherwise.
🪄 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: 4c943e4c-fdba-4510-a1ab-ab915351b0b5
📒 Files selected for processing (10)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/devotion.rscrates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/devotion.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/policies/devotion.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/tests/devotion.rscrates/phase-ai/src/policies/tests/mod.rs
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — persisted AI artifacts and threshold detection regress.
🔴 Blocker
[HIGH] Preserve deserialization of pre-devotion PolicyPenalties artifacts. Evidence: crates/phase-ai/src/config.rs:476-481 adds required serde fields without defaults, while the existing compatibility pattern at :474,1555-1600 proves these sections load independently. Add shared default functions, #[serde(default = ...)], and a pre-devotion artifact test.
[MED] Retain every distinct devotion threshold. Evidence: crates/phase-ai/src/features/devotion.rs:138-146,363-385 keeps only the maximum threshold, and policies/devotion.rs:100-110 scores only it. A cast crossing a lower inactive threshold is missed whenever a higher one exists. Store deduplicated thresholds and reward any newly crossed threshold.
[LOW] Reuse the engine permanent-type authority. features/devotion.rs:180-194 and policies/devotion.rs:122-136 duplicate it and cite CR 205.2a, which only lists card types. CoreType::is_permanent_type already owns the distinction at crates/engine/src/types/card_type.rs:138; CR 110.4 defines permanent types.
Recommendation: request changes; restore artifact compatibility and threshold-class correctness before re-review.
…esholds, permanent-type authority Addresses matthewevans' CHANGES_REQUESTED on phase-rs#6590. [HIGH] Persisted-artifact compatibility. `devotion_pip_progress` and `devotion_god_activation` now carry `#[serde(default = "...")]` backed by shared `default_*` functions that the `Default` impl also calls, so an older `policy_penalties` payload that omits them deserializes with the 0.35/2.5 fallbacks instead of failing. Adds `policy_penalties_load_pre_devotion_artifact` mirroring the pre-poison / pre-graveyard artifact tests. [MED] Retain every distinct devotion threshold. `DevotionFeature` now carries `thresholds: Vec<u32>` (deduplicated, sorted) instead of a single maximum, and the policy rewards a cast that crosses ANY not-yet-met threshold — each Theros god turns on independently, so a cast reaching a lower gate activates that god even when a higher gate it does not reach also exists. A cast clearing several gates at once activates all of them (`gods_activated` fact). Two feature/policy tests pin the lower-gate-crossing and double-crossing cases. [LOW] Reuse the engine permanent-type authority. Both the feature and the policy now call `CoreType::is_permanent_type` (which itself cites CR 110.4) instead of re-implementing the match, and the annotations cite CR 110.4 (the permanent-type rule) rather than CR 205.2a (which only lists card types). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks — all three addressed in [HIGH] persisted-artifact compatibility. Both new penalties now carry [MED] distinct thresholds. [LOW] permanent-type authority. Both the feature and the policy now call Verification on |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/phase-ai/src/features/devotion.rs (1)
341-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
ColorThresholdsre-implements theColorTotalsper-color-storage building block.Both structs are the same shape: five named
ManaColorfields plusslot/slot_refexhaustive matches. Per crates path instructions, a new per-color container should compose the existing one rather than re-deriving it. Consider a small genericPerColor<T>(or an array indexed by aManaColor -> usizemapping) so both accumulation (ColorTotals) and threshold collection (ColorThresholds) share one indexing implementation instead of two parallel five-arm matches.As per path instructions,
crates/**/*.rs: "any new helper that duplicates an existing building block. Prefer composingstdprimitives and reusing existing helpers over re-implementation."🤖 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/devotion.rs` around lines 341 - 348, Replace the duplicated per-color storage in ColorThresholds with the existing ColorTotals building block or a shared generic PerColor<T> abstraction. Centralize ManaColor-to-storage indexing and exhaustive color handling so both accumulation and threshold collection reuse the same implementation, while preserving their distinct value types and behavior.
🤖 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/devotion.rs`:
- Around line 335-362: Update ColorThresholds::insert to retain every threshold
occurrence instead of deduplicating by numeric value: remove the contains-based
guard and always append the threshold to the color slot. Preserve the existing
per-color storage and sorting behavior in ColorThresholds::get so duplicate
thresholds remain available for independent god activations.
---
Nitpick comments:
In `@crates/phase-ai/src/features/devotion.rs`:
- Around line 341-348: Replace the duplicated per-color storage in
ColorThresholds with the existing ColorTotals building block or a shared generic
PerColor<T> abstraction. Centralize ManaColor-to-storage indexing and exhaustive
color handling so both accumulation and threshold collection reuse the same
implementation, while preserving their distinct value types and 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: db0ad41c-7f4f-4913-91a1-9a44638371b4
📒 Files selected for processing (5)
crates/phase-ai/src/config.rscrates/phase-ai/src/features/devotion.rscrates/phase-ai/src/features/tests/devotion.rscrates/phase-ai/src/policies/devotion.rscrates/phase-ai/src/policies/tests/devotion.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/phase-ai/src/features/tests/devotion.rs
- crates/phase-ai/src/config.rs
| /// Per-color set of DISTINCT god thresholds. Each Theros god turns on | ||
| /// independently at its own `DevotionGE` threshold, so a cast crossing a lower | ||
| /// gate (activating a smaller god) matters even when a higher gate exists — | ||
| /// keeping only the maximum would miss that. An empty slot distinguishes "no | ||
| /// gate in this color" from a real threshold, so a scaling-only deck is never | ||
| /// handed a fabricated ceiling. | ||
| #[derive(Default)] | ||
| struct ColorThresholds { | ||
| white: Vec<u32>, | ||
| blue: Vec<u32>, | ||
| black: Vec<u32>, | ||
| red: Vec<u32>, | ||
| green: Vec<u32>, | ||
| } | ||
|
|
||
| impl ColorThresholds { | ||
| fn insert(&mut self, color: ManaColor, threshold: u32) { | ||
| let slot = self.slot(color); | ||
| if !slot.contains(&threshold) { | ||
| slot.push(threshold); | ||
| } | ||
| } | ||
| /// The distinct thresholds in this color, sorted ascending. | ||
| fn get(&self, color: ManaColor) -> Vec<u32> { | ||
| let mut out = self.slot_ref(color).clone(); | ||
| out.sort_unstable(); | ||
| out | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Dedup-by-value collapses distinct gods sharing the same threshold.
insert dedupes by the numeric threshold value, not by payoff identity. When two different payoff cards in the same color happen to share a threshold (a very common real pattern — many Theros-cycle gods for the same color use an identical devotion threshold), only one entry is kept, even though both gods activate independently. This directly contradicts the doc comment at Line 69: "Each entry is one god that turns on independently" — it's only true when thresholds happen to be distinct.
This silently understates the AI's incentive: in policies/devotion.rs, crossed = feature.thresholds.iter().filter(|&&t| current < t && current + added >= t).count() (Lines 108-112) will report crossed == 1 for a cast that actually turns on two gods sharing that threshold, halving the intended activation bonus. The new tests in policies/tests/devotion.rs (verdict_crossing_lower_threshold_activates_that_god, verdict_crossing_two_thresholds_activates_both) only exercise distinct values (vec![3, 5]), so this gap is untested.
Since highest_devotion_gate is called once per deck entry and a face contributes at most one (colors, threshold) tuple, duplicate thresholds can only arise across distinct cards — so dropping the dedup check is safe and makes each vec entry genuinely correspond to one god, matching the doc's claim.
🐛 Proposed fix
impl ColorThresholds {
fn insert(&mut self, color: ManaColor, threshold: u32) {
- let slot = self.slot(color);
- if !slot.contains(&threshold) {
- slot.push(threshold);
- }
+ // Duplicates are meaningful: two payoffs sharing a threshold are two
+ // gods that both turn on when it's crossed.
+ self.slot(color).push(threshold);
}
- /// The distinct thresholds in this color, sorted ascending.
+ /// The thresholds gating each god in this color (one entry per payoff
+ /// card; duplicate values are kept — e.g. two gods both requiring
+ /// devotion 5 to black), sorted ascending.
fn get(&self, color: ManaColor) -> Vec<u32> {
let mut out = self.slot_ref(color).clone();
out.sort_unstable();
out
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /// Per-color set of DISTINCT god thresholds. Each Theros god turns on | |
| /// independently at its own `DevotionGE` threshold, so a cast crossing a lower | |
| /// gate (activating a smaller god) matters even when a higher gate exists — | |
| /// keeping only the maximum would miss that. An empty slot distinguishes "no | |
| /// gate in this color" from a real threshold, so a scaling-only deck is never | |
| /// handed a fabricated ceiling. | |
| #[derive(Default)] | |
| struct ColorThresholds { | |
| white: Vec<u32>, | |
| blue: Vec<u32>, | |
| black: Vec<u32>, | |
| red: Vec<u32>, | |
| green: Vec<u32>, | |
| } | |
| impl ColorThresholds { | |
| fn insert(&mut self, color: ManaColor, threshold: u32) { | |
| let slot = self.slot(color); | |
| if !slot.contains(&threshold) { | |
| slot.push(threshold); | |
| } | |
| } | |
| /// The distinct thresholds in this color, sorted ascending. | |
| fn get(&self, color: ManaColor) -> Vec<u32> { | |
| let mut out = self.slot_ref(color).clone(); | |
| out.sort_unstable(); | |
| out | |
| } | |
| /// Per-color set of DISTINCT god thresholds. Each Theros god turns on | |
| /// independently at its own `DevotionGE` threshold, so a cast crossing a lower | |
| /// gate (activating a smaller god) matters even when a higher gate exists — | |
| /// keeping only the maximum would miss that. An empty slot distinguishes "no | |
| /// gate in this color" from a real threshold, so a scaling-only deck is never | |
| /// handed a fabricated ceiling. | |
| #[derive(Default)] | |
| struct ColorThresholds { | |
| white: Vec<u32>, | |
| blue: Vec<u32>, | |
| black: Vec<u32>, | |
| red: Vec<u32>, | |
| green: Vec<u32>, | |
| } | |
| impl ColorThresholds { | |
| fn insert(&mut self, color: ManaColor, threshold: u32) { | |
| self.slot(color).push(threshold); | |
| } | |
| /// The thresholds gating each god in this color (one entry per payoff | |
| /// card; duplicate values are kept), sorted ascending. | |
| fn get(&self, color: ManaColor) -> Vec<u32> { | |
| let mut out = self.slot_ref(color).clone(); | |
| out.sort_unstable(); | |
| out |
🤖 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/devotion.rs` around lines 335 - 362, Update
ColorThresholds::insert to retain every threshold occurrence instead of
deduplicating by numeric value: remove the contains-based guard and always
append the threshold to the color slot. Preserve the existing per-color storage
and sorting behavior in ColorThresholds::get so duplicate thresholds remain
available for independent god activations.
|
Closed without implementation-diff review. The canonical model-declaration gate was published in |
Summary
Adds a devotion deck-feature axis (
DevotionFeature) and its companionDevotionPolicy— CR 700.5 mono-color pip density.CR 700.5: devotion to a color is the number of that color's mana symbols among the mana costs of permanents you control. It is the payoff currency for the Theros gods (not creatures below their
DevotionGEthreshold), Gray Merchant drains, and X = devotion scalers — 43 cards in the corpus read it. The AI models mana value and board presence but not pip density, so between two comparable permanents it could not prefer the double-pip one, and it could not see that casting one more colored permanent flips a dormant god into a body.Three structural detection axes, no card-name matching:
StaticCondition::DevotionGE { colors, threshold }, walked through theNot/And/Ortree (Erebos gates "isn't a creature" onNot{DevotionGE})QuantityRef::Devotionin an effect magnitude, reached via the engine's ownEffect::count_exprauthority — so drains, damage, token/draw counts and dynamic P/T are all covered without hand-listing effect variantsManaCost::count_colored_pips, the single CR 700.5 counting authority (hybrid{G/W}{G/W}= 2), over permanent faces onlyTwo design points worth a reviewer's eye:
primary_coloris the deck's most-devoted color among the colors its payoffs read. AChosenColorpayoff (Nykthos) makes every color eligible, so the deck's own densest color wins.highest_thresholdisOption<u32>so a scaling-only deck is never handed a fabricated god ceiling (mirrorsgraveyard_types).Effect::count_expr(magnitude),ManaCost::count_colored_pips(deck/cast pips), andengine::game::devotion::count_devotion(runtime battlefield devotion, which correctly drops phased-out permanents per CR 702.26b).Commitment is a geometric mean over (payoff, pip) — both mandatory: pips with no payoff is just a mono deck, a payoff with no pips never turns on. Both collapse to
0.0(tests pin each). Calibrated to Mono-Black Devotion > 0.85, with a lone-splashed-payoff anti-calibration below the floor.Policy scores
CastSpellof a permanent by primary-color pips added, with a distinct god-activation spike when the cast crosses aDevotionGEthreshold — turning a non-creature enchantment into a body, a multi-card swing rather than a marginal +1 (the "last missing piece" structuregraveyard_typesuses for the fourth card type).Files changed
crates/phase-ai/src/features/devotion.rs(new) ·features/tests/devotion.rs(new)crates/phase-ai/src/policies/devotion.rs(new) ·policies/tests/devotion.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 700.5— devotion is the count of a color's mana symbols among your permanentsCR 205.2a— only permanents can enter the battlefield and contributeCR 702.26b— a phased-out permanent's symbols drop out of devotion (honored by the reusedcount_devotion)CR 109.3— conjunction rule for theAndarm of the gate walkEvery number grep-verified against
docs/MagicCompRules.txtbefore it was written.Performance
verdict()runs per candidate per search node. The card-local check — pips in the candidate's own mana cost, plus a permanent-type check — runs first and rejects every non-permanent and off-color cast. Only a confirmed primary-color permanent pays forcount_devotion, one pass over the AI's battlefield permanents. No affordability sweep, nofind_legal_targets.activation()opts the whole policy out below the commitment floor.Measurement —
cargo ai-gatewith a paired controlThose WARNs are pre-existing, not caused by this PR. I ran the identical gate on this branch's clean base (
22e77160d) 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). None of the three gate matchups is a devotion deck, soDevotionPolicy::activationreturnsNoneand the policy never fires — the control proves that empirically.I deliberately did not refresh the baseline — absorbing pre-existing drift into a baseline inside an unrelated feature PR would hide it. The suite baseline looks due for a refresh on its own commit.
Verification
cargo test -p phase-ai --lib— 1488 passed; 0 failed (11 feature + 7 policy tests new; the policy tests exercise everyverdictbranch — pip progress, god-activation crossing, below-threshold, off-color, and the not-a-permanent path — against a realPolicyContext)cargo clippy -p phase-ai --all-targets -- -D warnings— cleancargo fmt --all— cleancargo ai-gate— 0 FAIL, with the paired control aboveNotes
devotion_pip_progressanddevotion_god_activationare registered inUNTUNED_POLICY_PENALTY_FIELDSpending a paired-seed calibration.features/tests/andpolicies/tests/per the house convention (no#[cfg(test)]in source files).tribal/mana_ramp: devotion counts colored pips, not creatures of a type or mana sources — a five-Forest ramp deck has highmana_rampand zero devotion; a{B}{B}Gray Merchant deck is the reverse.Effectmagnitude) and a spell's own "costs {X} less where X is devotion" self-discount (already applied by the engine — no per-cast decision to make). Both only lowercommitment, never over-credit.Tier: Frontier
Summary by CodeRabbit