fix(phase-ai): repair AI land and mana decision-making across nine units - #6745
Merged
Conversation
…cEDH Community report: the AI mulliganed down to 2-3 cards. Only cEDH and Momir decks had any floor at all, so an ordinary deck had none -- KeepablesByLandCount was the sole unbounded ForceMulligan source, and its own hand_size <= 4 stop condition was dead code (under the London Mulligan the engine always presents a 7-card hand at the keep decision, because bottoming happens after). Lifts the floor out of CedhKeepablesMulligan into a universal MulliganCardFloor policy: one authority, not an archetype special case. Retires CEDH_MULLIGAN_FLOOR and the vestigial +5.0 short-hand branch, whose production domain is exactly the floor's band in both free-first regimes -- no gap, no overlap -- so the deletion cannot change a verdict, and ForceKeep strictly dominates the Score it replaced. The floor is an AI strategy heuristic, NOT a rules requirement. CR 103.5 permits mulliganing until the opening hand would be zero cards; CR 103.5c is only the free-first discount. Corrects an over-claiming annotation that said otherwise, and an engine doc comment that said the hard cap was a 1-card hand when the file's own tests show it is 0. The reporter's inferred cause (MDFC lands miscounted as spells) was refuted -- 200/200 modal_dfc entries carry two faces sharing one oracle id, and the live oracle-id fallback satisfies that. The observation was right; the cause was not. ai-gate is structurally two-player 60-card and never reaches a 4th mulligan, so it shows no harm but carries no positive evidence; the unit tests are the whole sufficiency argument. baselines/suite-baseline.json deliberately not refreshed -- concurrent units also move AI behaviour and a shared refresh would destroy per-unit attribution.
…ocstring 286b4c64ea shipped a docstring asserting "no registered policy and no part of evaluate_hand reads card names". That is false: five registered policies read obj.name in production -- landfall_keepables.rs:63, aristocrats:75, plus_one_counters:75, spellslinger:88, tokens_wide:69 and :79, all outside cfg(test) and all in MulliganRegistry::default()'s vec. The conclusion the sentence supported still holds, on a narrower ground that nobody had established: DeckFeatures derives Default and features/ carries no manual impl Default, so every *_names Vec<String> is empty under DeckFeatures::default() and no name comparison can match. That is a guarantee about this fixture's configuration, not about the policy architecture -- and it breaks the moment someone writes a test on non-default DeckFeatures. The docstring now says which guarantee it is relying on. Comment-only; no behaviour change.
…-time offset
Community reports: the AI does not play its lands on curve ("played against
modern tron and it kept just not playing its lands"), and discards lands while
holding one land on the battlefield. Root cause at the eval layer: a land drop's
weighted delta is strictly negative for every shipped archetype. The card
leaving hand costs `hand_size` (serve-time value up to 2.5 x 2.5 = 6.25 for
Combo in the late phase), plus its `zone_quality` hand-castability term and
`synergy` dilution, for a binding total of 6.5545 -- and nothing on the
battlefield side pays for the mana source the card becomes.
Adds `mana_development_offset`: `f(n) = MANA_DEVELOPMENT_COEFF * min(n,
MANA_SOURCE_TARGET)`, with C = 7.5 and S = 12. The coefficient is fixed rather
than learned, mirroring the existing `energy_offset` pattern: it is applied
AFTER weighting in `evaluate_state_breakdown` and is excluded from
`EvalFeatures::weighted_total`, so the Texel-fitted weights and the train/serve
invariant are preserved and no retrain is required.
`mana_source_count` counts renewable sources only (CR 106.1), delegating to
`zone_eval::is_intrinsic_mana_source` so that cracking a Treasure, Gold or
Lotus Petal (CR 701.21) does not read as losing a mana source.
Two consequences are disclosed in the constants' own docstrings rather than
left for a maintainer to rediscover, and both are pinned by assertions:
- The offset is archetype-invariant, so every archetype receives exactly +7.5
per source and only the counterweight differs. Control's rock-vs-body margin
inverts (-3.23 -> +4.27); Midrange, the `#[default]`, does not invert but
lands 0.33 away. Combo and Ramp are UNMEASURED, not safe.
- At and above the source cap the marginal is exactly 0.0 while the land-drop
cost is unchanged, so the reported bug RETURNS in full. Reachable in
Commander from roughly turn 10 with 12 sources out and two or more lands in
hand. Closing that is the root fix and is deliberately out of scope here;
raising the cap trades the flooding defect back in.
`mana_development_floor_holds_for_every_archetype_and_phase` recomputes the
floor from the real weights and pins both poles, so a retrain or a multiplier
change fails loudly instead of silently invalidating the constant.
The `effect_kind` sweep lines that a concurrent agent's `TargetSelectionSlot`
work required are deliberately NOT in this commit; that feature lands
separately and this commit builds against `main` without it.
Verification: `tilt` test-ai 1766/1766 passed, 14 skipped; clippy
`--all-targets -D warnings` plus `check-interaction-bindings.sh` clean.
The AI pitched lands and mana rocks at cleanup and fed its own accelerants to sacrifice outlets, because both seams ordered candidates by a single scalar `evaluate_card_value` that carried no notion of what a card does for mana development. A land and a junk instant of equal raw value were interchangeable. Introduces `card_value` as the single authority for that ordering: - `ManaRole` / `mana_role` classify a card's contribution to mana development (LandDrop, Accelerant, None), and `KeepTier` / `keep_tier` rank cards for surrender against the controller's own plan schedule. - `keep_key` + `cmp_keep` replace bare `evaluate_card_value` sorts at the discard seams; `sacrifice_key` + `cmp_sacrifice` do the same for sacrifice selection; `cost_card_value` prices `PayCostKind` arms through one authority rather than per-arm ad hoc formulas. - The tier is keyed on the DISCARDING player and their board, not the AI's, and reverses under CR 723.5 when the AI is the authorized submitter for another seat. - `fallback_action` now takes `&AiConfig` so penalties reach the seam; `fallback_action_default` preserves the previous call shape in tests. When no plan snapshot is available every card is Ordinary and the tuple comparator degenerates exactly to the previous scalar ordering, so the change is inert wherever the plan is absent. Three tests pin that fail-safe. Behavioural constants that a paired-seed `ai-gate` run must attribute correctly: - `sacrifice_land_penalty` 4.0 -> 4.5. This is a change to a pre-existing CMA-ES-trained default, NOT a consequence of the band rescale. A run that omits it will misattribute the delta. - `SACRIFICE_VALUE_RAW_CEILING` = 30.0, derived from the p99.9 of measured sacrifice-selection raw scores (p50 6.0 / p90 12.5 / p95 15.0 / p99 20.0). - `GUARDED_SELECTION_CARDINALITY` = 7. CR corrections made in passing: three sites cited CR 305.7 (which governs subtype-SETTING effects) for the proposition that a creature-land is a creature and a land simultaneously. That proposition is CR 300.2; the citations now point there. Two CR 305.7 sites that genuinely quote the subtype-setting rule are kept.
…stroy The AI would pay a spell's sacrifice cost with whatever fodder was eligible, including the mana sources it still needed — feeding artifact lands to Fanatical Offering, Reckoner's Bargain and Lava Dart while behind on mana. Nothing in the candidate scoring asked whether committing to the cost was worth what the payment would cost the mana base. Adds `SacrificeCostManaGatePolicy`, which rejects a candidate whose required sacrifice would consume a mana source the controller's own plan still needs. It reads the same `KeepTier` / `ManaRole` authority introduced for discard and sacrifice ordering, so the two compose cleanly: the gate decides whether to commit to the cost at all, the tier decides which fodder is surrendered once committed. The eligibility computation is NOT reimplemented on the AI side. The policy calls `casting::find_eligible_sacrifice_targets` + `casting::sacrifice_cost_bounds` — the same pair, in the same order, that `cost_payability.rs` uses on the real payment path. Both are widened to `pub` for this; `casting` already exports three other `pub` fns consumed by phase-ai. An AI-side copy would have to re-derive the `player_cant_sacrifice_as_cost` static check and would over-count fodder under Yasharn or Angel of Jubilation, which is precisely the divergence the engine owning this logic prevents. Three test helpers move verbatim from `search.rs` into a new `test_support` module. Scope of the evidence, stated because a green run here does not mean what it looks like: `SacrificeCostManaGatePolicy` is unreachable in the ai-gate duel suite. Of the 120 cards in `duel_suite/inline_decks.rs`, zero carry a gated cost shape (measured, with Fanatical Offering / Lava Dart / Dread Return as positive controls), so the suite's exit-0 is evidence the gate is inert there, not that it is safe. The 17 unit tests are the whole of its coverage; F6 drives the production aggregation through `PolicyRegistry::shared().priors(..)`. The flashback-arm `at_root()` perf contingency is untriggered and unmeasured: the ai-gate report emits no wall-clock figures and the suite contains no flashback card. Recorded as an open risk rather than a pass. Candidate generation already walks `effective_flashback_cost` once or twice before a graveyard candidate can exist, so the added walk is amortized, but that is an argument and not a measurement.
…lute count
Unit 1 gave the evaluator a fixed-coefficient mana-development offset keyed on
the AI's own source count. That closed the "AI ignores its mana base" half of the
reported problem and opened a narrower one: because the term took no opponent
input, it saturated on an absolute count. At MANA_SOURCE_TARGET sources — an
ordinary turn-10 Commander board, not a corner case — an additional source
scored exactly zero marginal, so a land drop cost the AI a card for nothing and
the evaluator preferred to keep the card.
The offset is now signed and relative:
f(d) = C * clamp(d, -S, +S) d = self_sources - opponent_aggregate
Marginal is exactly C for |d| < S and exactly 0.0 beyond, so unilateral
development still cannot be farmed — but it saturates on the RELATIVE axis. At
12-vs-12 sources a land drop moves d from 0 to 1 and earns the full C, which is
the regression Unit 1 disclosed and this closes. An opponent ahead on mana now
yields a negative term rather than the previous unsigned reading of zero.
With one opponent the aggregate is a plain average. With two or more it is
threat-weighted (sum of w_i * sources_i, w_i from threat_level), matching how
card_advantage_breakdown already aggregates the other board_stats-derived count.
board_stats returns a BoardStats struct rather than a 4-tuple. The four fields
are all i32, so the previous positional reads (.1 for power) were a transposition
waiting to happen; call sites now name what they want and the compiler checks
them. plan/mod.rs::controlled_mana_sources was a deliberate byte-identical copy
of the then-private eval::mana_source_count, carrying a comment instructing a
future commit to dedup it. That instruction is honored here: the body delegates
to board_stats(..).mana_sources and the comment records the discharge.
Scope of the evidence, stated because a green gate here does not mean what it
looks like:
- cargo ai-gate is exit 0 with 0 FAIL / 3 WARN, and the WARNs are NOT readable as
this change's effect. suite-baseline.json predates Unit 1, so every delta is
confounded across all five units plus concurrent work on main. All three
matchups are mirrors, where 50% is the null and n=10 has wide variance; neither
sign test is significant (p = 0.34, p = 0.14). The single baseline refresh is
deliberately deferred so it happens once, after this lands.
- MatchupSpec has exactly two seats, so the gate exercises the averaged
<=1-opponent branch ONLY. It structurally cannot reach the threat-weighted
branch or the threat-weight channel below, both guarded on
opponents.len() >= 2, nor the Commander regime this unit was written for.
Their only coverage is unit tests. A green gate is necessary, not sufficient.
Two accepted costs, disclosed rather than fixed:
- R8, the threat-weight channel. In the threat-weighted branch a candidate that
changes an opponent's threat level moves the mana term with no mana source
changing hands, and the normalization is sign-inverting: lowering a mana-rich
seat's threat raises the evaluator's mana score, lowering a mana-poor seat's
threat lowers it. card_advantage_breakdown has the identical channel, so this
is the house pattern rather than a new exposure, but the magnitude is larger
because C carries no fitted weight. Pinned by
threat_reweighting_moves_the_mana_term_without_a_source_changing_hands. The
root fix (teach threat_level about mana development) is circular with the
aggregate it would feed and is larger than this unit.
- R6, mutual saturation. A 13-source lead returns the marginal to 0.0, so the
leader's land drop scores negative. Far rarer than the absolute-form cap it
replaces, which triggered at 12 sources outright.
HarvestMeta.schema goes 2 -> 3. The bump is provenance only: the column keeps its
name across the semantic change, so train_eval_weights.py's defaulted_rows
warning does not fire on a mixed pre/post corpus and this is not a pooling guard.
Recorded so a human reading a shard's meta line can tell which semantics it
carries; the real repair is a column rename or a trainer-side minimum-schema
check.
Discrimination was demonstrated by applying three mutants to the real tree,
compiling, and recording which rows went red — not asserted. Mutant A, the
half-migration clamp(d, 0.0, S), reds a different row set than Mutant B, the true
revert, which is what distinguishes the two failure modes.
Notion Thief's only coverage was a parser assertion that `execute.is_some()`.
That is true of any non-empty chain — including one whose no-op head never
reaches its `sub_ability` — so it proved the parser produced something and
nothing about whether anyone draws. The card's real behavior was unverified in
both halves.
The card parses into a two-link chain: the head is
`Effect::Unimplemented { name: "draw" }` for "that player skips that draw", and
`sub_ability` is `Effect::Draw { count: Fixed(1), target: Controller }` for "and
you draw a card". The head being non-`Draw` is exactly what makes
`draw_is_substituted_away` zero the opponent's draw count, so the `Unimplemented`
is load-bearing rather than debt — replacing it with `Effect::Draw` would stop
the pre-zeroing and produce a double draw. Nothing recorded that, and nothing
tested it, so the annotation was one plausible cleanup away from a regression.
The open question was whether `resolve_ability_chain` continues past the no-op
head into `sub_ability`. It does. Answered by execution, not by reading:
the controller's hand goes +1 off their own library while the opponent's hand and
library are untouched.
Four tests, in `tests/integration/` so they share the existing binary:
- a no-Notion-Thief control, so the redirect test's deltas cannot come from a
draw that never happened;
- the redirect itself, asserting both halves independently and asserting its own
preconditions (phase is not Draw, active player is the controller) so it is
provably outside the card's excluded window;
- both directions of the `ExceptFirstDrawInDrawStep` gate in one turn — first
draw exempt, second draw in the same step redirected;
- a mutation probe that severs `sub_ability` on the live object and pins the
mutant world (opponent still loses the draw, controller gains nothing),
asserting its own removal took effect so it cannot pass vacuously.
The probe is a test rather than a transient revert because this is a shared
checkout — a deliberate engine mutation would have shown other agents a red
`test-engine` on a tree they are also building. Keeping it as a fourth test makes
the discrimination permanent instead of momentary.
CR 614.1a: "instead" makes this a replacement effect. CR 614.6: the replaced draw
never happens. CR 121.1 + CR 504.1: the exception is the active player's
turn-based draw, which the card must leave alone.
…ng a tapped body as cheaper to give up
The AI fed its whole board to Baron Bertram Graywater — `{1}{B}, Sacrifice
another creature or artifact: Draw a card` — one permanent at a time until
nothing expendable was left, then took its commander. Two defects sat on top of
each other, and neither was the one the code appeared to have.
SelfCostValuePolicy computed the price of the permanent about to be surrendered
and then discarded it whenever the payoff was non-trivial. The gate was binary,
never a comparison, and a draw is unconditionally non-trivial — so "sacrifice a
12/12: draw one card" scored identically to "sacrifice a 1/1 token: draw one
card". Meanwhile FreeOutletActivationPolicy answered the same question a second
time, with a cliff at 4.0 against a cost scan that walked creatures only: blind
to the ability's artifact leg, blind to `Another`, and rewarding the drain at
+0.5 rather than merely failing to stop it.
The comparison now lives in one place. `appraise_benefit` walks the effect chain
once and returns a typed appraisal — trivial, priced, or explicitly unpriceable —
and the cost side binds through the filter-faithful give-up authority. The crude
duplicate is deleted; FreeOutletActivationPolicy is narrowed to what its name
says, scoring genuinely free outlets by death-trigger payoff and opting out
entirely below the aristocrats floor.
Two things measurement changed about the design, both of them load-bearing.
A graduated penalty does not restrain a sampler. The decision is a softmax
sample at temperature 1.0, and a batch runs ~100 priority windows. Pricing the
trade correctly halved the per-window probability of activating — 63.9% to 28.3%,
and the root argmax became Pass on both test boards — and the board drained
exactly as before, because across 100 windows any finite negative is near-certain
eventual selection while fodder remains. Only -inf is categorical. So the
underwater arm emits a Reject, and it does so with no threshold: it fires on the
sign of a fully-priced net, after eight exemption rungs have each declined to
stand the comparison down. There is no constant to tune, which is the difference
between this and the 4.0 cliff it replaces.
And the give-up price was inheriting a discount that does not apply to it.
`evaluate_creature` subtracts a tapped penalty — correctly, for board evaluation,
where a tapped creature cannot block or attack. `sacrifice_cost` called it and
inherited that discount, which asserts that sacrificing a tapped creature
destroys less permanent than sacrificing an untapped one. It does not. Attacking
tokens end tapped, so a 1/1 priced at 1.0 against a 1.0 draw, landed exactly on
the inclusive covers-cost boundary, and the entire drain flowed through an arm the
design had blessed — never reaching the underwater arm at all. A new
`evaluate_creature_intrinsic` prices the permanent, not the turn; board
evaluation keeps its discount, pinned by a negative control.
Measured, four fixture arms, both search states, against the recorded pre-fix
baseline of five activations and a lost commander:
tokens search on/off -> 0 activations, board intact, commander safe
3/3 search on/off -> 0 activations, board intact, commander safe
Clues search on/off -> all five cracked, then the drain stops at the
commander rather than at fodder exhaustion
The Clue arm is the one that matters most: it proves the veto does not leak into
trades that pay for themselves, and because Clues run out it drives the board to
the exact state where the commander is the cheapest legal target and shows the
economics declining it. Two reach guards keep it honest — every Clue must be
cracked, and the batch must end naturally rather than at the action cap, since
either shortfall would let the commander assertions pass without the boundary
ever being offered.
Scope of the evidence. `cargo ai-gate` is exit 0 with 0 FAIL, but its baseline
predates this campaign so the two WARNs are confounded across every unit plus
concurrent work, and MatchupSpec has exactly two seats — it cannot see the
Commander regime the report came from. The single suite-baseline refresh is
deliberately deferred to after this lands, since both changes here alter
sacrifice pricing and selection.
The commander is safe here because the trade is priced, not because anything
values a commander. Nothing in the AI does; a cheap commander would still sort as
ordinary fodder. That is tracked separately.
…mpaign
The previous baseline was taken on 2026-06-17, before the first unit of this
campaign landed. Every reading against it since has been confounded across six
AI behaviour changes plus unrelated concurrent work, which is why Unit 5's gate
run reported three WARNs that nobody could attribute to anything. Refreshing it
was deliberately deferred four times so it would happen exactly once, after the
last behaviour change landed.
Final comparison against the stale baseline before overwriting it — 0 FAIL,
2 WARN, 1 PASS:
affinity-mirror 20% -> 40% flips W->L 1, L->W 1 sign p = 0.5000 PASS
enchantress-mirror 50% -> 40% flips W->L 3, L->W 2 sign p = 0.3438 WARN
red-mirror 70% -> 40% flips W->L 5, L->W 2 sign p = 0.1445 WARN
Neither WARN is significant, and neither is attributable. All three matchups are
mirrors, where the null is 50% and n = 10 per matchup gives very wide variance.
Read red-mirror's 70% -> 40% carefully: 70% was further from the mirror null than
40% is, so the honest statement is that neither value is distinguishable from a
coin flip at this sample size, not that a regression occurred. Pooled across all
three, 12/30 against 0.5 is p ~ 0.36.
What the refresh buys is the thing this campaign never had: a reference point
where a delta means one unit's effect. Units B and C are queued and are both AI
behaviour changes; measured against the old baseline their results would have
been unreadable in exactly the way Unit 5's were.
What it does not buy, stated because a green suite reads like more than it is:
- MatchupSpec has exactly two seats, so the suite exercises only the averaged
<= 1-opponent branch. It cannot reach Unit 5's threat-weighted aggregate, its
disclosed threat-weight channel, or the Commander regime that three of the six
originating bug reports came from. That gap is tracked separately and is not
closable by any baseline.
- The refresh enshrines current behaviour as the reference. If a landed unit
regressed something the fixtures do not cover, this makes it invisible rather
than detectable. The six sacrifice-outlet arms landed with Unit A all measure
what they should, which bounds but does not eliminate that risk.
- Harvest shards recorded before and after Unit 5 pool without warning, since the
mana_development_offset column kept its name across a semantics change. Any
training corpus spanning that boundary needs the column renamed or a
trainer-side minimum-schema check first.
The AI fed permanents to Baron Bertram Graywater — `{1}{B}, Sacrifice another
creature or artifact: Draw a card` — while an opponent had Notion Thief out.
Every activation paid a permanent and {1}{B}, drew the AI nothing, and handed
the opponent a card. Unit A taught the policy to compare cost against benefit;
the comparison was still being fed a benefit that did not exist.
`effect_benefit_value` priced `Draw{Controller}` at count x one card
unconditionally. The engine already knows better: `can_draw_at_least_one`
answers whether a draw right now would actually put a card in a player's hand,
delegating each leg to the authority that owns it — `allowed_draw_count` for
`CantDraw` and exhausted per-turn limits, `select_cards_to_draw` for an empty
library, and `proposed_draw_survives_replacement`, which shares its
applicability and substitution classifiers with the live pipeline. It was built
for exactly this preflight and `DrawPayoffPolicy` already delegates wholly to
it. The Draw arm now consults it, and prices a removed draw at 0.0.
Priced zero, not `None`. `None` is `BenefitAppraisal::Unpriced`, which stands
the whole comparison down and ALLOWS the activation — the opposite of what the
engine has just certified. The zero is not a heuristic and cannot under-price a
real draw: an optional replacement is never assumed to apply.
The end-to-end arm is the part worth reading, because the plan for it was
falsified by measurement. Built as specified — Clue board, opposing thief,
assert zero activations — it failed post-fix at five. Instrumenting every
activation with the state it was decided in showed all five taken with
`can_draw_at_least_one == true` and the thief gone from the battlefield: it is
a 3/1 on a board that plays real combat, it dies partway through, and after
that cracking Clues for cards is correct play. The fixture had stopped
measuring its own premise. Granting indestructible was measured insufficient —
the opponent decks out before the run ends, plausibly accelerated by the cards
the thief steals — so a final-state guard is the wrong shape entirely.
The instrument is now per-decision. `suppressed_activations` counts cracks
taken in a window where the engine had already certified the payoff would not
arrive; that is the quantity this change governs, and it goes 5 of 5 pre-fix in
both search regimes to 0 post-fix. `suppressed_windows` — AI priority, fodder
still on board, payoff dead — is asserted `>= 1` so the zero cannot pass by the
opportunity never arising. The raw activation count is deliberately unasserted,
and the fixture says why: it mixes suppressed windows with the correct-play
windows after the suppressor leaves.
Unlike its thief-less control, this arm escapes the saturation ceiling recorded
with Unit A. A vetoed candidate has softmax weight exactly zero, so the
categorical zero is a prediction any single suppressed activation falsifies,
not a ceiling five fodder can reach by exhaustion.
A census of the crate was measured rather than read: the production edit alone
was applied and one full `test-ai` cycle read. Seven pre-existing tests went red
and four more were green on a premise that had become false — three and one
more than the most careful read had found. All eleven now build a seeded
library through one helper, so each stated premise is true rather than
annotated as false.
The deliverability check is a live predicate, and one row now pins that.
`draw_price_follows_the_suppressor_leaving_within_one_state` scores the same
source twice inside a single `GameState` — suppressor live, then suppressor
moved to the graveyard — through a helper that shares one `AiContext` and
`AiSession` across both verdicts, so a memo parked at decision, game or process
scope has somewhere to go stale. Both directions were watched go red rather than
predicted: a process-global memo reddens the row even before the fix, and a
session-scoped one is green before the fix and red after, which is the finding.
The one memo home the row cannot catch — a field on the policy value itself,
which production holds process-global through `PolicyRegistry::shared()` while
the row rebuilds it per call — is recorded at the row instead of left implied.
The per-window suppressor-liveness conjunct reads `obj.zone` against the same
zone set as the replacement candidacy gate; membership in the battlefield vector
alone is a no-op for that gate. Reading `obj.zone` is wider than the gate in one
direction, because the gate also excludes liminal sources, so the fixture
discloses the measured delta — zero here, since nothing on this board occupies
the command zone — rather than arguing the widening is harmless. The probe
numbers behind the conjunct are labelled by kind at the fixture: which are
measured, which follow from the types, and which were taken on a shrunk-library
probe build and so are not the shipped configuration's counts.
Scope of the evidence. `cargo ai-gate` is exit 0 with 0 FAIL and zero flips on
every matchup, which reads as no-regression and nothing more: the gate is
2-player and its decks field no draw suppression, so the new branch is never
taken there. The measuring instrument for this change is the fixture above.
Disclosed and unchanged: pricing is binary rather than count-aware, so a
`draw(3)` under a limit with headroom 1 still prices 3.0 — over-pricing, which
can let a marginal crack through but can never forbid one that pays. Cast-seam
draw bonuses (`card_advantage`, `etb_value`, `card_hints`,
`best_proactive_cast_score`) remain ungated; they are unreachable from the
activate path this fixes and are tracked as their own measured unit.
The AI fed its own commander to a sacrifice outlet. It was not a tie-break that
went badly — it was an exact tie. Measured before the change: an owned 4/4
commander and an ordinary 4/4 bear both price 10.0 through
`evaluate_creature_intrinsic` and produce identical `(Ordinary, 10.0)` sort keys,
so the softmax over `SelectCards` candidates gives each a probability of 0.5. The
policy had no way to tell them apart, because the quantity it measures does not
distinguish them.
A commander is the one permanent whose loss is recoverable. CR 903.9a lets its
owner move it to the command zone instead of the graveyard, and CR 903.8 charges
{2} for each previous time it was cast from there. So its replacement price is
what recasting costs — mana value plus the accumulated tax — and that is a
different quantity from its board presence, not a correction to it. The premium
is therefore added, never substituted: `GameObject::mana_value_on_battlefield_exit`
plus `commander_tax`, and the bear stays at 10.0 while the commander goes to 16.0.
The mana value has to come from the face the permanent will have once it leaves.
CR 712.8a gives a double-faced card only its front face outside the battlefield,
CR 708.2a gives a face-down permanent no mana cost at all, and CR 710.1c leaves a
flip card's cost unchanged. The engine already reverts those statuses on the way
out, so the method reads the same stash `zones::apply_zone_exit_cleanup` restores
from, gated per flag rather than on a single disjunction. It lives on `GameObject`
because it is a face rule that happens to have a commander as its first caller —
filing it under commander would name it after the card instead of the class. It is
documented battlefield-exit-only: one transition is proven, and a destination
parameter with one proven value is speculative generality.
The partition is the part worth reading. A repurchase price is only owed where the
permanent is actually surrendered, and five ledger rows were charging it for
permanents that are merely used: crew, station, `ReturnToHand`, `RemoveCounter`,
and `TapCreatures`. Tapping a commander to crew a Vehicle gives up nothing, and
CR 903.9b leaves a bounced commander command-zone-bound and in no case lost. So
`sacrifice_cost` is now a wrapper — `permanent_board_value` plus the premium — and
those five rows call `permanent_board_value` directly. The extracted body is the
previous `sacrifice_cost` byte for byte; no coefficient moved; the premium is added
in exactly one place. This is the distinction `payment_cost`'s own docstring
already drew in prose, promoted to a function boundary.
Two of those five are the ones that mattered least by size and most by effect. The
crew and station rows scale by 0.05, so the leak there is +0.30 against the +3.00
of `ReturnToHand` — but they are also the only two whose consumers return an
absolute score into `PolicyRegistry`'s sum rather than a comparison among sibling
candidates, and +0.30 lands exactly on `NUDGE_MAX`. Magnitude was the wrong axis;
what matters is whether the number ends up in a comparison or a total.
The seven-card guarantee in `SacrificeValuePolicy`'s docstring was falsified by
this change and is now a formula. The land guard holds while a selection's raw
magnitude stays under `SACRIFICE_VALUE_RAW_CEILING`, which with one owned commander
priced `C` means `ceil((34 - C) / 4) - 1` cards: five for a four-drop cast once,
three for a ten-drop at heavy tax. There is deliberately no single integer, and
the pin now measures the bound from a live `sacrifice_cost` call rather than
restating a constant.
Coverage sits at the seam the report came from. The rate assertion drives
`SacrificeValuePolicy::score` and `verdict` through a real `PolicyContext` over
`WaitingFor::PayCost { Sacrifice }`, and the comparable-body arm asserts the
outcome literally: the bear scores -10.0, the commander -16.0, and the selection
the AI gives up is the bear. `pick_lowest_value_sacrifices` is asserted in both
input orders, because a stable sort over an unbroken tie returns whichever element
came first and one order cannot distinguish an ordering from a lucky input. The
saturation arm compares a policy-computed -30.0 against the ceiling constant, so
raising that ceiling reddens it — which is the point, since raising it is the
anticipated follow-up.
Disclosed and unchanged. The premium is finite, so a body worth more than
`base + premium` still outranks the commander; giving up a recastable commander
rather than a large irreplaceable creature is defensible play and is documented
rather than asserted against. `blight_value` keeps the full price because a -1/-1
counter surrenders the permanent only at toughness <= 1 (CR 704.5f) — a partial
case that wants a branch-level split, tracked separately. Tempo is still unpriced,
a known direction-safe underestimate.
matthewevans
enabled auto-merge
July 28, 2026 20:52
Contributor
|
Warning Review limit reached
Next review available in: 17 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 (2)
📝 WalkthroughWalkthroughThe PR expands engine sacrifice and mana-source authorities, adds mana-development evaluation and plan-aware valuation, introduces sacrifice-cost and mulligan policies, revises self-cost appraisal, and updates search, regression, integration, baseline, and training-schema coverage. ChangesEngine and evaluation
Policies
Validation and data
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
Unit B's Notion Thief repro adds a fixture constructor that builds an opposing Notion Thief, which is a Draw replacement by construction — so it is a new producer and the frozen list has to say so. Plan 03 pins this surface because a Draw replacement's CR 121.2 scope is declared at construction and cannot be recovered afterwards, which is exactly why the gate demands a human look before re-freezing rather than regenerating on the fly. Reviewed: the regenerate is a pure single-line addition. No existing row moved, was reordered, or lost its scope, and the new row's required scope is 1 — the fixture replaces one player's draw, not a global one.
Parse changes introduced by this PR✓ No card-parse changes detected. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
scripts/train_eval_weights.py (1)
519-527: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winEmpty phase buckets still crash here
scripts/train_eval_weights.py:526
load_selfplay_corpus()has no localcolumns, and the onlycolumnsbinding is insideextract_selfplay_weights(). If any phase has zero rows, this branch raisesNameErrorbefore the later skip logic can run. Use the loader’s fixed width instead, e.g.len(SELFPLAY_FEATURE_NAMES) + len(SELFPLAY_CONTROLS).🤖 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 `@scripts/train_eval_weights.py` around lines 519 - 527, Fix the empty-phase allocation in load_selfplay_corpus by replacing the undefined columns reference in phase_features with the loader’s fixed feature width, using len(SELFPLAY_FEATURE_NAMES) + len(SELFPLAY_CONTROLS). Leave the non-empty conversion and later phase-skip behavior unchanged.
🧹 Nitpick comments (4)
crates/phase-ai/src/policies/payment_selection.rs (1)
445-450: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNew
_arms on a knownZoneenum silently price future zones.
ExileFromManaZoneandExileAggregateare zone-parameterized building blocks; the_ => cost_card_value(..) * 0.5fallback means a newly supported source zone (Command, Library, Stack) gets an arbitrary half-card price with no compile-time signal. Spell the remainingZonevariants out (grouping them if they share a price) so adding a variant is a compile error.As per coding guidelines: "wildcard
_match arms where the enum is known and an exhaustive match would let the compiler catch missing variants".Also applies to: 458-463
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/policies/payment_selection.rs` around lines 445 - 450, Replace the wildcard arms in the PayCostKind::ExileFromManaZone and ExileAggregate zone matches with explicit remaining Zone variants, grouping variants that share the existing half-card pricing. Preserve the current Battlefield, Hand, and Graveyard behavior while making future Zone additions require updating these exhaustive matches.Source: Coding guidelines
scripts/train_eval_weights.py (1)
589-590: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winPass
strict=Truetozipso a column/coefficient mismatch fails loudly.
columnsmust have exactly the width the loader produced; silent truncation would mis-label coefficients.♻️ Proposed change
- raw_coefs = {name: round(float(coef), 6) for name, coef in zip(columns, model.coef_[0])} + raw_coefs = { + name: round(float(coef), 6) + for name, coef in zip(columns, model.coef_[0], strict=True) + }🤖 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 `@scripts/train_eval_weights.py` around lines 589 - 590, Update the raw_coefs construction in the model coefficient export flow to call zip with strict=True, ensuring SELFPLAY_FEATURE_NAMES + SELFPLAY_CONTROLS and model.coef_[0] have identical lengths and mismatches raise immediately instead of truncating silently.Source: Linters/SAST tools
crates/phase-ai/src/card_value.rs (1)
119-141: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win
mana_roleandmana_behindmeasure different populations, so a one-shot source can be promoted by a deficit it can never close.
mana_rolepromotes onis_mana_ability, while themana_behinddeficitkeep_tiercompares against comes fromplan::controlled_mana_sources→is_intrinsic_mana_source, which excludes self-sacrificing one-shots. A Treasure/Lotus-Petal-shaped card therefore classifiesAccelerant, is protected asNeededManaSource, and deploying it never reduces the deficit — structurally the same feedback loop the land/mana split in this PR was written to remove, just one class narrower. The doc records it and pushes the conjunct onto each consumer, which means every future consumer must remember to re-add it.Consider making the predicate composition explicit here (an
is_intrinsic_mana_sourceconjunct on theAccelerantarm, or a distinctManaRole::OneShotvariant) so the tier axis and the deficit axis share one population, rather than relying on caller discipline.Also applies to: 153-164
🤖 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/card_value.rs` around lines 119 - 141, Update mana_role so the Accelerant classification uses the same intrinsic-source population as mana_behind: require is_intrinsic_mana_source in addition to is_mana_ability, or introduce and consistently handle a distinct one-shot ManaRole if that is the established design. Keep land classification and the None fallback unchanged, and remove or revise the divergence documentation to match the resulting behavior.crates/phase-ai/src/eval.rs (1)
1712-1794: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
land_drop_at_the_former_cap_raises_the_scorebypasses the replacement-aware zone pipeline.The "THE DROP" step calls
engine::game::zones::move_to_zonedirectly. Per this repo's engine contract, zone changes route throughProposedEvent::ZoneChangeso replacements apply; a test that pins "the land drop raises the score" through the raw primitive is not driving the production land-play path (GameAction::PlayLand), so it proves the eval arithmetic only, not the drop. Either narrow the test's claim in its doc, or drive it throughapply/GameAction::PlayLand. Same pattern at Lines 1985 and 2069 in the threat-reweighting test.🤖 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/eval.rs` around lines 1712 - 1794, The test land_drop_at_the_former_cap_raises_the_score currently uses the raw move_to_zone primitive, bypassing replacement-aware zone handling and the production land-play path. Update the THE DROP setup to execute the land play through apply with GameAction::PlayLand, ensuring the hand-to-battlefield transition goes through ProposedEvent::ZoneChange; apply the same correction to the corresponding threat-reweighting test cases around the noted lines, or narrow their documentation if they intentionally remain arithmetic-only.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/engine/src/game/mana_abilities.rs`:
- Around line 70-119: Update the self-removal helper used by
is_renewable_mana_ability to also detect AbilityCost::ReturnToHand targeting
TargetFilter::SelfRef, alongside self-sacrifice costs. Rename
cost_sacrifices_self to reflect both removal forms, and preserve the existing
recursive cost matching so bare and composite costs are handled.
In `@crates/phase-ai/src/duel_suite/harvest.rs`:
- Around line 68-72: Update the documentation for the mana_development_offset
field in the harvest schema to describe the current signed differential
calculation, C × clamp(self − opponent_aggregate, −S, +S), rather than the
obsolete fixed-coefficient formula. Keep the note that it is excluded from
weighted_total and used as a regression input, and align the wording with the
schema version 3 semantics.
In `@crates/phase-ai/src/duel_suite/run.rs`:
- Around line 209-222: Update the schema-3 handling around HarvestMeta and
train_eval_weights.py’s shard metadata filtering so schema-2 and schema-3 rows
cannot be pooled under the unchanged mana_development_offset column. Prefer
adding a minimum-schema guard that excludes older shards, or consistently rename
and consume the column as mana_development_diff; preserve compatible training
behavior for valid schema-3 data.
In `@crates/phase-ai/src/eval.rs`:
- Around line 641-644: Replace the incorrect CR 701.21 citations with CR 701.17
in the comments at crates/phase-ai/src/eval.rs:641-644,
crates/phase-ai/src/zone_eval.rs:135-148, and
crates/phase-ai/src/plan/mod.rs:65-89; make no behavioral code changes.
In `@crates/phase-ai/src/policies/self_cost_value.rs`:
- Around line 149-200: Adjust the `BenefitAppraisal::Priced` boundary in the
self-cost policy to treat tiny negative `net` values as break-even, using a
small epsilon appropriate for floating-point comparison. Preserve the existing
`PolicyVerdict::neutral` path for values within tolerance and keep genuinely
negative values routed to `PolicyVerdict::reject` with the current reasons and
facts.
---
Outside diff comments:
In `@scripts/train_eval_weights.py`:
- Around line 519-527: Fix the empty-phase allocation in load_selfplay_corpus by
replacing the undefined columns reference in phase_features with the loader’s
fixed feature width, using len(SELFPLAY_FEATURE_NAMES) + len(SELFPLAY_CONTROLS).
Leave the non-empty conversion and later phase-skip behavior unchanged.
---
Nitpick comments:
In `@crates/phase-ai/src/card_value.rs`:
- Around line 119-141: Update mana_role so the Accelerant classification uses
the same intrinsic-source population as mana_behind: require
is_intrinsic_mana_source in addition to is_mana_ability, or introduce and
consistently handle a distinct one-shot ManaRole if that is the established
design. Keep land classification and the None fallback unchanged, and remove or
revise the divergence documentation to match the resulting behavior.
In `@crates/phase-ai/src/eval.rs`:
- Around line 1712-1794: The test land_drop_at_the_former_cap_raises_the_score
currently uses the raw move_to_zone primitive, bypassing replacement-aware zone
handling and the production land-play path. Update the THE DROP setup to execute
the land play through apply with GameAction::PlayLand, ensuring the
hand-to-battlefield transition goes through ProposedEvent::ZoneChange; apply the
same correction to the corresponding threat-reweighting test cases around the
noted lines, or narrow their documentation if they intentionally remain
arithmetic-only.
In `@crates/phase-ai/src/policies/payment_selection.rs`:
- Around line 445-450: Replace the wildcard arms in the
PayCostKind::ExileFromManaZone and ExileAggregate zone matches with explicit
remaining Zone variants, grouping variants that share the existing half-card
pricing. Preserve the current Battlefield, Hand, and Graveyard behavior while
making future Zone additions require updating these exhaustive matches.
In `@scripts/train_eval_weights.py`:
- Around line 589-590: Update the raw_coefs construction in the model
coefficient export flow to call zip with strict=True, ensuring
SELFPLAY_FEATURE_NAMES + SELFPLAY_CONTROLS and model.coef_[0] have identical
lengths and mismatches raise immediately instead of truncating silently.
🪄 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: c1bf7678-4e1e-4475-8542-2f9a23b37109
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (47)
crates/engine/src/game/casting.rscrates/engine/src/game/game_object.rscrates/engine/src/game/mana_abilities.rscrates/engine/src/game/mulligan.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/main.rscrates/engine/tests/integration/notion_thief_opponent_draw_redirect.rscrates/phase-ai/Cargo.tomlcrates/phase-ai/baselines/suite-baseline.jsoncrates/phase-ai/fixtures/scenarios/community-scenarios.jsoncrates/phase-ai/src/card_value.rscrates/phase-ai/src/config.rscrates/phase-ai/src/deck_profile.rscrates/phase-ai/src/duel_suite/harvest.rscrates/phase-ai/src/duel_suite/run.rscrates/phase-ai/src/eval.rscrates/phase-ai/src/lib.rscrates/phase-ai/src/plan/mod.rscrates/phase-ai/src/planner/mod.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/blight_value.rscrates/phase-ai/src/policies/cycling_discipline.rscrates/phase-ai/src/policies/free_outlet_activation.rscrates/phase-ai/src/policies/life_total_resource.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/mulligan/card_floor.rscrates/phase-ai/src/policies/mulligan/cedh_keepables.rscrates/phase-ai/src/policies/mulligan/keepables_by_land_count.rscrates/phase-ai/src/policies/mulligan/mod.rscrates/phase-ai/src/policies/payment_selection.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/policies/sacrifice_cost_mana_gate.rscrates/phase-ai/src/policies/sacrifice_value.rscrates/phase-ai/src/policies/self_cost.rscrates/phase-ai/src/policies/self_cost_value.rscrates/phase-ai/src/policies/strategy_helpers.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/tests/sac_outlet_drain_repro.rscrates/phase-ai/src/policies/tests/score_contract_lint.rscrates/phase-ai/src/policies/x_cast_gate.rscrates/phase-ai/src/search.rscrates/phase-ai/src/test_support.rscrates/phase-ai/src/zone_eval.rscrates/phase-ai/tests/ai_quality.rscrates/phase-ai/tests/community_scenarios.rscrates/phase-ai/tests/scenarios.rsscripts/train_eval_weights.py
matthewevans
disabled auto-merge
July 28, 2026 21:19
Contributor
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 `@scripts/train_eval_weights.py`:
- Around line 486-521: Update the shard-processing loop around the per-row
updates so rows, seeds, total, and metadata are accumulated in per-shard
temporary buffers while validating the entire file. Commit those buffers to
phase_feat, phase_lab, seeds, total, and meta only after the shard passes all
schema and metadata checks; ensure a valid row followed by an invalid schema
marker contributes nothing, and add a regression test covering that sequence.
🪄 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: 67ad6a3b-d609-4df1-ac65-7678675500df
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (10)
crates/engine/src/game/mana_abilities.rscrates/phase-ai/Cargo.tomlcrates/phase-ai/src/card_value.rscrates/phase-ai/src/duel_suite/harvest.rscrates/phase-ai/src/duel_suite/run.rscrates/phase-ai/src/eval.rscrates/phase-ai/src/policies/payment_selection.rscrates/phase-ai/src/policies/sacrifice_cost_mana_gate.rscrates/phase-ai/src/policies/self_cost_value.rsscripts/train_eval_weights.py
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/phase-ai/Cargo.toml
- crates/phase-ai/src/duel_suite/run.rs
- crates/phase-ai/src/duel_suite/harvest.rs
- crates/phase-ai/src/policies/sacrifice_cost_mana_gate.rs
- crates/phase-ai/src/policies/payment_selection.rs
- crates/phase-ai/src/card_value.rs
- crates/phase-ai/src/policies/self_cost_value.rs
- crates/phase-ai/src/eval.rs
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Nine units addressing the reported cluster of poor AI decision-making around lands and mana. Every unit went through the
/engine-implementerpipeline (plan → independent plan review to clean → implementation → independent implementation review to clean) and landed only on a zero-finding review round.What was reported, and which unit answers it
The commits
c4f194089a+b4d1f60b42— apply the mulligan card-count floor to every deck, not just cEDH, and correct a false architectural claim in its docstring.de0a5815b0— value mana development with a fixed-coefficient serve-time offset, mirroringenergy_offset. No retrain; the Texel-fitted weights are preserved exactly.7ec5c70ab8— tier cleanup discard and sacrifice selection by mana role, so a land is not interchangeable with a spare spell.8bd50d217b— gate sacrifice costs on the mana the payment would actually destroy.39834f01eb— value mana development as a signed differential against the curve, not an absolute count.18090a18c1— runtime coverage proving Notion Thief's controller does draw. The reported behaviour was correct; this pins it.ce3eb2362d— veto certified-losing self-cost trades, and stop pricing a tapped body as cheaper to give up.0387d5fa6d— refresh the duel-suite baseline so later units are individually measurable.0bffb54b97— price a draw at what the pipeline will actually deliver, rather than at its nominal count.92a0df0552— price an owned commander at what it costs to get it back (CR 903.9a + CR 903.8 tax), and split the five ledger rows that were charging a repurchase price for permanents that are merely used rather than surrendered.Notes for review
crates/phase-ai/baselines/suite-baseline.jsonmoves once, in0387d5fa6d, and is untouched by every unit after it.SacrificeValuePolicy's docstring previously promised a seven-card guarantee that unit C falsifies; it is now stated as the formulaceil((34 - C) / 4) - 1, and the pin measures the bound from a livesacrifice_costcall instead of restating a constant.Controllerdraw payoffs still bypass both the veto and the gate;blight_valuewants a branch-level loss/use split rather than a call-site swap; theOneOfcost arm takes an unconditionalminwith no affordability check.Summary by CodeRabbit