feat(phase-ai): add discard-matters deck-feature axis + DiscardPayoffPolicy - #6786
Conversation
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
superagent-security
Bot
added
the
contributor:flagged
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds structural detection for self-discard sources and payoff engines, integrates the resulting commitment feature into deck analysis, and introduces a registry-enabled policy that rewards qualifying discard actions using live battlefield triggers. ChangesDiscard payoff evaluation
Estimated code review effort: 4 (Complex) | ~45 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant DeckFeatures
participant DiscardMatters
participant DiscardPayoffPolicy
participant BattlefieldTriggers
DeckFeatures->>DiscardMatters: analyze discard sources and payoff engines
DiscardMatters-->>DeckFeatures: return commitment feature
DiscardPayoffPolicy->>DiscardPayoffPolicy: classify candidate controller discard
DiscardPayoffPolicy->>BattlefieldTriggers: check live discard payoff triggers
BattlefieldTriggers-->>DiscardPayoffPolicy: return fireable engine count
DiscardPayoffPolicy-->>DeckFeatures: return capped policy verdict
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
…Policy CR 701.9: a deck built on Archfiend of Ifnir, Bone Miser, Waste Not or Containment Construct *wants* to discard — every card pitched is a repeatable value trigger. The AI has the opposite instinct: `card_advantage` scores a card leaving hand as a loss, and nothing credits the trigger it fires. So the AI declines its own engine, routing around rummaging outlets and treating a discard cost as pure downside even when the discard IS the payoff. Feature (`features/discard_matters.rs`), structural over `CardFace` AST: - `source_count` — self-discard outlets. BOTH enabler spellings are read: `Effect::Discard` (rich form) and `Effect::DiscardCard` (older simple form). They are separate `Effect` variants with separate resolver paths and real cards use each, so classifying only one would silently drop half the class. - `payoff_count` — `TriggerMode::Discarded` / `DiscardedAll` engines, keeping only controller-scoped ones (a "whenever an opponent discards" punisher is a different deck) and excluding `SelfRef` triggers, so a pile of madness cards cannot masquerade as an engine base. - `commitment` — geometric mean over (source, payoff): both pillars mandatory. Discard with no engine is pure card disadvantage and the AI is RIGHT to avoid it; this axis must not push it to. Policy (`policies/discard_payoff.rs`, `CastSpell` + `ActivateAbility`) credits a discard only when a LIVE engine is on the battlefield, preflighted through the engine's `hypothetical_trigger_fireable` authority so a rate-limited, off-timing or no-legal-target engine is not credited value it cannot produce. The card-local check runs first, so every non-discard candidate is rejected after reading one card's AST — no board sweep, no affordability query in the search inner loop. Boundaries, stated in the module docs so they are reviewable rather than inferred: - `hand_disruption` scores OPPONENT discard (`TargetFilter::Opponent`); this axis is the disjoint half (`Controller`). The scope that qualifies here is exactly the one that disqualifies there, so the two never read the same card. - `cycling_discipline` owns when to pay a cycling cost. `TriggerMode:: CycledOrDiscarded` is deliberately NOT read here — counting it would double-score one card across two policies with different intents. Calibration anchors are computed, not asserted from intuition: 10 outlets + 4 engines over 36 nonland → 0.878; 6 + 2 → 0.481; incidental 2 + 1 → 0.196, below the 0.35 floor. Each is pinned by a test to the value the formula returns. Both scoring scalars land in `UNTUNED_POLICY_PENALTY_FIELDS` pending a paired-seed calibration. 60 tests. Five are proven RED by breaking the specific invariant each guards: dropping the `Effect::DiscardCard` sibling (fails at BOTH the deck-time and live seams), dropping the opponent-scope guard, admitting cycling triggers, and dropping the `SelfRef` madness exclusion. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
50d2a46 to
137e0bc
Compare
|
Rebased onto The conflict was self-inflicted and worth naming: this branch was cut before #6743 (the cost-reduction axis) merged as Five files auto-merged. A naive concatenation there yields an unclosed function and a nested one. Both hunks were resolved by hand so each side keeps its own closing token. I then explicitly verified that the merged cost-reduction registrations survived at every site, since silently dropping them would revert landed work rather than just break the build:
Verification on the rebased head:
The |
d0cbea3 to
137e0bc
Compare
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the stated activated-discard use case is not reached.
🔴 Blocker
discard_matters.rs:104-108 and 187-204 only classify Effect::Discard / Effect::DiscardCard; discard_payoff.rs:148-155 applies that same effect-only predicate to an activated ability. Real rummaging outlets pay AbilityCost::Discard (ability.rs:8309-8320), which this path never reads, so activating the Wild Mongrel/Anje class remains neutral even with a live discard engine.
The claimed regression does not witness that path: policies/tests/discard_payoff.rs:62-70 constructs an activated ability whose effect is discard, while activated_rummaging_outlet_is_credited calls that helper at 304-319. It never supplies AbilityDefinition.cost = Some(AbilityCost::Discard { .. }).
Extend the shared discard-source classification to account for positive, controller-owned discard costs (including the appropriate Composite / OneOf semantics), use it for deck detection and activation scoring, and add a cost-backed runtime policy test that would fail without the change. The accidental fork-only release merge has been removed; this review is against current head 137e0bc3.
Recommendation: repair the cost path and resubmit for review.
…an effect Addresses the blocker on phase-rs#6786. The axis was built for the rummaging class — Wild Mongrel, Anje Falkenrath, flashback-discard — and then never reached it. Those cards spell the discard as `AbilityCost::Discard` (ability.rs:8309), while the source predicate read only `Effect::Discard` / `Effect::DiscardCard`. So activating a real outlet stayed neutral even with a live engine on the battlefield: exactly the case the policy exists to fix. Worse, the regression that claimed to cover it did not. `activated_rummaging_ outlet_is_credited` built an ability whose *effect* was a discard — a shape no printed card has — so it exercised the effect path a second time and witnessed nothing about costs. A test that constructs its own subject can confirm a behavior that never occurs. The fix extends the SHARED `is_discard_source_parts`, so deck detection and activation scoring pick it up from one place rather than growing a second cost-only path that could drift from the effect one: - `ability_cost_discards` gates on the engine's own `AbilityDefinition::cost_categories()` authority, which already flattens `Composite`/`OneOf` nesting, before walking anything. - `cost_discards` then walks for a discard the payer actually makes, reusing the existing `AbilityScope` parameter for the branch question. CR 601.2h: every component of a `Composite` is paid, so a discard inside it is guaranteed. CR 118.12a: a `OneOf` resolves to one branch, so its discard is something the DECK can plan around (`Potential`) but not something a live candidate is committed to (`Unconditional`) — crediting it there would score a discard the player may never make. - Count positivity flows through the existing `DiscardQuantity` parameter, so a zero-count cost is not credited at the live seam. - Non-recursive cost forms defer to `categories()` rather than a hand-written match arm: a newly added discarding cost variant is then picked up automatically, and the count check that path skips can only UNDER-credit. A discard cost needs no controller filter — it is always paid by the activating player, so there is no opponent-scoped case to exclude the way there is for `Effect::Discard`. Tests 60 → 68. Six of the eight new ones fail without the cost axis (three deck-time, three live), including the real Wild Mongrel shape. The two negatives — `one_of_cost_is_not_credited_at_the_live_seam` and `zero_count_discard_cost_is_not_credited` — would pass vacuously against the old code, so they were separately proven by mutating the `OneOf` branch gate and the count check; both fail under that mutation. Verified: `cargo test -p phase-ai` 1853 passed, `cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings` exit 0, `check-engine-authorities.sh` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Fixed at The blockerThe axis was built for the rummaging class and then never reached it. Wild Mongrel, Anje and flashback-discard all spell the discard as The test that didn't witness it
The fixExtended the shared
A discard cost needs no controller filter — it is always paid by the activating player, so there is no opponent-scoped case to exclude the way there is for Tests: 60 → 68Six of the eight fail without the cost axis — three deck-time, three live — including the real Wild Mongrel shape ( The other two are negatives — Verification
The body's |
Maintainer fixup — current-head holdThe prior cost-axis blocker is resolved on this head. I also corrected the Required CI, the paired-seed AI gate, decision-cost performance gate, and CodeRabbit restarted on |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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/policies/tests/discard_payoff.rs`:
- Around line 503-506: Update the discard-payoff tests around
DiscardPayoffPolicy.verdict, including the cases near the referenced assertions,
to capture the returned score delta alongside reason. Assert a positive delta
for the guaranteed composite path and assert a zero delta for the zero-count and
no-engine neutral paths, while retaining the existing reason.kind checks.
- Around line 494-496: Add a test case in the discard payoff tests covering a
Composite cost containing Tap and a nested OneOf with Discard and PayLife, and
assert it returns discard_payoff_na with a zero delta. Ensure the test exercises
recursive classification so selecting the non-discard branch does not receive
credit.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c4670fe8-f8b0-432a-8372-a46d5230827c
📒 Files selected for processing (3)
crates/phase-ai/src/features/discard_matters.rscrates/phase-ai/src/features/tests/discard_matters.rscrates/phase-ai/src/policies/tests/discard_payoff.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/phase-ai/src/features/discard_matters.rs
…ore deltas Addresses the two CodeRabbit findings on phase-rs#6786, on top of the maintainer's `OneOf` fixup. 1. Nested `OneOf` inside a `Composite` was unproven. A top-level `OneOf` and a flat `Composite` do not show that recursion carries the "not guaranteed" answer UP through the composite, so `Composite([Tap, OneOf([Discard, PayLife])])` could have been credited by treating any nested discard as certain. Added that case plus its positive twin, `Composite([Tap, OneOf([Discard, Discard])])`, so the boundary is pinned from both sides rather than only the rejecting one. 2. Several cost-path tests asserted only `reason.kind`, which would stay green if the guaranteed-composite path returned 0.0 or a neutral path returned a nonzero score. They now capture and assert `delta` too — including the maintainer's `one_of_cost_with_only_discard_branches_is_credited_at_the_live_seam`, which promised positive credit without checking it. `empty_one_of_cost_is_not_credited` is deliberately NOT claimed as a guard on the `!costs.is_empty()` precondition. Mutation testing showed it stays green with that check deleted: an empty `OneOf` never reaches the walk, because the engine's `cost_categories()` gate reports no `Discards` category for a branch list with nothing in it. The test is kept because the outcome is worth pinning at whichever layer enforces it, and its comment now says exactly that instead of implying coverage it does not provide. Tests 69 → 72. `composite_wrapping_a_mixed_one_of_is_not_credited` is proven RED by flipping the `OneOf` live-seam quantifier from `all` to `any`. Verified: `cargo test -p phase-ai` 1857 passed, `cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings` exit 0. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
Both CodeRabbit findings addressed at Your correction is better than what I wrote — I blanket-rejected every 1. Nested optional discard. Valid gap. A top-level 2. Assert deltas, not only reason IDs. Also valid — several cost-path tests would have stayed green if the guaranteed-composite path returned One thing I need to correct in my own work. I wrote I kept the test, because the outcome is worth pinning at whichever layer enforces it, but its comment now says exactly that instead of implying coverage it doesn't provide. Reporting it rather than letting "72 tests" carry an implication one of them doesn't earn. The Verification at this head:
Tests 69 → 72. Body's |
Maintainer review — current-head holdReviewed current head |
Maintainer hold — paired-seed gate pendingCurrent head |
Re-review requested — the held condition is now metThe paired-seed AI gate has passed (gate pass), which was the sole remaining condition in your hold on Per your note, approval and merge-queue enrollment could resume once it completed successfully "without another implementation review unless the head changes" — the head has not changed. It is still Current signals on that exact head:
The For completeness, I also ran the paired-seed control locally against this head rather than relying solely on CI, since that job had timed out at its 60-minute limit on three earlier runs across both PRs: Zero flips on every deck, which is consistent with the policy being inert across the quick-filter suite — no gate deck clears Happy to rebase, re-run anything, or take further findings if you would rather another pass. Thanks for the two fixups you pushed on this one — the |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — spell-cast discard costs are still outside the policy’s source authority.
🔴 Blocker
crates/phase-ai/src/policies/discard_payoff.rs:141-146 classifies every GameAction::CastSpell only through CastFacts::primary_effects. That collection is spell-resolution text only (crates/phase-ai/src/cast_facts.rs:224-229), so it cannot see the casting-cost authority on the object. The engine represents ordinary additional casting costs at GameObject::additional_cost / AdditionalCost::{Required, Optional, Kicker, Choice} (crates/engine/src/types/card.rs:196-200, crates/engine/src/types/ability.rs:8994-9030); it separately makes Retrace and Jump-start discard requirements part of cast legality (crates/engine/src/game/casting.rs:13068-13078). Consequently, a cast whose mandatory additional cost discards (the Abandon Hope / Big Score / Cathartic Reunion class) remains neutral even with a live payoff engine.
The added cost tests do not witness that production path: crates/phase-ai/src/policies/tests/discard_payoff.rs:466-597 constructs AbilityDefinition.cost on an ActivateAbility candidate. They validate activated costs only, not a CastSpell with additional_cost or a graveyard casting variant.
Extend the cast-side classification at the existing casting-cost authority. sacrifice_cost_mana_gate.rs:195-245 is the relevant pattern: it exhaustively distinguishes AdditionalCost::Required from optional/kicker/choice costs before inspecting the cost structure. Preserve that distinction here—mandatory discard may be credited, while an optional or selected-choice cost must be credited only when the candidate/selection proves it will be paid. Include retrace and jump-start in the same cast-cost seam, and add production-path policy tests for a mandatory spell additional cost, an unselected optional/choice cost, and the applicable graveyard-cast cases.
Recommendation: rework the CastSpell branch around casting-cost provenance and resubmit for review.
2 participants
Summary
Adds a
discard_mattersdeck-feature axis and its companionDiscardPayoffPolicytophase-ai, so the AI can see that in some decks discarding is the payoff, not the cost.CR 701.9: with Archfiend of Ifnir, Bone Miser, Waste Not or Containment Construct on the battlefield, every card pitched is a repeatable value trigger. The AI's instinct is the opposite —
card_advantagescores a card leaving hand as a loss, and nothing credits the trigger it fires — so it declines its own engine, routing around rummaging outlets and treating a discard cost as pure downside.Files changed
crates/phase-ai/src/features/discard_matters.rs(new)crates/phase-ai/src/features/tests/discard_matters.rs(new)crates/phase-ai/src/policies/discard_payoff.rs(new)crates/phase-ai/src/policies/tests/discard_payoff.rs(new)crates/phase-ai/src/features/mod.rscrates/phase-ai/src/features/tests/mod.rscrates/phase-ai/src/policies/mod.rscrates/phase-ai/src/policies/tests/mod.rscrates/phase-ai/src/policies/registry.rscrates/phase-ai/src/config.rsTrack
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: not-applicable — this is AI heuristic code under
crates/phase-ai/. Nocrates/engine/file is touched: no parser, effect-resolution, targeting, stack or rules-behavior change. The axis reads already-parsed AST through existing engine authorities.CR references
CR 701.9— Discard (the keyword action this axis is built on). Verified atdocs/MagicCompRules.txt:3327.CR 107.1b— a resolved quantity of zero moves no card, so a zero-count discard emits no event and fires no engine.CR 700.2— modality: a live candidate is scored before modes are chosen, so only an unconditional discard is credited.CR 603.3d— trigger target legality, via the engine'shypothetical_trigger_fireablepreflight.Every number was grep-verified against
docs/MagicCompRules.txtbefore being written. No CR annotation is attached to ordinary implementation logic.Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo test -p phase-ai— 1801 passed, 0 failed, plus every integration binary green (includingevery_policy_penalty_is_tuning_registered_or_explicitly_untuned, which enforces that both newPolicyPenaltiesfields are registered).cargo test -p phase-ai --lib discard— 60 passed, 0 failed.cargo clippy -p phase-ai -p phase-engine --all-targets --features proptest -- -D warnings— exit 0../scripts/check-engine-authorities.sh— exit 0.cargo fmt --all— clean.Mutation evidence that the guards are not vacuous. Four invariants were broken individually and five tests went RED: dropping the
Effect::DiscardCardsibling (fails at BOTH the deck-time and the live seam), dropping the opponent-scope guard, admittingCycledOrDiscarded, and dropping theSelfRefmadness exclusion.One note on method, because it nearly produced a false conclusion: with all four mutations applied at once,
opponent_discard_is_not_a_sourcedid not fail — theDiscardCardmutation had disabled the very variant that test's fixture uses, masking the scope mutation. Re-run in isolation it fails correctly. Combined mutations can hide a real guard and make it look vacuous.cargo ai-gateis not attached: no quick-filter deck in the suite clearsDISCARD_MATTERS_FLOOR, so the policy is inert there, and the gate has been timing out at its 60-minute job limit on recent heads. Happy to attach a paired-seed control if you want it on this head.Gate A
Gate A PASS head=275d6812376e64a42508c33b933bb3238050caa0 base=1604a6f3023413a34740d6d268d77872b3150b0f
Anchored on
crates/phase-ai/src/features/draw_matters.rs:75— the established axis shape this mirrors:detect()folding per-DeckEntryCardFaceAST into counts,pub(crate)parts-based predicates shared with the policy, and acommitmentscalar;:248is the same two-pillarcommitment::geometric_meanwith a calibration anchor in its docstring.crates/phase-ai/src/policies/draw_payoff.rs:45— the sameTacticalPolicyshape:activation()as a single floor-plus-scale knob, cheap-to-expensiveverdict()ordering where a card-local AST check rejects non-members before any board scan, thehypothetical_trigger_fireableliveness preflight,MAX_REWARDED_ENGINESas apub(crate)cap the bounded-score test asserts against, and the exhaustiveGameActionmatch so a new variant fails at compile time.Final review-impl
Final review-impl PASS head=275d6812376e64a42508c33b933bb3238050caa0
Self-review before submission caught and fixed two things:
_ => falseon theGameActionmatch. Replaced with the exhaustive 126-variant enumeration the house idiom requires, so a newly added action fails this match at compile time instead of silently bypassing the payoff.Claimed parse impact
None. No parser, card-data or Oracle-text code is touched.
Scope Expansion
None. One feature axis and one policy, in new files, plus their registration sites.
Validation Failures
None.
CI Failures
None known at submission.
🤖 Generated with Claude Code
Summary by CodeRabbit