fix(engine): add player-counter Ward cost for "Get five poison counters" - #6662
fix(engine): add player-counter Ward cost for "Get five poison counters"#6662hurryup52 wants to merge 8 commits into
Conversation
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:
📝 WalkthroughWalkthroughThe engine adds ChangesPlayer-counter Ward costs
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleParser
participant WardTrigger
participant PaymentEngine
participant PlayerCounters
participant GameState
OracleParser->>WardTrigger: parse Get five poison counters
WardTrigger->>PaymentEngine: create unless payment
PaymentEngine->>PlayerCounters: add five poison counters
PlayerCounters->>GameState: apply replacement and poison state
GameState-->>PaymentEngine: Paid, Failed, Paused, or GameOver
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🤖 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 `@client/src/pages/GamePage.tsx`:
- Around line 3349-3355: Keep engine counter values opaque in the frontend:
remove any lowercasing or fallback substitution for counter_kind and count, and
pass the engine-provided values unchanged into display rendering. Update the
GetPlayerCounters payload type to a discriminated union with required fields per
counter variant instead of optional ad hoc properties, and use raw engine enum
strings when interpolating translated templates.
In `@crates/engine/src/analysis/ability_graph.rs`:
- Around line 1572-1577: The AbilityCost::GetPlayerCounters arm currently
discards counter_kind and count, so update the loop-analysis handling to
classify supported counter kinds explicitly and preserve their semantic axis and
quantity. Keep poison counters excluded only where justified, and handle
unsupported kinds conservatively rather than treating every variant as a no-op;
extend the relevant parser/loop tests to cover Experience and unsupported kinds.
In `@crates/engine/src/parser/oracle_keyword.rs`:
- Around line 753-786: Update the Ward-cost parsing branch before the mana
fallback to return None for counter-shaped “get … counter” or “get … counters”
text whenever the count or counter kind cannot be parsed. Preserve the existing
GetPlayerCounters result for valid inputs, and add negative tests covering
unknown counter kinds and malformed counts so unsupported text never becomes a
free Mana Ward.
In `@crates/engine/tests/integration/serpent_society_ward_poison_cost.rs`:
- Around line 115-156: The existing Serpent Society Ward test only covers poison
increasing from zero to five. Add a paid-Ward scenario using the same setup and
payment flow, initialize P1 with five poison counters, and assert that reaching
ten causes state-based elimination: P1 is no longer an active priority holder
and the targeted destroy spell does not continue resolving after the loss.
🪄 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: 698b3451-a1d0-4198-860d-e6fa5c1e1da8
📒 Files selected for processing (16)
client/src/i18n/locales/en/game.jsonclient/src/pages/GamePage.tsxcrates/engine/src/analysis/ability_graph.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/cost_payability.rscrates/engine/src/game/costs.rscrates/engine/src/game/engine_payment_choices.rscrates/engine/src/game/printed_cards.rscrates/engine/src/game/replacement.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_keyword.rscrates/engine/src/types/ability.rscrates/engine/src/types/keywords.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/serpent_society_ward_poison_cost.rs
| // CR 702.21a: a Ward player-counter cost, like the effect-side | ||
| // `Effect::GivePlayerCounter` above, carries no modeled axis here — | ||
| // poison accumulation is a loss condition, not a combo resource this | ||
| // loop detector tracks. | ||
| | AbilityCost::GetPlayerCounters { .. } | ||
| | AbilityCost::Unimplemented { .. } => {} |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Preserve counter_kind in loop analysis.
GetPlayerCounters is parameterized, but this arm discards both counter_kind and count. The supplied parser tests create the same variant for Experience, while the comment only justifies ignoring poison counters; this can produce incorrect net-axis and loop conclusions for other player-counter kinds. Classify supported kinds explicitly, or make unsupported kinds conservatively handled and tested.
As per path instructions, parameterized variants must retain their semantic axis; the parser coverage demonstrates this is not a poison-only variant.
🤖 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/engine/src/analysis/ability_graph.rs` around lines 1572 - 1577, The
AbilityCost::GetPlayerCounters arm currently discards counter_kind and count, so
update the loop-analysis handling to classify supported counter kinds explicitly
and preserve their semantic axis and quantity. Keep poison counters excluded
only where justified, and handle unsupported kinds conservatively rather than
treating every variant as a no-op; extend the relevant parser/loop tests to
cover Experience and unsupported kinds.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 High — the new counter cost is not fully threaded through the workspace
Reviewed head 0e2e2f5eea6ba3cf11fad16716c11b984f77a348. The submitted claude-sonnet-5 / Frontier declaration is accepted by current policy; it is not a review finding. The implementation still has these blockers:
-
Rust lint is terminal red with
E0004:WardCost::GetPlayerCountersis missing from both severity matches incrates/phase-ai/src/policies/anti_self_harm.rs:784-811and fromstrategy_helpers.rs:300-343;AbilityCost::GetPlayerCountersis missing fromcrates/mtgish-import/src/convert/action.rs:346and the exhaustive representative test incrates/engine/src/game/costs.rs:1985. Thread the variant through every workspace match without a wildcard escape hatch. In particular, define and test intentional AI semantics for a poison Ward payment that reaches ten: the AI must not treat a lethal self-poison payment as an ordinary affordable/low-severity ward cost. Make compound and non-poison player-counter behavior explicit as well. -
crates/engine/src/parser/oracle_keyword.rs:758-786recognizesget … counter(s)but falls through to mana parsing when its counter shape is malformed or unknown. That can silently lower unsupported text toWardCost::Mana(0). Counter-shapedgetWard text must strict-fail before the mana fallback when either count or kind is unsupported, with negative tests proving malformed counts and unknown counter kinds do not regain free Ward reachability. -
Frontend CI fails locale parity: the new English keys
game.gamePage.cost.playerCounters_{one,other}are absent fromde,es,fr,it,pl, andpt. Add corresponding locale entries. -
serpent_society_ward_poison_cost.rscovers only 0→5 poison. Add a production payment-path case beginning P1 at five poison; assert the ten-poison state-based loss removes P1 from active priority and prevents the targeted destroy spell from continuing. -
client/src/pages/GamePage.tsx:3349-3393supplies defaults and lowercasescount/counter_kind, reinterpreting engine data in the display layer. Make the counter-cost adapter payload a typed discriminated variant with required fields, and render engine-provided values unchanged (or expose a display-ready engine label); remove frontend fallback/lowercasing behavior.
Required CI is currently red (Rust lint/test shards/WASM/consolidated Rust and frontend). Repair all items, bring the branch current as needed, and request a new review on the resulting head; do not enqueue before required checks pass.
Parse changes introduced by this PR✓ No card-parse changes detected. |
CI on phase-rs#6662 caught two gaps my local `cargo check -p engine --lib` never touched: - crates/phase-ai has its own exhaustive matches directly on WardCost (not AbilityCost) for AI tactical scoring: the Ward-cost "severity" pricing in anti_self_harm.rs (both the top-level match and its duplicated inline copy inside the Compound arm) and can_pay_ward_cost's affordability check in strategy_helpers.rs. Add GetPlayerCounters arms to all three: severity scales uncapped with count (voluntarily taking poison counters is a real, severe cost, unlike the capped mana/life costs), and affordability is unconditionally true (no resource limit on giving yourself more counters, mirroring the engine's own can_pay_resolution). - The i18n "locale key parity" test requires es/fr/de/it/pt/pl to have the exact same key set as en, not just en as I'd assumed. Add cost.playerCounters_one/_other and a cost.playerCounterKind noun map to all six locales with real translations (not English placeholders), matching the badges section's existing poison/experience/rad counter naming conventions. Also redesigned formatUnlessCost's GetPlayerCounters case to look the counter-kind noun up through i18n rather than interpolating the raw English enum name, so non-English locales render correctly.
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/phase-ai/src/policies/anti_self_harm.rs (1)
775-797: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftApply Ward-cost scoring to noncreature permanents.
This loop is nested inside the creature-only branch beginning at Line 722, so the new Ward-cost handling is unreachable for artifacts, enchantments, and planeswalkers with Ward. The AI can therefore target a noncreature permanent with
Ward—Get ... counterswithout accounting for the payment cost. Move Ward valuation outside the creature-specific block or share it through a common helper.🤖 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/anti_self_harm.rs` around lines 775 - 797, Move the Ward-cost valuation loop out of the creature-only branch surrounding it, or extract it into a shared helper used by all permanent types. Ensure artifacts, enchantments, and planeswalkers with Ward invoke can_pay_ward_cost and receive the same severity scoring, including GetPlayerCounters, while preserving the existing creature scoring behavior.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/policies/anti_self_harm.rs`:
- Around line 793-797: Update the WardCost::GetPlayerCounters scoring in both
the direct branch around the shown match and the compound-cost path near the
referenced secondary location. Score harmful counters such as poison and rad
using their counter kinds and current player totals where relevant, while
assigning appropriate lower or non-harmful values to beneficial/resource
counters such as experience and tickets; do not continue applying count * 3.0
indiscriminately.
---
Outside diff comments:
In `@crates/phase-ai/src/policies/anti_self_harm.rs`:
- Around line 775-797: Move the Ward-cost valuation loop out of the
creature-only branch surrounding it, or extract it into a shared helper used by
all permanent types. Ensure artifacts, enchantments, and planeswalkers with Ward
invoke can_pay_ward_cost and receive the same severity scoring, including
GetPlayerCounters, while preserving the existing creature scoring 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: 543a5e2b-2bf4-4fec-ae3a-1720a50b33d9
📒 Files selected for processing (10)
client/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/pages/GamePage.tsxcrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/strategy_helpers.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- client/src/pages/GamePage.tsx
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — counter Ward is still incomplete on the fresh head
Reviewed commit fe70e11bc3a14b18f1c184c0b9828a4738571c64.
-
The parse-diff artifact reports no card-parse changes, which conflicts with the PR's claimed The Serpent Society
Mana(0)→GetPlayerCounterschange. Please explain that discrepancy and add actual-card/pipeline evidence that the card parses from card data to the typed Ward cost and reaches the paid-Ward flow; a parser fixture alone is not sufficient. -
crates/engine/src/parser/oracle_keyword.rs:758-786recognizesget N <known> countersbut lets malformed or unknown counter-shapedget ... counterstext fall through toWardCost::Mana(0). Make this grammar failure explicit (or otherwise prevent the zero-mana fallback) and cover negative reach cases. -
The workspace is not exhaustively updated:
crates/mtgish-import/src/convert/action.rsandcrates/mtgish-import/src/convert/costs.rsomitAbilityCost::GetPlayerCounters;crates/phase-ai/src/ability_graph.rsalso discards its kind/count semantics. Update these sites according to their domain contracts rather than treating the new cost as inert. -
crates/phase-ai/src/policies/anti_self_harm.rsassigns every player-counter costcount * 3, andcrates/phase-ai/src/strategy_helpers.rscalls every such cost payable. Counter kinds are not interchangeable: lethal poison at the current total plus the requested count must be rejected, with explicit behavior for the supported resource kinds and compound costs. -
The Serpent Society test covers only poison
0 → 5. Add a discriminating continuation/state-based-action case (for example5 → 10) that demonstrates the cost is not merely parsed but changes the game outcome correctly.
Please also resolve the corresponding review threads after addressing these points.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — corrected current-head review for counter Ward
Reviewed commit fe70e11bc3a14b18f1c184c0b9828a4738571c64. This supersedes the inaccurate path claims in my prior review; the following current-head blockers remain:
-
The parse-diff sticky reports no card-parse changes, contradicting the claimed The Serpent Society
Mana(0)→GetPlayerCountersconversion. Explain the discrepancy and add actual-card/pipeline proof that card data parses the card to the typed Ward cost and reaches the paid-Ward path. A parser fixture alone does not establish that claim. -
crates/engine/src/parser/oracle_keyword.rs:758-786still lets malformed or unknown counter-shapedget ... counter(s)Ward text drop through toWardCost::Mana(0). Fail closed before the mana fallback and add negative reach tests for malformed counts and unknown counter kinds. -
The new
AbilityCost::GetPlayerCountersis missing from therewrite_bound_x_in_ability_costmatch incrates/mtgish-import/src/convert/action.rs:346, producing the Rust exhaustive-match failure. Its actual exhaustive test/mirror iscrates/engine/src/game/costs.rs:1985-2099, where both thesample_formatch andall_variantstags omit the new variant. Thread the no-X semantics through the importer and keep the lockstep test exhaustive. -
client/src/pages/GamePage.tsx:3348-3400silently substitutes a count and converts an unrecognizedcounter_kindto poison (while lowercasing known kinds). The display layer must not reinterpret an engine cost: make the payload typed with required fields and render engine-provided data unchanged, or provide an engine-owned display value. -
crates/phase-ai/src/policies/anti_self_harm.rs:722,775-821scores Ward only within the creature branch, whilecrates/phase-ai/src/tactical_gate.rs:308-315evaluates Ward for every targetable permanent. Give all Ward targets one full typed-cost evaluation: lethal poison must be rejected, supported non-poison kinds must have explicit semantics, compound costs must compose those semantics, and noncreature Ward must receive the same treatment. -
serpent_society_ward_poison_cost.rsonly tests poison0 → 5; that does not discriminate the lethal boundary. Add a real payment-path5 → 10continuation/SBA test proving the player is eliminated and the targeted spell does not continue.
The required Rust aggregate is still failed in the current recommendation packet. Please address these blockers, resolve the related review threads, and request review on a new head.
…frontend type Addresses matthewevans's review on phase-rs#6662: - Third crate with an exhaustive AbilityCost match my earlier per-crate checks missed: crates/mtgish-import/src/convert/action.rs's rewrite_bound_x_in_ability_cost (no-op arm — count is a fixed u32, not an X-bindable QuantityExpr). - crates/engine/src/game/costs.rs's exhaustive "lockstep gate" test (sample_for + all_variants) only exercised under --tests, which this sandbox can't compile, so cargo check --lib alone never caught it. Added a real arm so GetPlayerCounters is actually exercised by every_ability_cost_variant_has_resolution_support_answer, not just made to compile. - AI severity scoring (anti_self_harm.rs) is now lethality- and kind-aware instead of a flat count*3.0: a poison payment that would push the AI to LETHAL_POISON (10, per features::poison) scores prohibitively (100.0, matching this file's own "never do this" sentinel) rather than an ordinary linear severity; non-poison player counters (experience/rad/ticket) carry no loss-condition risk and score 0. Fixed the Compound arm's sum-then-min(4.0) cap, which would otherwise silently clamp a lethal poison sub-cost back down to an ordinary-looking severity. - Parser (oracle_keyword.rs): counter-shaped "get ... counter(s)" text that fails to parse (unparseable count, unknown counter kind) now fails closed (returns None) instead of falling through to the mana-cost fallback — the same silent-free-Ward bug class phase-rs#6640 was about, for different malformed input. Added negative tests for both failure shapes. - Frontend (GamePage.tsx): formatUnlessCost's GetPlayerCounters case is now a real discriminated union member with required, exact-cased fields (count: number, counter_kind: "Poison"|"Experience"|"Rad"| "Ticket") — no more toLowerCase()/fallback-default reinterpretation of engine data in the display layer. Renamed the playerCounterKind i18n keys in all 7 locales to match the engine's exact PascalCase strings. - New integration test: a Ward payment that pushes the payer to ten poison counters triggers the CR 104.3d loss SBA immediately (before the targeted destroy spell can resolve), mirroring sba.rs's own sba_poison_10_player_loses unit test shape. Verified with cargo check --lib on all three affected crates (engine, phase-ai, mtgish-import) — all clean. Full cargo test still blocked by this sandbox's memory ceiling (disclosed previously on this PR); the new/changed test code was hand-verified against the real APIs it calls.
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/phase-ai/src/policies/anti_self_harm.rs (1)
793-874: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing
reaches_lethalhelper instead of duplicating the threshold check.Both the direct arm (Lines 811-813) and the
Compoundhas_lethal_poisonclosure (Lines 834-838) hand-rollcurrent.saturating_add(*count) >= LETHAL_POISON.crates/phase-ai/src/policies/poison.rsalready exposespub(crate) fn reaches_lethal(current_poison: u32, added: u32) -> booldoing exactly this. Reusing it here removes the duplication and the exact kind of drift risk that the earlier review comment on this file already had to flag (the direct vs. compound scoring paths falling out of sync).♻️ Proposed refactor
- let current = - ctx.state.players[ctx.ai_player.0 as usize].poison_counters; - if current.saturating_add(*count) - >= crate::features::poison::LETHAL_POISON - { + let current = + ctx.state.players[ctx.ai_player.0 as usize].poison_counters; + if crate::policies::poison::reaches_lethal(current, *count) { 100.0 } else { *count as f64 * 3.0 }- let has_lethal_poison = costs.iter().any(|c| { - matches!( - c, - WardCost::GetPlayerCounters { - counter_kind: - engine::types::player::PlayerCounterKind::Poison, - count, - } if ctx.state.players[ctx.ai_player.0 as usize] - .poison_counters - .saturating_add(*count) - >= crate::features::poison::LETHAL_POISON - ) - }); + let poison_counter = ctx.state.players[ctx.ai_player.0 as usize].poison_counters; + let has_lethal_poison = costs.iter().any(|c| { + matches!( + c, + WardCost::GetPlayerCounters { + counter_kind: + engine::types::player::PlayerCounterKind::Poison, + count, + } if crate::policies::poison::reaches_lethal(poison_counter, *count) + ) + });Please verify
reaches_lethal's exact module path (crate::policies::poison) and itspub(crate)visibility resolves fromanti_self_harm.rs.As per path instructions, "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/policies/anti_self_harm.rs` around lines 793 - 874, In the direct Poison GetPlayerCounters arm and the Compound arm’s has_lethal_poison closure, replace the duplicated saturating threshold checks with crate::policies::poison::reaches_lethal, passing the current poison total and added count. Preserve the existing lethal severity behavior and verify the pub(crate) helper is accessible from anti_self_harm.rs.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.
Nitpick comments:
In `@crates/phase-ai/src/policies/anti_self_harm.rs`:
- Around line 793-874: In the direct Poison GetPlayerCounters arm and the
Compound arm’s has_lethal_poison closure, replace the duplicated saturating
threshold checks with crate::policies::poison::reaches_lethal, passing the
current poison total and added count. Preserve the existing lethal severity
behavior and verify the pub(crate) helper is accessible from anti_self_harm.rs.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: d6003ea1-89f8-4975-adf3-a754068fd09e
📒 Files selected for processing (13)
client/src/i18n/locales/de/game.jsonclient/src/i18n/locales/en/game.jsonclient/src/i18n/locales/es/game.jsonclient/src/i18n/locales/fr/game.jsonclient/src/i18n/locales/it/game.jsonclient/src/i18n/locales/pl/game.jsonclient/src/i18n/locales/pt/game.jsonclient/src/pages/GamePage.tsxcrates/engine/src/game/costs.rscrates/engine/src/parser/oracle_keyword.rscrates/engine/tests/integration/serpent_society_ward_poison_cost.rscrates/mtgish-import/src/convert/action.rscrates/phase-ai/src/policies/anti_self_harm.rs
🚧 Files skipped from review as they are similar to previous changes (10)
- client/src/i18n/locales/pt/game.json
- client/src/i18n/locales/de/game.json
- client/src/i18n/locales/en/game.json
- client/src/i18n/locales/pl/game.json
- client/src/i18n/locales/fr/game.json
- client/src/i18n/locales/it/game.json
- crates/engine/tests/integration/serpent_society_ward_poison_cost.rs
- client/src/i18n/locales/es/game.json
- crates/engine/src/parser/oracle_keyword.rs
- crates/engine/src/game/costs.rs
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 `@client/src/pages/GamePage.tsx`:
- Around line 3376-3378: Update the counter-kind branch in the cost formatting
logic to pass cost.counter_kind unchanged as the kind interpolation value,
removing the gamePage.cost.playerCounterKind translation. Keep localization
limited to the surrounding gamePage.cost.playerCounters template.
🪄 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: 3f017326-411f-4ad4-b8ec-96ffa40a7fa4
📒 Files selected for processing (2)
client/src/pages/GamePage.tsxcrates/engine/tests/integration/serpent_society_ward_poison_cost.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/engine/tests/integration/serpent_society_ward_poison_cost.rs
| if ("counter_kind" in cost) { | ||
| const kind = t(`gamePage.cost.playerCounterKind.${cost.counter_kind}`); | ||
| return t("gamePage.cost.playerCounters", { count: cost.count, kind }); |
There was a problem hiding this comment.
Keep engine-provided counter_kind unchanged.
This still transforms the engine enum into an i18n key and passes the translated noun as kind. Localize only the surrounding template; pass cost.counter_kind through unchanged or expose a display-ready label from the engine/adapter.
Suggested fix
- const kind = t(`gamePage.cost.playerCounterKind.${cost.counter_kind}`);
- return t("gamePage.cost.playerCounters", { count: cost.count, kind });
+ return t("gamePage.cost.playerCounters", {
+ count: cost.count,
+ kind: cost.counter_kind,
+ });🤖 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 `@client/src/pages/GamePage.tsx` around lines 3376 - 3378, Update the
counter-kind branch in the cost formatting logic to pass cost.counter_kind
unchanged as the kind interpolation value, removing the
gamePage.cost.playerCounterKind translation. Keep localization limited to the
surrounding gamePage.cost.playerCounters template.
Sources: Path instructions, Learnings
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
🔴 Blocker — current head still treats player-counter Ward as safe when it can lose the game
Reviewed commit 4af0dd76762a91769a782cadff20f4f7e609272b.
-
HIGH — payability is still split from scoring.
crates/phase-ai/src/policies/strategy_helpers.rs:337-346returnstruefor everyGetPlayerCounters, andcrates/phase-ai/src/tactical_gate.rs:307-315uses that result as a hard target-choice gate.anti_self_harm.rs:805-876only changes the later score, so the AI can still select a Ward payment that takes poison from 5 to 10. Its compound logic also checks each poison component against the same starting total, so individually nonlethal components can be jointly lethal. Put typed player-counter payability in the shared authority, aggregate poison acrossCompound, and add target-choice tests proving both direct and compound lethal poison Ward are rejected. -
MED — Rad is incorrectly valued as harmless.
anti_self_harm.rs:801-819assigns non-poison player counters zero harm, but Rad is a typed admitted counter kind (oracle_nom/quantity.rs:5387-5393) whose engine resolution mills and can cause life loss (game/effects/rad_counters.rs:13-15,99-159). Give every supported kind explicit typed valuation and test Rad directly and inside Compound. -
MED — Ward counter parsing remains structurally outside the nom grammar.
oracle_keyword.rs:758-788now fails closed, but it recognizes itscounter(s)tail withstrip_suffixrather than composable nom productions. Refactor the count/article, kind, and singular/plural axes into nom combinators so this parser family has one grammatical authority rather than ad-hoc string dispatch. -
The parse-diff sticky predates this head and still reports zero card changes. Refresh/reconcile the artifact with the claimed The Serpent Society conversion and provide current-head actual-card pipeline evidence.
Lower priority but confirmed: client/src/pages/GamePage.tsx:3367-3378 still localizes/interprets the engine counter kind in the display layer; keep the transport/display contract explicit while fixing the blocking engine/AI behavior. I am not repeating the previously refuted ability-graph claim.
|
Correction to review #4781781812: its statement that the parse-diff sticky predates the current head is no longer accurate. The refreshed parse-diff artifact was updated at 2026-07-26T13:07:37Z for head This corrects only the artifact-freshness claim. Reconciliation and actual-card pipeline evidence remain required, and all other blockers in the current changes-requested review stand. |
|
@matthewevans — pushed d5d0f44 addressing all three blockers plus the parse-diff evidence and the lower-priority note: 1. Payability split from scoring (HIGH) — fixed. 2. Rad valued as harmless (MED) — fixed. Rad now scores 3. Ward parsing outside the nom grammar (MED) — fixed. 4. Parse-diff reconciliation / actual-card pipeline evidence — ran {
"name": "The Serpent Society",
"keywords": [
"Deathtouch",
{ "Ward": { "type": "GetPlayerCounters", "data": { "counter_kind": "Poison", "count": 5 } } }
]
}Before this issue's fix (verified against Lower priority — GamePage.tsx counter_kind display: fixed. Replaced Also fixed (from an earlier round, not repeated in your last review but still open): the Ward severity-pricing block in Verification this round: |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/policies/strategy_helpers.rs`:
- Around line 324-329: Update can_pay_ward_cost’s lethal-poison check to
calculate the effective poison counter increase using the same player-counter
replacement logic as add_player_counter_with_replacement, rather than
total_poison_from_ward_cost alone. Compare the post-replacement count against
LETHAL_POISON, and add a regression test covering a poison Ward cost modified by
a replacement effect.
🪄 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: 0ff40a5f-e6d1-4e95-84a4-a352a6e55287
📒 Files selected for processing (5)
client/src/pages/GamePage.tsxcrates/engine/src/parser/oracle_keyword.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/strategy_helpers.rscrates/phase-ai/src/tactical_gate.rs
🚧 Files skipped from review as they are similar to previous changes (2)
- client/src/pages/GamePage.tsx
- crates/engine/src/parser/oracle_keyword.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the new noncreature Poison/Rad Ward behavior reaches target scoring without a production-seam regression.
🟡 Non-blocking
[MED] Missing AntiSelfHarmPolicy target-selection coverage for payable noncreature player-counter Ward costs. Evidence: crates/phase-ai/src/policies/anti_self_harm.rs:845-939 scores payable Ward costs on every permanent type, while the new noncreature Ward cases in crates/phase-ai/src/tactical_gate.rs:1137-1269 only assert GateDecision affordability/rejection. Why it matters: a change can preserve the tactical gate yet omit or mis-rank the Poison/Rad Ward penalty in the production target scorer. Suggested fix: add a target-selection regression through AntiSelfHarmPolicy using a noncreature permanent with a payable, nonlethal GetPlayerCounters Poison or Rad Ward and an unwarded equivalent; assert the Ward target receives the intended lower score/ranking.
Recommendation: request changes — add the production target-scoring regression, then request another review.
|
Pushed 86c8d26 addressing the target-scoring coverage gap: Added `noncreature_ward_target_scores_lower_than_unwarded_equivalent` in `anti_self_harm.rs` — two identical opponent-owned artifacts (one bare, one with a small nonlethal payable poison-counter Ward: 0+2 poison, well under the lethal threshold) both as legal targets of the same removal (exile) effect. Asserts `AntiSelfHarmPolicy.score` ranks the unwarded artifact higher than the warded one, confirming the severity-pricing block I moved out of the creature-only branch last round actually reaches the production target scorer's ranking output for a noncreature target, not just the Verified: `cargo check -p phase-ai --lib`, `cargo test -p phase-ai --lib noncreature_ward_target_scores_lower` (1 passed), `cargo clippy -p phase-ai --lib -- -D warnings`, `cargo fmt --all` — all clean. |
The Serpent Society's Ward reads "Ward—Get five poison counters." No WardCost variant existed for "give yourself N counters", so the parser fell through to mana-cost parsing, found no mana symbols, and silently produced WardCost::Mana(generic: 0) — a free, always-paid Ward that never gave the targeting opponent any poison counters (issue phase-rs#6640). Add WardCost::GetPlayerCounters { counter_kind, count } (parameterized over PlayerCounterKind rather than poison-only, so a future Ward cost of a different counter kind reuses the same shape) and a matching AbilityCost::GetPlayerCounters variant. Wire it through: - ward_cost_to_ability_cost (triggers.rs) - supported_at_resolution + can_pay_resolution + pay_ability_cost_inner (costs.rs) — the resolution-time payment authority; pay_ability_cost_inner routes through the existing add_player_counter_with_replacement so "can't get counters" replacement effects still apply - handle_unless_payment (engine_payment_choices.rs) — the unless-pay round trip, unchanged in shape otherwise - parse_ward_cost_single (oracle_keyword.rs) — new nom-combinator branch for "get N <kind> counter(s)", reusing the same parse_number/ parse_player_counter_kind combinators the GivePlayerCounter effect parser already uses, inserted before the mana-cost fallback that caused the silent misparse Every other exhaustive AbilityCost match in the crate (ability_graph.rs, ability_scan.rs, cost_payability.rs, printed_cards.rs, replacement.rs, oracle_effect/lower.rs, plus four helper methods on AbilityCost itself) now has an explicit arm, found and fixed via the compiler's own exhaustiveness checking rather than manual auditing. Also fixes the frontend Ward-payment prompt, which would otherwise show a useless generic "Pay a cost" label for this new cost shape. Closes phase-rs#6640
CI on phase-rs#6662 caught two gaps my local `cargo check -p engine --lib` never touched: - crates/phase-ai has its own exhaustive matches directly on WardCost (not AbilityCost) for AI tactical scoring: the Ward-cost "severity" pricing in anti_self_harm.rs (both the top-level match and its duplicated inline copy inside the Compound arm) and can_pay_ward_cost's affordability check in strategy_helpers.rs. Add GetPlayerCounters arms to all three: severity scales uncapped with count (voluntarily taking poison counters is a real, severe cost, unlike the capped mana/life costs), and affordability is unconditionally true (no resource limit on giving yourself more counters, mirroring the engine's own can_pay_resolution). - The i18n "locale key parity" test requires es/fr/de/it/pt/pl to have the exact same key set as en, not just en as I'd assumed. Add cost.playerCounters_one/_other and a cost.playerCounterKind noun map to all six locales with real translations (not English placeholders), matching the badges section's existing poison/experience/rad counter naming conventions. Also redesigned formatUnlessCost's GetPlayerCounters case to look the counter-kind noun up through i18n rather than interpolating the raw English enum name, so non-English locales render correctly.
…frontend type Addresses matthewevans's review on phase-rs#6662: - Third crate with an exhaustive AbilityCost match my earlier per-crate checks missed: crates/mtgish-import/src/convert/action.rs's rewrite_bound_x_in_ability_cost (no-op arm — count is a fixed u32, not an X-bindable QuantityExpr). - crates/engine/src/game/costs.rs's exhaustive "lockstep gate" test (sample_for + all_variants) only exercised under --tests, which this sandbox can't compile, so cargo check --lib alone never caught it. Added a real arm so GetPlayerCounters is actually exercised by every_ability_cost_variant_has_resolution_support_answer, not just made to compile. - AI severity scoring (anti_self_harm.rs) is now lethality- and kind-aware instead of a flat count*3.0: a poison payment that would push the AI to LETHAL_POISON (10, per features::poison) scores prohibitively (100.0, matching this file's own "never do this" sentinel) rather than an ordinary linear severity; non-poison player counters (experience/rad/ticket) carry no loss-condition risk and score 0. Fixed the Compound arm's sum-then-min(4.0) cap, which would otherwise silently clamp a lethal poison sub-cost back down to an ordinary-looking severity. - Parser (oracle_keyword.rs): counter-shaped "get ... counter(s)" text that fails to parse (unparseable count, unknown counter kind) now fails closed (returns None) instead of falling through to the mana-cost fallback — the same silent-free-Ward bug class phase-rs#6640 was about, for different malformed input. Added negative tests for both failure shapes. - Frontend (GamePage.tsx): formatUnlessCost's GetPlayerCounters case is now a real discriminated union member with required, exact-cased fields (count: number, counter_kind: "Poison"|"Experience"|"Rad"| "Ticket") — no more toLowerCase()/fallback-default reinterpretation of engine data in the display layer. Renamed the playerCounterKind i18n keys in all 7 locales to match the engine's exact PascalCase strings. - New integration test: a Ward payment that pushes the payer to ten poison counters triggers the CR 104.3d loss SBA immediately (before the targeted destroy spell can resolve), mirroring sba.rs's own sba_poison_10_player_loses unit test shape. Verified with cargo check --lib on all three affected crates (engine, phase-ai, mtgish-import) — all clean. Full cargo test still blocked by this sandbox's memory ceiling (disclosed previously on this PR); the new/changed test code was hand-verified against the real APIs it calls.
…wing
Two real compile/type errors caught by CI on the previous push:
- crates/engine/tests/integration/serpent_society_ward_poison_cost.rs:
`WaitingFor::GameOver { winner: Some(p) if p == P0 }` nests the guard
inside the `Some(p)` sub-pattern, which is the unstable `guard_patterns`
feature (rust-lang/rust#129967), not a stable match-arm guard. Moved
the guard to the whole-pattern position:
`WaitingFor::GameOver { winner: Some(p) } if p == P0` — standard,
stable Rust. Verified in isolation with a standalone rustc invocation
(this sandbox can't compile the full integration test binary).
- client/src/pages/GamePage.tsx: `switch (cost.type)` / a `cost.type ===
"GetPlayerCounters"` check can't actually narrow the discriminated
union to the `GetPlayerCounters` member, because the sibling member's
`type: string` is too wide for literal-comparison narrowing to
exclude it — `cost.counter_kind` doesn't type-check inside that
branch. Switched to a `"counter_kind" in cost` property-presence check
before the switch, which narrows correctly since the sibling member
doesn't declare that property at all.
Verified locally this time (pnpm/tsc/vitest are available in this
sandbox, unlike full Rust test codegen): `tsc -b --noEmit` clean,
`eslint` clean (pre-existing warning only, unrelated line), and the
full `vitest run` suite green (270 files / 2366 tests passed).
… Ward correctly - can_pay_ward_cost now aggregates poison across the whole Compound cost tree before checking lethality once, instead of checking each sub-cost against the same starting total (missed jointly-lethal combinations). - anti_self_harm.rs gives Rad, Experience, and Ticket counters explicit typed severity valuations instead of a zero-cost wildcard fallback. - The Ward severity-pricing block now runs for every permanent type, not just creatures, so an affordable Ward tax on a noncreature target is priced by the AI's judgment layer too. - oracle_keyword.rs's "get N <kind> counter(s)" ward-cost parsing is now fully composed from nom combinators instead of ad hoc strip_suffix dispatch, and fails closed (no fallback to a free Mana Ward) on malformed count/kind text. - GamePage.tsx maps the engine's player-counter kind to its i18n key via an exhaustive switch instead of interpolating the raw engine value into the key template, so a future counter kind fails to compile here rather than silently missing a translation. Addresses matthewevans's review on phase-rs#6640.
The prior fix moved Ward severity pricing out of the creature-only branch so it runs for every permanent type, but nothing exercised the production target scorer directly for a noncreature case -- only tactical_gate's accept/reject GateDecision tests covered it. Adds a regression asserting an unwarded artifact scores higher (a more attractive target) than an otherwise-identical artifact with a small, payable, nonlethal poison-counter Ward.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the current head has one compile error and one replacement-safety gap in lethal poison Ward scoring.
🔴 Blocker
[HIGH] The new PendingTrigger fixture supplies an unboxed ability where the current type requires Box<ResolvedAbility>. Evidence: crates/phase-ai/src/policies/anti_self_harm.rs:3492 constructs PendingTrigger, and :3496 passes bare ResolvedAbility::new(...); current Rust lint and test shard 2 report E0308. Why it matters: the PR cannot compile. Suggested fix: wrap the ResolvedAbility::new(...) value in Box::new(...) at the fixture construction site.
🟡 Non-blocking
[MED] Lethal poison Ward scoring uses the nominal counter total rather than the replacement-aware applied amount. Evidence: crates/phase-ai/src/policies/strategy_helpers.rs:324-329 compares current + total_poison_from_ward_cost, while crates/engine/src/game/costs.rs:1336-1346 pays GetPlayerCounters through add_player_counter_with_replacement. Why it matters: an amount-changing player-counter replacement can make a Ward payment lethal after the AI accepts it. Suggested fix: project the replacement-aware poison result before scoring, or conservatively reject when it cannot be projected; add a doubled-poison boundary regression.
Recommendation: request changes — repair the boxed fixture and make lethal-poison affordability replacement-aware before requesting another review.
…am rebase Rebased onto upstream/main, which boxed GameState.pending_trigger and PendingTrigger.ability since this branch was created. The new noncreature Ward scoring test still constructed both unboxed.
86c8d26 to
816f749
Compare
|
The CI failures on the last push (`Rust lint`, `Rust tests shard 1/2`, `shard 2/2` on run 30222057052) were all a single root cause, not separate issues: this branch was 20 commits behind `upstream/main`. Since this branch was created, main boxed `GameState.pending_trigger` (`Option<Box>`) and `PendingTrigger.ability` (`Box`) as part of unrelated upstream work (workspace bumped to v0.38.0). The noncreature Ward scoring test added last round still constructed both unboxed, which failed to compile — and since every one of those CI jobs builds `phase-ai`'s test binary, the single compile error surfaced as multiple red checks. Fixed in `816f749eb`: rebased onto `upstream/main` and wrapped both construction sites in `Box::new(...)`. Verified against the rebased code: `cargo check -p phase-ai --lib` clean, `cargo test -p phase-ai --lib noncreature_ward_target_scores_lower` (1 passed), `cargo clippy -p phase-ai --lib -- -D warnings` clean, `cargo fmt --all` clean (no changes). |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the earlier fixture boxing issue is fixed, but two counter-cost paths remain rules-incorrect.
🔴 Blockers
[HIGH] Ward poison affordability uses the printed count instead of the replacement-adjusted result. crates/phase-ai/src/policies/strategy_helpers.rs:324-329 gates lethal poison with total_poison_from_ward_cost, while the cost path at crates/engine/src/game/costs.rs:1335-1344 applies counters through the replacement-aware helper. A doubling or +N replacement can make the AI accept a lethal Ward payment. Project the result side-effect-free through replacement handling, or conservatively decline when it cannot be projected; add a doubled/+N boundary regression test.
[HIGH] A prevented player-counter payment is currently accepted as paid. crates/engine/src/game/effects/player_counter.rs:36-58 returns true for ReplacementResult::Prevented, and crates/engine/src/game/costs.rs:1911-1914 makes GetPlayerCounters unconditionally payable. The parser already models “Players can't get counters” as counter prevention in oracle_replacement.rs:9145-9163, so a Ward counter cost can be paid for free under that effect. Model a distinct payment result (or map prevention to a failed cost) and cover Solemnity plus The Serpent Society.
The current CI/parse artifacts are still incomplete, but these blockers are independently reproducible from the current source and do not depend on them.
…paid Two rules-correctness bugs in the Ward player-counter cost path: - can_pay_ward_cost trusted WardCost::GetPlayerCounters's printed count when checking poison lethality, while actual payment routes counter additions through the replacement pipeline. A doubler or +N effect on the payer could make a lethal payment look safe. Added preview_player_counter_addition (mirrors the existing object-counter preview_counter_addition) to project the real, replacement-adjusted result side-effect-free; total_poison_from_ward_cost now uses it and conservatively declines when the outcome can't be cleanly projected. - add_player_counter_with_replacement returned true for a replacement- prevented addition (e.g. Solemnity's "players can't get counters"), identical to a genuinely applied one. Effect-resolution callers correctly treat prevented and applied the same way (nothing more to wait for), but the GetPlayerCounters cost arm in costs.rs used that same true to mean "paid" -- so a Ward's poison-counter cost could be paid for free under a counter-prevention effect, bypassing Ward entirely. The function now returns a PlayerCounterAdditionOutcome (Applied/Prevented/NeedsChoice); the cost arm maps Prevented to a failed payment so Ward correctly counters the spell instead. Adds a doubling-replacement boundary test (tactical_gate.rs) and a Solemnity + Serpent Society integration test asserting the payment fails and the targeting spell is countered.
|
@matthewevans — pushed afb85f4 addressing both HIGH blockers. 1. Affordability used the printed count instead of the replacement-adjusted result. Added
2. A prevented player-counter payment was accepted as paid.
Added Verification this round: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the engine now rejects a prevented player-counter payment, but the AI still treats it as a free safe Ward cost.
🔴 Blocker
[HIGH] A prevented counter preview becomes zero poison and leaves the Ward cost payable. crates/phase-ai/src/policies/strategy_helpers.rs:324-328 maps PlayerCounterAdditionPreview::Prevented to Some(0), while :411-415 continues to mark GetPlayerCounters payable. The payment path now correctly fails a prevented counter addition in crates/engine/src/game/costs.rs:1345-1358; the AI can therefore target into Solemnity/player-counter prevention, receive the optimistic Ward evaluation, and have its spell countered.
Treat prevented or unprojectable player-counter preview as unpayable recursively, and add a tactical-gate regression for the prevention case.
🟡 Evidence requested
The sticky parse-diff artifact predates this head even though parser files changed. Please refresh it against this commit so the production impact is attributable to the submitted source.
2 participants
Summary
The Serpent Society's Ward reads "Ward—Get five poison counters." No
WardCostvariant existed for "give yourself N counters," so the parser fell through to mana-cost parsing, found no mana symbols, and silently producedWardCost::Mana(generic: 0)— a free, always-paid Ward that never gave the targeting opponent any poison counters. AddsWardCost::GetPlayerCounters { counter_kind, count }/AbilityCost::GetPlayerCountersand wires it through the existing Ward/unless-pay machinery — no new interactive round-trip needed.Files changed
crates/engine/src/types/keywords.rs— newWardCost::GetPlayerCountersvariantcrates/engine/src/types/ability.rs— matchingAbilityCost::GetPlayerCountersvariant + 4 helper-method armscrates/engine/src/game/triggers.rs—ward_cost_to_ability_costarm + test casecrates/engine/src/game/costs.rs—supported_at_resolution,can_pay_resolution,pay_ability_cost_innerarmscrates/engine/src/game/engine_payment_choices.rs—handle_unless_paymentarmcrates/engine/src/parser/oracle_keyword.rs— new nom-combinator parser branch + 3 unit testscrates/engine/src/analysis/ability_graph.rs,crates/engine/src/game/ability_scan.rs,crates/engine/src/game/cost_payability.rs,crates/engine/src/game/printed_cards.rs,crates/engine/src/game/replacement.rs,crates/engine/src/parser/oracle_effect/lower.rs— arms for the six other exhaustiveAbilityCostmatches in the crate (found via the compiler's own exhaustiveness checking, not manual audit)client/src/pages/GamePage.tsx+client/src/i18n/locales/en/game.json— Ward-payment prompt label (would otherwise show a generic "Pay a cost" fallback for this new cost shape)crates/engine/tests/integration/serpent_society_ward_poison_cost.rs(new) +crates/engine/tests/integration/main.rs— integration testsTrack
Non-developer
LLM
Model: claude-sonnet-5
Tier: Frontier
Thinking: medium
Implementation method (required)
Method: not-applicable — implemented directly. Traced the full Ward pipeline (parser →
ward_cost_to_ability_cost→ the resolution-timeAbilityCostpayment authority → theUnlessPayment/PayUnlessCostround trip) against the existing "enchanted player is attacked"-style precedents before writing any code, per theadd-keyword/card-testskills, rather than spawning the/engine-implementerpipeline.CR references
GetPlayerCounterscost/effect shape).All three verified against
docs/MagicCompRules.txtdirectly before writing (104.3d at line 346, 702.21a at line 4114).Verification
cargo fmt --all— clean.cargo check -p engine --lib— clean (Finished dev profile ... in ~37son the final pass), confirming every production-code match arm (including the 6 additional exhaustiveAbilityCostsites found only by the compiler) type-checks.cargo test -p engine/cargo clippy— could not run to completion in this environment, same limitation already disclosed on PR fix(engine): parse "one or more of your opponents are attacked" triggers #6658 this session: Tilt isn't running here, and every directcargo test/cargo check --testsinvocation for this crate gets OOM-killed (17GB RAM, no swap) regardless of flags — this is a cold, non-incremental, non-Tilt build of a very large crate, not something local retries fix. The new parser unit tests (oracle_keyword.rs) and integration tests (serpent_society_ward_poison_cost.rs) were verified by hand against the exact nom-combinator functions and scenario/runner API calls they use (confirmed each helper's real signature by reading the source, and modeled the integration test line-for-line on the existingphyrexian_fleshgorger_ward.rs), but this is not a substitute for an actual compile+run. Flagging explicitly rather than claiming green — real CI will run the full suite./engine-implementerpipeline invocation).Anchored on
crates/engine/src/parser/oracle_keyword.rs(parse_ward_cost_single's existing"pay N life"/"sacrifice ..."branches) — the direct structural precedent for the new "get N counter(s)" branch, same nom-combinator idiom.crates/engine/src/game/costs.rs(EffectCost/Effect::PutCounterarm usingeffects::counters::add_counter_with_replacement) — the precedent for routingGetPlayerCounters's payment througheffects::player_counter::add_player_counter_with_replacementrather than a raw state mutation, so replacement effects still apply.crates/engine/tests/integration/phyrexian_fleshgorger_ward.rs— the exactGameScenario/GameRunnerWard test pattern (add_creature_from_oracle+add_spell_to_hand_from_oracle→cast(..).target_objects(..).commit()→advance_until_stack_empty()→ assertWaitingFor::UnlessPayment→act(PayUnlessCost{pay})) the new test file mirrors.Claimed parse impact
The Serpent Society:
WardCost::Mana(generic: 0)→WardCost::GetPlayerCounters { counter_kind: Poison, count: 5 }.Scope Expansion
None —
WardCost::GetPlayerCountersis parameterized overPlayerCounterKind(not poison-only) per this repo's "parameterize, don't proliferate" principle, since the shape is identical for any player-counter kind, but no other card's parse output is claimed as changed by this PR.Validation Failures
None known — see the Verification section for the one open item (local test execution blocked by sandbox memory, not a known validation failure).
CI Failures
None yet observed (PR not yet run through CI at the time of writing).
Summary by CodeRabbit