feat(phase-ai): add devotion deck-feature axis + DevotionPolicy - #6602
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}). Every DISTINCT threshold in the primary color is retained
(Vec<u32>), since each god turns on independently.
* scaling payoffs: QuantityRef::Devotion anywhere in an effect's magnitude,
reached via the engine's own Effect::count_expr authority (drains, damage,
token counts, draw counts, dynamic P/T) 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
(CR 110.4 permanent-type test via CoreType::is_permanent_type)
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.
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 god-
activation spike for every DevotionGE threshold the cast newly crosses (a cast
can flip several gods at once). 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 carry #[serde(default = ...)] backed by shared default
functions the Default impl also calls, so older policy_penalties artifacts that
omit them still deserialize; both are 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). Both only lower commitment.
Boundary with tribal/mana_ramp: devotion counts colored pips, not creatures of
a type or mana sources.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughAdds devotion deck feature detection, commitment scoring, and a registered tactical policy for permanent casts. The policy rewards primary-color pip progress and newly crossed god thresholds, with configurable penalties and backward-compatible tuning defaults. ChangesDevotion AI support
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant DeckFeaturesAnalyze
participant DevotionDetect
participant PolicyRegistry
participant DevotionPolicy
DeckFeaturesAnalyze->>DevotionDetect: scan deck card faces and permanent pips
DevotionDetect-->>DeckFeaturesAnalyze: return DevotionFeature
PolicyRegistry->>DevotionPolicy: route CastSpell decision
DevotionPolicy->>DevotionPolicy: count devotion and crossed thresholds
DevotionPolicy-->>PolicyRegistry: return PolicyVerdict
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 284-298: Update continuous_modification_quantity to remove the
wildcard _ => None arm and make its match exhaustive over
ContinuousModification. Preserve the existing QuantityExpr-returning variant
mappings so any newly added quantity-carrying variant causes a compile-time
match failure.
🪄 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: ca579350-d297-4305-a342-a3ff10e766d9
📒 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
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the detector omits part of its declared devotion-payoff class.
🔴 Blocker
[HIGH] Detect every production QuantityRef::Devotion payoff path, or narrow the feature and its claims. Evidence: crates/phase-ai/src/features/devotion.rs:10-11,21-24 explicitly includes Nykthos ramp and cost reductions, but crates/phase-ai/src/features/devotion.rs:248-253 limits detection to Effect::count_expr() and states that exactly those mana-production and cost-reduction reads are not reached. Why it matters: a deck whose devotion payoff is Nykthos (or a devotion-based self-discount) is classified as having no payoff, so its feature commitment stays at zero and the policy never activates. Suggested fix: extend the existing AST quantity authority/visitor to cover every production QuantityExpr carrier used by this feature, including the mana-production and cost-reduction carriers, and add regressions using the parsed card shapes.
[MED] Make continuous_modification_quantity exhaustive. Evidence: crates/phase-ai/src/features/devotion.rs:284-298 uses _ => None despite mirroring the engine quantity authority. Why it matters: a future dynamic ContinuousModification can silently be omitted from devotion detection. Suggested fix: enumerate the non-quantity variants so the compiler forces this scan to be reconsidered with every enum change.
Recommendation: request changes.
…odification scan Addresses matthewevans' CHANGES_REQUESTED on phase-rs#6602. [HIGH] Detect the mana-production devotion payoff path. `Effect::count_expr` returns None for `Effect::Mana` (a mana effect has no count/amount magnitude — its QuantityExpr lives in the ManaProduction), so a Nykthos / Nyx Lotus / Karametra's Acolyte deck ("add mana equal to your devotion") was classified as having no payoff, leaving commitment at zero and the policy inert. Detection now digs the count out of the ManaProduction via `mana_production_count`, enumerated without a wildcard over all 15 variants. The docstring no longer claims the cost-reduction self-discount carrier, which is genuinely not detected — the "known limitation" is narrowed to exactly that one shape (a StaticMode cost modifier the engine already applies, with no per-cast decision to build toward). Adds `nykthos_mana_production_is_a_payoff`. [MED] Make `continuous_modification_quantity` exhaustive. Replaced the `_ => None` wildcard with the full non-quantity variant list, mirroring the engine authority `game::quantity::continuous_modification_dynamic_quantity`, so a future dynamic ContinuousModification forces this devotion scan to be reconsidered. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
|
Both addressed in [HIGH] mana-production detection. You're right — I took the "detect it" half of your either/or for mana-production since Nykthos is the marquee card, and the "narrow the claims" half for cost-reduction: a spell's own "costs {X} less where X is your devotion" is a [MED] exhaustive Verification on |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: blocked — the previous fixes are present, but two current-head devotion-policy correctness gaps remain.
🔴 Blocker
[HIGH] Multi-color devotion gates collapse to one color. Evidence: crates/phase-ai/src/features/devotion.rs:127-155 expands each DevotionGE { colors } to individual colors and selects one primary_color; crates/phase-ai/src/policies/devotion.rs:73-112 evaluates only that color. Athreos (white+black) and Xenagos (red+green) use a single combined threshold. Why it matters: a multi-color cast can cross the combined threshold while this policy sees only one component and fails to value activation. Suggested fix: preserve each exact DevotionColors::Fixed set and calculate board/candidate contributions against it, including hybrid pips once; add Athreos/Xenagos threshold-crossing regressions.
[MED] Deck-time payoff detection excludes otherwise/modal branches. Evidence: crates/phase-ai/src/features/devotion.rs:235-238 calls collect_chain_effects, while crates/phase-ai/src/ability_chain.rs:27-45,86-90 defines AbilityScope::Potential specifically for deck-time classification. Why it matters: cards whose devotion payoff appears only in a branch are never classified as devotion payoffs. Suggested fix: use collect_scoped_effects(..., AbilityScope::Potential) and cover otherwise/modal paths.
Recommendation: request changes for the two structural gaps above.
…yoff branches Round-3 review (phase-rs#6602): two devotion-policy correctness gaps. [HIGH] Multi-color devotion gates collapsed to a single color, so a two-color god (Athreos W+B, Xenagos R+G) — which gates on COMBINED devotion to both colors (CR 700.5) — was scored against only one component and never valued its activation. - DevotionFeature.primary_color: Option<ManaColor> -> primary_colors: Vec<ManaColor>; thresholds: Vec<u32> -> gates: Vec<DevotionGate>, each gate keyed on its exact DevotionColors::Fixed set. - detect() tracks whole color-set demands, normalized to WUBRG order, and picks primary_colors as the demanded set with the densest pips. - new cost_devotion_pips() counts a candidate's pips toward a color set, hybrids once (contributes_to) — the single-cost analogue of the engine's count_devotion. - DevotionPolicy evaluates every gate against its own combined set: count_devotion(&gate.colors) vs gate.threshold + cost_devotion_pips. - Athreos/Xenagos threshold-crossing regressions at feature + policy. [MED] Deck-time payoff detection now walks otherwise/modal branches via collect_scoped_effects(ability, AbilityScope::Potential), so a payoff living only in one mode is still classified (mirrors the poison scan). Tests: 27 devotion + full 1499-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Both round-3 blockers are fixed at head [HIGH] Multi-color devotion gates no longer collapse to one colorThe feature now preserves each
The policy evaluates every gate against its own combined color set: let current = count_devotion(ctx.state, ctx.ai_player, &gate.colors); // combined board devotion
let added = cost_devotion_pips(&obj.mana_cost, &gate.colors); // combined candidate pips
current < gate.threshold && current + added >= gate.thresholdHybrid pips are counted once via Regressions added:
[MED] Modal / otherwise branches now covered
for effect in collect_scoped_effects(ability, AbilityScope::Potential) { ... }so a payoff living only in one mode of a modal ability (or an Verification
|
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/devotion.rs`:
- Around line 193-197: Update the candidate scoring and pip_count calculation
around primary_colors to use set-wise devotion totals from
cost_devotion_pips(&face.mana_cost, set), multiplying by each entry’s count,
instead of summing per-color pip_totals. Ensure hybrid symbols contribute once
when evaluating multi-color sets while preserving existing candidate selection
and commitment 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: a9271fd4-c67c-449d-b987-e6dfd450c5f2
📒 Files selected for processing (4)
crates/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/policies/devotion.rs
- crates/phase-ai/src/features/tests/devotion.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: blocked — exact devotion gate sets are preserved, but deck calibration still double-counts hybrid pips for a multi-color primary set.
🔴 Blocker
[MED] Multi-color candidate totals sum per-color buckets instead of counting the color set once. Evidence: crates/phase-ai/src/features/devotion.rs:145-150 records each permanent's pips with count_colored_pips(Some(color)), then :193-197 chooses a multi-color primary set and sums those buckets for both candidate selection and pip_count. A {W/B} shard is therefore counted in both the white and black buckets, then counted twice for a W+B set. cost_devotion_pips at :93-108 and the engine's game/devotion.rs:7-28 establish the required set-wise one-symbol behavior. Why it matters: hybrid-heavy W+B (and other multi-color) decks receive inflated pip_count and commitment, which can over-activate the policy. Suggested fix: calculate every candidate set's deck total set-wise across permanent faces with cost_devotion_pips(&face.mana_cost, set) * entry.count, use that same total for primary selection and pip_count, and add a W/B combined-set regression proving each hybrid shard contributes exactly once.
Recommendation: request changes for the hybrid-safe candidate-set accounting gap.
… a color set Round-4 review (phase-rs#6602): the deck-calibration scan built per-color pip buckets with count_colored_pips(Some(color)) and then summed those buckets for a multi-color primary set. A hybrid {W/B} shard lands in both the white and black buckets, so a W+B set counted it twice — inflating pip_count and commitment and over-activating the policy on hybrid-heavy multi-color devotion decks. CR 700.5 counts each symbol once per set. detect() now keeps each permanent face's mana cost and computes every candidate set's deck total SET-WISE via cost_devotion_pips(cost, set) * count — the same one-symbol-per-set counting count_devotion performs at runtime — for both primary-set selection and pip_count. The per-color ColorTotals accumulator is removed. Mono-color totals are unchanged (a single-color set counts the same shards either way); only multi-color hybrid accounting is corrected. Adds hybrid_pips_count_once_for_a_combined_set: an Athreos W+B deck with four {W/B}{W/B} bodies must read pip_count 10 (2 god + 4×2), not the 18 a per-color-bucket sum would report. Tests: 28 devotion + full 1500-test phase-ai lib suite pass; clippy -p phase-ai --all-targets -D warnings clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Fixed at head [MED] Hybrid pips now counted once per color setYou're right — the deck scan built per-color pip buckets (
let set_total = |set: &[ManaColor]| -> u32 {
permanent_costs
.iter()
.map(|&(cost, count)| cost_devotion_pips(cost, set).saturating_mul(count))
.sum()
};
let primary_colors = candidates.into_iter().max_by_key(|set| set_total(set)).unwrap_or_default();
let pip_count = set_total(&primary_colors);Both primary-set selection and Regression added — Verification
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on the current head. Exact multi-color gates, modal/otherwise payoff discovery, and set-wise hybrid-pip accounting are covered by current-head regressions; merge when required checks complete.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Fresh submission of the devotion axis from current
main. The prior PR (#6590) was closed only for a missing canonicalModel:line (the #6584 gate landed shortly before it opened); this body carries both declarations. The three review blockers matthewevans raised on #6590 — serde defaults for persisted-artifact compatibility, retaining every distinct devotion threshold, and reusing the engine permanent-type authority — are already incorporated here.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. Every distinct threshold in the primary color is retained (Vec<u32>), since each god turns on independentlyQuantityRef::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 only (CoreType::is_permanent_type, CR 110.4)primary_coloris the deck's most-devoted color among the colors its payoffs read; aChosenColorpayoff (Nykthos) makes every color eligible, so the deck's densest color wins.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
CastSpellof a permanent by primary-color pips added, with a god-activation spike for everyDevotionGEthreshold the cast newly crosses — a cast can flip several gods at once (surfaced as thegods_activatedfact).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 110.4— the six permanent types (reusedCoreType::is_permanent_type)CR 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.txt.Implementation method (required)
Method: /add-ai-feature-policy
Track
Developer
LLM
Model: claude-opus-4-8
Thinking: high
Performance
verdict()runs per candidate per search node. The card-local check — 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.Verification
cargo test -p phase-ai --lib— 1494 passed; 0 failed (11 feature + 8 policy tests, including a pre-devotion persisted-artifact test and everyverdictbranch — pip progress, single- and double-god-activation crossings, 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— on the prior submission this was 0 FAIL, and a paired control on the same base produced a byte-identical report (none of the gate matchups is a devotion deck, soDevotionPolicy::activationreturnsNoneand the policy never fires). CI's paired-seed AI gate will re-confirm on this head.Notes
devotion_pip_progressanddevotion_god_activationcarry#[serde(default = "...")]backed by shared default functions theDefaultimpl also calls, and are registeredUNTUNEDpending a paired-seed calibration.features/tests/andpolicies/tests/per the house convention.tribal/mana_ramp: devotion counts colored pips, not creatures of a type or mana sources.Effectmagnitude) and a spell's own "costs {X} less where X is devotion" self-discount (already applied by the engine). Both only lowercommitment.Summary by CodeRabbit