fix(engine): add player-counter Ward cost for "Get five poison counters" - #6662
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)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 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.
|
Generated for head 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.
…-ccf2845 # Conflicts: # crates/engine/src/game/printed_cards.rs
|
Maintainer port review is clean at Hold: #6844 remains OPEN and independently covers the same #6640 / The Serpent Society Ward fix. Do not approve or enqueue either PR until reconciliation selects one canonical PR and closes or supersedes the other. CI and the current-head parse-diff are in progress for this new head. Both must finish before reconsideration. |
|
Maintainer update for current head The safe compatibility fix adds Replacement CI and the current-head parse-diff result must complete before this can move forward. Hold remains: #6844 is still OPEN and independently covers the same #6640 / The Serpent Society Ward issue. Neither PR may be approved or enqueued until canonical reconciliation selects one PR and closes or supersedes the other. |
|
Maintainer update for current head The safe port fix updates only a stale stage2 census source coordinate after a +13 line shift. It makes no production or Ward behavior change. Replacement CI and the current-head parse-diff result are pending. Hold remains: #6844 is OPEN and independently covers the same #6640 / The Serpent Society Ward issue. Neither PR may be approved or enqueued until reconciliation selects one canonical PR and closes or supersedes the other. |
# Conflicts: # crates/engine/src/game/engine.rs
Maintainer reconciliation update — current head
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head changes requested — parse evidence is stale
Reviewed 78ae37012f4959a9b7fc3abfb966d6b47639addc.
The sole <!-- coverage-parse-diff --> receipt is the existing sticky comment, and it explicitly identifies old head 8d47671edb6f5e26740a73312e76838413871167. This PR changes engine/parser behavior and now has a new merged head, so that receipt cannot support approval of the current commit.
Regenerate a current-head parse-diff receipt/artifact and reconcile its zero-card status with current-head actual-card pipeline evidence for The Serpent Society's Ward conversion. The evidence must be bound to this head, not inferred from the prior artifact or earlier local output.
CI is presently in progress on this head; completion of checks is required but is not a substitute for the current-head parse artifact. The earlier functional blockers are not being re-litigated in this review.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current-head evidence complete
Reviewed 78ae37012f4959a9b7fc3abfb966d6b47639addc.
The refreshed parse-diff receipt is bound to this head and reports no card-parse changes. Current-head actual-card evidence reconciles that zero-card status for The Serpent Society's supported-aspect Ward conversion. Required CI is green, and the fresh review found no remaining functional blocker.
Approved for merge queue. The retained bug label matches the actual change; no quality label is applied.
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