fix(engine): honor the power gate on Goreclaw's spell cost reduction - #6626
fix(engine): honor the power gate on Goreclaw's spell cost reduction#6626michiot05 wants to merge 5 commits into
Conversation
"Creature spells you cast with power 4 or greater cost {2} less to cast"
reduced ALL spells: the cost-reduction static parsed with spell_filter:null
(the "creature ... with power 4+" gate dropped), and even once parsed the
cost-modifier path evaluated a power gate as fail-closed false.
Two layers:
- Parser (oracle_static/static_helpers.rs): add a "with power N or
greater/less" cost-mod qualifier peel mirroring the mana-value qualifier
(phase-rs#5606), reusing the canonical parse_pt_comparison combinator. Goreclaw now
parses spell_filter = Typed{Creature, [PtComparison{Power,Current,GE,4}]}.
- Runtime (game/filter.rs): the cost-modifier matcher evaluates a spell
against a SpellCastRecord snapshot that carries no power/toughness, so
FilterProp::PtComparison was fail-closed. Add a PtComparison arm to
spell_object_matches_property that reads the live spell object's printed P/T
via the filter context (object_pt_value) — guarded to the context-bearing
cost/prohibition/keyword-grant callers, so every snapshot-only caller stays
byte-identical fail-closed. No SpellCastRecord/serde change, no new variant.
Now a power-4+ creature spell is reduced, power-3 and noncreature spells are
not (CR 208.1/208.3, 601.2f). Runtime honors spell_filter end-to-end.
Closes phase-rs#6375
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
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:
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughCost-modification parsing recognizes power-qualified creature spells, and live spell-object filters evaluate power comparisons using effective current P/T, including dynamic CDAs. Coverage signatures and unit/integration tests validate parsing, filtering, and Goreclaw cost reductions. ChangesPower-gated cost filtering
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleStaticParser
participant CostFilter
participant LiveSpellObject
participant EffectivePTResolver
participant CostDisplay
OracleStaticParser->>CostFilter: build creature filter with power gate
CostFilter->>LiveSpellObject: evaluate spell-object properties
LiveSpellObject->>EffectivePTResolver: compute effective P/T and dynamic CDAs
EffectivePTResolver-->>LiveSpellObject: return effective power
LiveSpellObject-->>CostFilter: return comparison result
CostFilter->>CostDisplay: apply matching cost reduction
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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/parser/oracle_static/static_helpers.rs`:
- Around line 139-147: Update parse_cost_mod_power_qualifier to accept a power
qualifier only when the remainder after parse_pt_comparison(rest.trim_start())
is empty after trimming; otherwise treat it as invalid. Preserve the distinction
between no qualifier and an unparseable or partially parsed qualifier so
strip_cost_mod_power_qualifier and try_parse_cost_modification return None
instead of producing an unrestricted filter.
In `@crates/engine/tests/integration/goreclaw_power_gated_cost_reduction_6375.rs`:
- Around line 46-47: Update goreclaw_generics and its callers so
shared_card_db() unavailability cannot silently produce passing tests without
assertions. Require the fixture for this regression suite, or explicitly mark
the suite ignored and run it only through a configured fixture-backed test job,
while ensuring the tests exercise the parser and production cost path.
🪄 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: b7369bd6-0cb6-4382-b05f-1aca71a21fdc
📒 Files selected for processing (5)
crates/engine/src/game/filter.rscrates/engine/src/parser/oracle_static/static_helpers.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/tests/integration/goreclaw_power_gated_cost_reduction_6375.rscrates/engine/tests/integration/main.rs
Parse changes introduced by this PRBaseline pending for |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] Dynamic-P/T creature spells are rejected even when their CDA gives them power 4 or greater. Evidence: crates/engine/src/game/filter.rs:3495-3525 evaluates PtComparison from the candidate object's stored P/T, but crates/engine/src/game/printed_cards.rs:1876-1881 initializes PtValue::Variable and PtValue::Quantity to 0; crates/engine/src/game/layers.rs:1824-1854 resets only hand-zone keywords, not P/T. CR 208.2a requires a P/T characteristic-defining ability to function everywhere. Why it matters: Goreclaw must reduce an eligible CDA creature spell (for example, a Terravore/Tarmogoyf-like spell) rather than treating it as power 0. Suggested fix: resolve the candidate spell's off-battlefield characteristics through the CDA/layer authority before comparing power, with discriminating dynamic-P/T 3-vs-4+ production-path tests and a fixed-P/T control.
[MED] The required parse-diff is a false negative for this PR's claimed parser change. Evidence: the current-head parse-diff artifact/comment reports "No card-parse changes detected" despite this PR changing StaticMode::ModifyCost.spell_filter; crates/engine/src/game/coverage.rs:4352-4387 projects only static affected, modifications, condition, CDA, and zone, omitting ModifyCost payload fields including spell_filter. Why it matters: CI cannot establish that Goreclaw's generated card-data filter changed, or detect a future blast radius in this semantic axis. Suggested fix: project semantically relevant ModifyCost fields (including spell_filter) into the coverage parse signature, add a red/green parse-diff regression test, and regenerate CI so the sticky identifies Goreclaw.
…ect ModifyCost.spell_filter into coverage (phase-rs#6375 review) Address matthewevans's two MED findings on phase-rs#6626: 1. A dynamic-P/T characteristic-defining spell (Tarmogoyf: power = distinct card types among all graveyards) was read as power 0 off the battlefield — `printed_cards::parse_pt` maps `*` to 0 and the layer pass only re-derives P/T for battlefield objects — so Goreclaw's power gate wrongly skipped it, contrary to CR 208.2a (a P/T CDA functions in all zones). New helper `spell_object_effective_pt_value` gathers the spell object's own self-sourced continuous effects (active_continuous_effects_from_static_source), keeps only characteristic-defining SelfRef dynamic-P/T modifications, applies set-CDAs (layer 7a/7b, CR 613.4a) before add-CDAs (7c) resolving each magnitude via the layer pass's own resolve_quantity, and falls back to the stored value when the object sources no CDA. Called only from the cost-path PtComparison arm; the battlefield and zone-change matchers (whose objects already have computed P/T) are untouched. 2. The coverage parse-diff signature omitted StaticMode::ModifyCost payload fields, so Goreclaw's spell_filter change (None -> Typed{Creature,[PtComparison]}) hashed identically and CI reported "No card-parse changes." static_details now projects the ModifyCost mode/amount/spell_filter/dynamic_count. Tests: 3 discriminating production-path CDA cases (Tarmogoyf power 4 reduced / power 3 not / fixed-P/T control) + a red/green coverage-signature test. Both revert-sensitive. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — both fixed at head MED 1 — dynamic-P/T CDA spells read as power 0 off-battlefield. Confirmed: Discriminating production-path tests via MED 2 — coverage parse-diff omitted Verified: |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: blocked — the dynamic-P/T fix is present, but the new off-battlefield CDA evaluator still omits conditional fixed P/T.
🔴 Blocker
[HIGH] Off-battlefield CDA evaluation skips fixed power/toughness setters. Evidence: crates/engine/src/game/filter.rs:4182-4198 applies only dynamic Set* variants before returning the computed value, while crates/engine/src/parser/oracle_static/tests.rs:825-900 shows that the parsed conditional constant P/T CDA form uses ContinuousModification::SetPower { value: 2 } and SetToughness { value: 2 } (Angry Mob's off-turn clause). Why it matters: the new cost-filter path starts from an off-battlefield object's stored P/T and can therefore evaluate a card whose active constant CDA is 2/* as printed 0/*, incorrectly failing a power gate. Suggested fix: handle applicable self-CDA SetPower/SetToughness in the resolver's initial set stage, in the same ordering as the layer authority, and add a discriminating production display_spell_cost test for a conditional constant-P/T creature spell.
Recommendation: request changes for the missing fixed-setter CDA path.
… SetPower/SetToughness (phase-rs#6375 review r2) Address matthewevans's 2nd-round HIGH: the off-battlefield CDA evaluator (spell_object_effective_pt_value) applied only the dynamic Set*/Set*Dynamic CDAs and skipped the fixed-constant ContinuousModification::SetPower{value} / SetToughness{value} setters (Angry Mob's conditional "are each N" clause), so a creature spell whose active constant CDA is 4/* was read as printed 0/* and wrongly failed Goreclaw's power gate. Add the two fixed-constant setters to the same layer-7a/7b set stage as the dynamic setters (before the 7c add stage), mirroring the layer authority (layers.rs lists all six P/T-set variants together). Also harden the shared self_cda filter with the layer pass's exact per-recipient condition gate (effect.condition.is_none_or(evaluate_condition_with_recipient(.., obj.id)), mirroring apply_continuous_effect_filtered) so a conditional CDA (fixed or dynamic) is applied only inside its window; None-condition CDAs (Tarmogoyf, fixed-P/T) pass unchanged. Fixed-P/T fallback is byte-identical. Tests: two conditional-constant-P/T cases through the production display_spell_cost path — a DuringYourTurn SetPower{4} spell is reduced on your turn; the Not{DuringYourTurn} variant stays printed 3 and is not reduced (condition-gating discriminator). CR 613.4a / 611.3a grep-verified. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Thanks — fixed at head You're right: the off-battlefield evaluator applied only the dynamic While there I also hardened the shared Discriminating tests through the production
Revert-proven: removing the two fixed arms flips the active case (reads printed 3, GE-4 rejects); restored → green. Tarmogoyf dynamic-CDA (power 4 reduced / 3 not) + Fire Elemental fixed control still pass. |
|
Current-head disposition for [MED] The required parse-diff does not substantiate the PR's claimed one-card scope and is stale for the current head. Evidence: the PR says [MED] The regression suite does not drive the actual cast action. Evidence: The security pre-check is clean (only the six engine/parser/test paths in this PR); no maintainer fixup was pushed. Current CI remains pending on Rust lint and both Rust test shards. The |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head disposition for 5de75508568f4e1614cb2540eb00b6fa78c17735: changes requested.
[MED] Fresh parse-diff / scope mismatch. The required <!-- coverage-parse-diff --> artifact does not substantiate this PR's claimed Goreclaw-only scope and is stale for the current head. The PR says only Goreclaw and Scope Expansion: None, while the artifact reports 555 cards / 249 signatures and predates this head. The broad result comes from projecting every ModifyCost payload field in coverage.rs, so it is a signature-schema migration as well as the Goreclaw filter change. Publish a fresh parse-diff for the current head and update the scope claim to account for that migration, or split the migration so Goreclaw's parser delta is independently reviewable.
[MED] Display-only cast-cost regression test. The Goreclaw regression suite asserts display_spell_cost, but does not drive the actual cast action / payment path (prepare_spell_cast with CastingMode::Actual). Because this fixes casting-cost behavior, add a scenario that submits the real cast action with sufficient mana and proves the power-3 spell pays its full generic cost while the power-4-or-greater spell pays the reduced cost.
These current-head blockers must be resolved before approval or merge-queue entry.
2 participants
Tier: Frontier
Model: claude-opus-4-8
Closes #6375.
Summary
Goreclaw, Terror of Qal Sisma — "Creature spells you cast with power 4 or greater cost {2} less to cast." — reduced the cost of every spell you cast. It's a two-layer defect (the parser-only fix alone would have flipped it to reducing nothing):
1. Parser — the power gate was dropped. The cost-reduction static parsed with
spell_filter: nulloveraffected: Typed{Card}, so it matched all your spells. Fix #5606 added a "[type] spells with mana value N or greater/less" qualifier arm to the cost-modifier parser; there was no "with power N" counterpart, so the power gate fell through. Added a parallelparse_cost_mod_power_qualifierinoracle_static/static_helpers.rs, mirroring the mana-value arm and reusing the canonicalparse_pt_comparisoncombinator (the same building block Goreclaw's own attack trigger already uses — no hand-rolled power parser). Goreclaw now parsesspell_filter = Typed{ Creature, [PtComparison{ Power, Current, GE, Fixed(4) }] }.2. Runtime — the cost-modifier matcher fail-closed on power. Even with a correct
spell_filter, the cost path evaluates a candidate spell against aSpellCastRecordsnapshot (spell_object_matches_property→spell_record_matches_property) that carries no power/toughness, soFilterProp::PtComparisonsat in the fail-closed=> falsegroup → Goreclaw would reduce nothing. Added aPtComparisonarm tospell_object_matches_property(game/filter.rs) that reads the live spell object's printed P/T via the filter context (context.spell_object_id → object_pt_value, the same authority the battlefield matcher uses), evaluating the comparator. It is guarded to the context-bearing callers (cost / can't-cast / keyword-grant, which supplySome(spell_object_id)); every snapshot-only /None-context caller returnsfalseexactly as before, so the fail-closed record path is unchanged. NoSpellCastRecord/serde change, no new enum variant.Result: a power-4+ creature spell is reduced; a power-3 creature spell and any noncreature spell are not (a noncreature is rejected by the type gate, and independently its power
None → 0failsGE 4— CR 208.3). ScopeCurrentis correct for an unresolved spell (= printed power; per Goreclaw's ruling, later +1/+1 counters don't retroactively count).Implementation method (required)
Method: /engine-implementer — plan (/engine-planner, with a run-then-delete reproduction probe confirming power-3/noncreature spells were wrongly reduced at base) → /review-engine-plan → implement (tests-first). During implementation, driving the real production cost path (not a force-set
power) empirically falsified the plan's "parser-only-safe" premise — the cost matcher reads a power-less snapshot — so the executor stopped rather than ship a fix that reduced nothing. A focused design-and-validation pass then pinned the runtime seam (traced the cost path decisively; ruled over-broadening safe by enumerating every caller of the shared matcher) → implement runtime arm → /review-impl (Maintainer-Simulation Gate PASS; two LOW notes, non-gating). Each step in a fresh agent context.Gate A
./scripts/check-parser-combinators.sh→ Gate A PASS head=ddbeec86d (Gate G PASS). The parser peel istake_until+parse_pt_comparison(pure combinators).Anchored on
oracle_static/static_helpers.rs:parse_cost_mod_mana_value_qualifier/strip_…/compose_…) — the exact peel-and-compose shape the power arm mirrors, one axis over (Cmc→PtComparison).compose_cost_mod_mana_valueis renamedcompose_cost_mod_qualifier_prop(alreadyFilterProp-generic; one caller) and called for both qualifiers.nom_filter::parse_pt_comparison(oracle_nom/filter.rs) — the canonical P/T-comparison combinator (self-consumeswith/base, gives GE/LE/EQ), already used by Goreclaw's attack trigger andoracle_target::parse_power_suffix. No new power parser.object_pt_value(game/filter.rs) — the single authority the live/battlefield matcher already uses to read an object's P/T; the new cost-path arm reuses it against the live spell object, so power evaluation is identical across matchers.Verification
cargo fmt --allclean;cargo clippy -p engine --lib --tests -- -D warnings0 warnings;cargo test -p engine --lib17,701 passed / 0 failed (no drift from the sharedfilter.rschange);cargo test -p engine --test integration -- goreclaw4 passed;cargo test -p engine --lib -- cost_mod_power4 passed (parser shape + attack-trigger control).crates/engine/tests/integration/goreclaw_power_gated_cost_reduction_6375.rs) drive the realdisplay_spell_costpath over real card faces viaadd_real_card(printed power, never force-set), verbatim Goreclaw Oracle text:spell_filtertonullre-reduces the power-3 spell (the original "reduces all spells" bug,left: 1, right: 3).GE/LE/ bare-EQpower qualifiers, plus the attack-trigger control (stillPtComparison{Power,GE,4}onTyped{Creature}, untouched).docs/MagicCompRules.txt: 208 / 208.1 / 208.3 (power is a creature characteristic; noncreatures have none), 601.2f (total cost), 202.3.Claimed parse impact
Parser: among real cards, only Goreclaw's cost-modifier
spell_filterchanges (fromnullto theCreature+ power-4 gate); the qualifier peel is all-or-nothing, so any subject lacking" with power "is byte-identical. Runtime: only the context-bearing cost / can't-cast / keyword-grant path gainsPtComparisonevaluation (all previously fail-closedfalse, so every flip is a bug fix); all snapshot/None-context callers are unchanged, confirmed by the full 17,701-test lib suite. The exact card-data parse-diff is deferred to CI'scoverage-parse-diffartifact (local card-data/MTGJSON corpus unavailable) — I expect it to report only Goreclaw's cost-mod signature.Scope Expansion
None. The runtime arm was necessary to make the parser fix actually correct (otherwise Goreclaw reduces nothing) and is minimal + guarded (no serde/type change). Honest limitation, out of class: a
*/*CDA creature spell reads power 0 off-battlefield (CR 208.3 /parse_ptmaps*→0), so it wouldn't be reduced — a pre-existing off-battlefield CDA limitation, not introduced here and outside Goreclaw's fixed-power class.🤖 Generated with Claude Code
Summary by CodeRabbit