Skip to content

feat(phase-ai): add devotion deck-feature axis + DevotionPolicy#6590

Closed
minion1227 wants to merge 3 commits into
phase-rs:mainfrom
minion1227:minion_ai_devotion
Closed

feat(phase-ai): add devotion deck-feature axis + DevotionPolicy#6590
minion1227 wants to merge 3 commits into
phase-rs:mainfrom
minion1227:minion_ai_devotion

Conversation

@minion1227

@minion1227 minion1227 commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Summary

Adds a devotion deck-feature axis (DevotionFeature) and its companion DevotionPolicy — 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 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.

Three structural detection axes, no card-name matching:

Axis AST surface
threshold payoffs (gods) StaticCondition::DevotionGE { colors, threshold }, walked through the Not/And/Or tree (Erebos gates "isn't a creature" on Not{DevotionGE})
scaling payoffs QuantityRef::Devotion in an effect magnitude, reached via the engine's own Effect::count_expr authority — so drains, damage, token/draw counts and dynamic P/T are all covered without hand-listing effect variants
pip density ManaCost::count_colored_pips, the single CR 700.5 counting authority (hybrid {G/W}{G/W} = 2), over permanent faces only

Two design points worth a reviewer's eye:

  • 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).
  • Reused authorities, not reimplementations: Effect::count_expr (magnitude), ManaCost::count_colored_pips (deck/cast pips), and engine::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 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, a multi-card swing rather than a marginal +1 (the "last missing piece" structure graveyard_types uses 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.rs

CR references

  • CR 700.5 — devotion is the count of a color's mana symbols among your permanents
  • CR 205.2a — only permanents can enter the battlefield and contribute
  • CR 702.26b — a phased-out permanent's symbols drop out of devotion (honored by the reused count_devotion)
  • CR 109.3 — conjunction rule for the And arm of the gate walk

Every number grep-verified against docs/MagicCompRules.txt before 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 for count_devotion, one pass over the AI's battlefield permanents. No affordability sweep, no find_legal_targets. activation() opts the whole policy out below the commitment floor.

Measurement — cargo ai-gate with a paired control

| matchup             | baseline p0% | current p0% | flips W→L | flips L→W | sign p | status |
| affinity-mirror     | 20%          | 40%         | 0         | 0         | —      | WARN   |
| enchantress-mirror  | 50%          | 40%         | 3         | 2         | 0.3438 | WARN   |
| red-mirror          | 70%          | 30%         | 5         | 1         | 0.0625 | WARN   |
compare: 0 FAIL, 3 WARN, 0 PASS, 0 NEW, 0 REMOVED

Those 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.json was last refreshed 2026-06-17 (#3492). None of the three gate matchups is a devotion deck, so DevotionPolicy::activation returns None and 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 --lib1488 passed; 0 failed (11 feature + 7 policy tests new; the policy tests exercise every verdict branch — pip progress, god-activation crossing, below-threshold, off-color, and the not-a-permanent path — against a real PolicyContext)
  • cargo clippy -p phase-ai --all-targets -- -D warnings — clean
  • cargo fmt --all — clean
  • cargo ai-gate — 0 FAIL, with the paired control above

Notes

  • Both devotion_pip_progress and devotion_god_activation are registered in UNTUNED_POLICY_PENALTY_FIELDS pending a paired-seed calibration.
  • Tests live in features/tests/ and policies/tests/ per the house convention (no #[cfg(test)] in source files).
  • 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.
  • Known safe undercounts (documented in-module): 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, never over-credit.

Tier: Frontier

Summary by CodeRabbit

  • New Features
    • Added deck devotion analysis, including devotion-gated payoff detection, primary color selection, and commitment scoring with activation thresholds.
    • Introduced a new AI tactical policy that uses devotion pip-density to guide casting decisions and trigger threshold-based “activation” behavior.
    • Registered the new devotion policy in the AI policy set.
  • Bug Fixes
    • Improved configuration compatibility by adding new devotion tuning settings with safe defaults for older saved artifacts.
  • Tests
    • Added extensive unit tests covering devotion detection, commitment gating, pip progression, threshold crossings (including multiple activations), and instant/non-devotion edge cases.

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>
@minion1227
minion1227 requested a review from matthewevans as a code owner July 24, 2026 08:31
@coderabbitai

coderabbitai Bot commented Jul 24, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

📝 Walkthrough

Walkthrough

Adds CR 700.5 devotion deck analysis, exposes devotion data through DeckFeatures, and introduces a registered tactical policy that scores primary-color permanent casts by pip progress or devotion-threshold activation.

Changes

Devotion analysis and policy

Layer / File(s) Summary
Devotion feature detection and aggregation
crates/phase-ai/src/features/devotion.rs, crates/phase-ai/src/features/mod.rs, crates/phase-ai/src/features/tests/*
Detects devotion payoffs, thresholds, scaling references, permanent colored pips, primary color, and commitment, then stores the result in DeckFeatures with structural and calibration tests.
Devotion policy and registry integration
crates/phase-ai/src/config.rs, crates/phase-ai/src/policies/*, crates/phase-ai/src/policies/tests/*
Adds devotion penalty defaults and compatibility handling, implements threshold-aware scoring for primary-color permanent casts, registers DevotionPolicy, and tests activation and verdict behavior.

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
Loading

Suggested labels: enhancement

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 68.18% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly summarizes the main change: a new devotion feature axis and DevotionPolicy in phase-ai.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Comment @coderabbitai help to get the list of available commands.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

🧹 Nitpick comments (1)
crates/phase-ai/src/features/devotion.rs (1)

67-71: 🎯 Functional Correctness | 🔵 Trivial

highest_threshold collapses multiple DevotionGE thresholds in the same color to a single max.

ColorThresholds::raise (Line 364-374) only keeps current.max(threshold), and DevotionFeature.highest_threshold exposes just that one value. If a primary color ever had two DevotionGE gates at different thresholds (the engine's StaticCondition::DevotionGE allows any u32), 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

📥 Commits

Reviewing files that changed from the base of the PR and between 2975e41 and 47ca1da.

📒 Files selected for processing (10)
  • crates/phase-ai/src/config.rs
  • crates/phase-ai/src/features/devotion.rs
  • crates/phase-ai/src/features/mod.rs
  • crates/phase-ai/src/features/tests/devotion.rs
  • crates/phase-ai/src/features/tests/mod.rs
  • crates/phase-ai/src/policies/devotion.rs
  • crates/phase-ai/src/policies/mod.rs
  • crates/phase-ai/src/policies/registry.rs
  • crates/phase-ai/src/policies/tests/devotion.rs
  • crates/phase-ai/src/policies/tests/mod.rs

Comment thread crates/phase-ai/src/config.rs
Comment thread crates/phase-ai/src/features/devotion.rs Outdated

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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>
@minion1227

Copy link
Copy Markdown
Contributor Author

Thanks — all three addressed in bf9be8997.

[HIGH] persisted-artifact compatibility. Both new penalties now carry #[serde(default = "default_devotion_pip_progress")] / ..._god_activation backed by shared default_* functions that the Default impl also calls (one source of truth). Added policy_penalties_load_pre_devotion_artifact, mirroring the pre-poison / pre-graveyard tests: it strips both fields from a serialized artifact, inserts a tuned wasted_cast_penalty, and asserts the round-trip preserves the tuned value and falls back to the shared defaults. This was a genuine regression on my part — the same finding you raised on the poison and graveyard axes; I should have carried it forward.

[MED] distinct thresholds. DevotionFeature now stores 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 doesn't reach also exists — and a cast clearing several gates at once activates all of them (surfaced as the gods_activated fact). Two new tests pin it: verdict_crossing_lower_threshold_activates_that_god (gods at 3 and 5, devotion 2 → the +1 that reaches 3 is now a god activation, not mere pip progress) and verdict_crossing_two_thresholds_activates_both.

[LOW] permanent-type authority. Both the feature and the policy now call CoreType::is_permanent_type (which already owns the distinction and cites CR 110.4) instead of re-implementing the six-arm match, and the annotations cite CR 110.4 rather than the incorrect CR 205.2a.

Verification on bf9be8997: cargo test -p phase-ai --lib — 1492 passed / 0 failed (22 devotion tests); cargo clippy -p phase-ai --all-targets -- -D warnings clean; cargo fmt clean. The logic change is gated behind activation (devotion floor), so it can't affect the non-devotion gate matchups — CI's paired-seed AI gate will confirm.

@coderabbitai coderabbitai Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🧹 Nitpick comments (1)
crates/phase-ai/src/features/devotion.rs (1)

341-348: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win

ColorThresholds re-implements the ColorTotals per-color-storage building block.

Both structs are the same shape: five named ManaColor fields plus slot/slot_ref exhaustive matches. Per crates path instructions, a new per-color container should compose the existing one rather than re-deriving it. Consider a small generic PerColor<T> (or an array indexed by a ManaColor -> usize mapping) 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 composing std primitives 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

📥 Commits

Reviewing files that changed from the base of the PR and between 47ca1da and bf9be89.

📒 Files selected for processing (5)
  • crates/phase-ai/src/config.rs
  • crates/phase-ai/src/features/devotion.rs
  • crates/phase-ai/src/features/tests/devotion.rs
  • crates/phase-ai/src/policies/devotion.rs
  • crates/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

Comment on lines +335 to +362
/// 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
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 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.

Suggested change
/// 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.

@matthewevans

Copy link
Copy Markdown
Member

Closed without implementation-diff review. The canonical model-declaration gate was published in b2628643 at 2026-07-24T04:42:30Z, before this PR was opened. The body declares Tier: Frontier but has no required canonical Model: <exact Frontier model> line; Tier alone cannot establish model eligibility. Please open a fresh PR from current main with both canonical declarations after completing the applicable contributor gates.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants