fix(engine): Ovika parses 'mana value of that spell' of-form, not just possessive - #6526
Conversation
📝 WalkthroughWalkthroughThe parser now shares prepositional object-scope handling for mana-value phrases. Casting tests cover Ovika’s mana-value-scaled Goblin tokens and Pure Reflection’s mana-value-based Reflection token stats. ChangesMana Value and Casting Coverage
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/engine/src/game/casting_tests.rsast-grep timed out on this file 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: 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/engine/src/game/casting_tests.rs`:
- Around line 36344-36362: Extend the token assertions in the battlefield-token
loop to verify every promised characteristic: red color, both Goblin and
Phyrexian subtypes, and power/toughness of 1/1, while preserving the existing
haste assertion. Use the token object’s existing color and P/T fields and
provide diagnostic assertion messages consistent with the current checks.
🪄 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: 28b979cf-30ae-485f-be37-5ad87e3f787c
📒 Files selected for processing (2)
crates/engine/src/game/casting_tests.rscrates/engine/src/parser/oracle_nom/quantity.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocking finding
parse_object_prepositional_anaphor_scope duplicates the existing parse_object_color_of_scope object-scope grammar while covering fewer sibling forms (it, the enchanted creature, and the equipped creature are missing). The new function’s “mirrors” claim is therefore not true, and future object-property grammar will drift.
Please reuse/parameterize the existing prepositional object-scope parser for the mana-value fallback (after the targeted-of-form path), retaining its recipient and target sibling coverage. The Ovika runtime test is a good production-path proof for the target case; Scryfall’s current Oracle text confirms the intended “mana value of that spell” clause.
CodeRabbit’s request to assert every token characteristic is non-blocking test-strengthening feedback; this architecture issue is the blocking item.
Parse changes introduced by this PR · 2 card(s), 3 signature(s) (baseline: main
|
…ma Goliath) Ovika, Enigma Goliath (issue phase-rs#1718) never created its Goblin tokens: the trigger fired but its "create X 1/1 red Phyrexian Goblin creature tokens, where X is the mana value of that spell" effect lowered to Effect::Unimplemented. parse_object_mana_value_ref (crates/engine/src/parser/oracle_nom/quantity.rs) recognized the possessive front-form "that spell's mana value" (-> ObjectManaValue { EventSource } via parse_object_possessive_scope) but the prepositional of-form "the mana value of that spell" required a "target" keyword and errored otherwise, so the where-X token count could not bind and the whole clause dropped to Unimplemented. Accept the non-target of-form by mirroring the possessive scope mapping via a new parse_object_prepositional_anaphor_scope combinator (that spell -> EventSource, that creature/permanent -> Target, this ... -> Source), tried only after the existing targeted TargetObjectManaValue path so "mana value of target creature" is unaffected.
b22369f to
9e42854
Compare
|
Addressed the blocking architecture finding: the duplicate object-scope table is gone. What changed
Tests
Full local parity on this head (isolated CARGO_TARGET_DIR, rebased onto
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Held on current head 9e42854b. The prior duplicated object-scope grammar blocker is resolved: the mana-value of-form now delegates to the shared prepositional scope parser, and the Ovika regression drives the cast/trigger pipeline at two mana values. Required Rust verification is still in progress and Tauri currently reports a failed job, so this is not ready for approval or enqueue.
|
One contributor-side blocker on 🔴 Blocker
🟡 Non-blocking — not your defect
✅ Clean
Recommendation: push one commit swapping the raw keyword read for |
…ty gate The raw `keywords.contains(&Keyword::Haste)` in the new Ovika token-creation runtime test tripped the Engine authority gate (raw keyword query). It asserts the literal keyword set stamped on the freshly created token, not an effective-keyword query, so annotate it with allow-raw-authority per the gate's documented escape hatch.
|
Pushed a one-line fix for the failing Engine authority gate: the new Ovika token-creation test used a raw The remaining |
|
The authority-gate blocker is confirmed fixed on ✅ The blocker is resolved — and CI proves it, not just my readingYour delta obj.keywords.contains(&Keyword::Haste), // allow-raw-authority: asserts the literal
// keyword set stamped on the freshly created token, not an effective-keyword queryThe mechanism works for a trailing same-line comment — 📋 Both CI reds are ours — nothing for you to do
Everything else on this head is green — both Rust test shards, Card data, WASM, Frontend, Lobby worker. Attributed by ancestry rather than timestamps, with the probe run first so the test isn't vacuously exonerating: We'll bring the branch current — it's ✅ Architecture fix — re-verified independentlyI didn't rely on my earlier confirmation.
✅ The parse-diff strengthens the PR — mention it in the descriptionSticky is current (updated The PR claims Ovika only, but a second card moved — Pure Reflection. I verified its Oracle text against Scryfall rather than assuming: "Whenever a player casts a creature spell, destroy all Reflections. Then that player creates an X/X white Reflection creature token, where X is the mana value of that spell." Identical of-form construction. That's in-class coverage, and it's the "build for the class, not the card" evidence — worth one line in the description so the 2-card delta is self-explaining. 🟡 One ask before enqueuePin the token's P/T — This matters more here than the usual "assert what the comment claims" nit, and the parse-diff is what sharpens it: your seam binds event source's mana value, and the two cards in the diff bind it to different axes — Ovika to the token count, Pure Reflection to the token P/T. With count pinned at 3 but P/T unpinned, a count↔P/T axis confusion (three 3/3 tokens instead of three 1/1s) passes green. Adding This confirms CodeRabbit's inline finding at Nit: 📋 Process noteMy Recommendation: add the P/T assertion, and we'll update your branch and approve. The CR annotations all grep-verify and describe their code — CR 202.3 ( |
|
Your round-4 blocker is resolved and I'm clearing it. The red is ours, not yours — don't touch it. Reviewed at head The Tauri failure is our lockfile skewThe Only Tauri, and the real error is:
Your diff touches exactly two files, both engine ( Clearing the round-4 blocker — and correcting myself on itThe keyword-assertion blocker is genuinely resolved. Your head commit annotates it with the gate's documented escape hatch: obj.keywords.contains(&Keyword::Haste), // allow-raw-authority: asserts the literal keyword set stamped on the freshly created token, not an effective-keyword queryand I also want to correct the rationale I gave you in round 4. I said a raw What's genuinely good hereRound 3's duplicate-grammar blocker is properly fixed rather than worked around. The of-form now delegates to the shared The I verified Ovika's Oracle text from Scryfall rather than your description, and confirmed against the shipped The parse-diff measures 2 cards, not 1 — Pure Reflection also gains a 🟡 One open item before I enqueueThe token assertions verify two of the five characteristics the test claims. [LOW] Both runtime fixtures use generic-only mana costs ( That's a ~6-line test-only commit. Push it and I'll enqueue. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Changes requested — the parser seam is sound, but the runtime proof misses a changed card class.
🔴 Blocker
[MED] Cover mana-value-derived token P/T through the production path. Evidence: crates/engine/src/game/casting_tests.rs:36344-36362 asserts Ovika's token count/Goblin/Haste but not its power or toughness; the current parse diff also changes Pure Reflection's mana-value-derived token P/T. A wrong ObjectManaValue { EventSource } binding can therefore pass. Add a Pure Reflection runtime test that asserts the created token's P/T and strengthen Ovika's characteristic assertion.
Recommendation: request changes; add the discriminating runtime coverage and re-run the current parse-diff.
…e cast pipeline
Strengthen the Ovika runtime test to assert each created token is exactly
1/1, red, and a Phyrexian Goblin, so a count/P-T axis confusion in the
ObjectManaValue { EventSource } binding can no longer pass.
Add a Pure Reflection runtime module driving the production cast/trigger
path: the created Reflection token P/T must equal the triggering creature
spell mana value, covering the P/T axis of the same of-form anaphor. The
second case casts a {1}{R} spell funded with red + colorless mana so
mana-value summation over colored pips is exercised (CR 202.3).
Also correct the mana-pool funding comment citation: 601.2f is total-cost
determination; funding/payment are 601.2g-h.
|
Pushed 1d5376e addressing the remaining review items:
Verification: |
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/engine/src/game/casting_tests.rs`:
- Around line 36400-36407: Update the rules citation in the Pure Reflection test
comment from CR 107.4 to CR 107.3, leaving the existing ObjectScope::EventSource
mapping and all other explanatory text unchanged.
🪄 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: dfc43845-239e-472d-9eca-63c7db6d9807
📒 Files selected for processing (1)
crates/engine/src/game/casting_tests.rs
| /// CR 107.4 + CR 202.3 + CR 603.2 (issue #1718): Pure Reflection — runtime | ||
| /// cast-pipeline coverage for the P/T axis of the mana-value of-form anaphor. | ||
| /// "Whenever a player casts a creature spell, destroy all Reflections. Then | ||
| /// that player creates an X/X white Reflection creature token, where X is the | ||
| /// mana value of that spell." Ovika binds `ObjectManaValue { EventSource }` to | ||
| /// the token COUNT; Pure Reflection binds it to the token's POWER/TOUGHNESS. | ||
| /// Pinning P/T here (with count pinned at one) discriminates a count/P-T axis | ||
| /// confusion that count-only assertions cannot catch. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm which CR governs "X defined by the ability's own text" vs mana-symbol enumeration.
grep -n "^107\.3" -A2 docs/MagicCompRules.txt | head -40
grep -n "^107\.4" -A2 docs/MagicCompRules.txt | head -20
# Confirm what ObjectScope "the mana value of that spell" (of-form) actually resolves to.
rg -n "fn parse_object_prepositional_scope" -A 60 crates/engine/src/parser/oracle_nom/Repository: phase-rs/phase
Length of output: 6117
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== locate CR text =="
fd -a 'MagicCompRules.txt' . || true
fd -a 'ComprehensiveRules.txt' . || true
fd -a 'CR*.txt' docs . || true
echo "== inspect relevant CR lines if found =="
for f in $(fd -a 'MagicCompRules.txt' . 2>/dev/null; true); do
echo "--- $f"
grep -n "^107\.[34]" -A4 -B2 "$f" | head -60
done
echo "== inspect object mana value resolver =="
rg -n "resolve_object_mana_value|ObjectManaValue|Demonstrative|that spell's mana value|mana value of that spell" crates/engine/src -A40 -B20
echo "== inspect surrounding parser for object prepositional scope =="
sed -n '4538,4572p' crates/engine/src/parser/oracle_nom/quantity.rs
echo "== inspect the exact test/comment location =="
sed -n '36384,36416p' crates/engine/src/game/casting_tests.rsRepository: phase-rs/phase
Length of output: 50371
Replace the CR 107.4 citation crates/engine/src/game/casting_tests.rs:36400-36407 — this clause is about X being set from the spell’s mana value, so CR 107.3 is the relevant rule; CR 107.4 is the mana-symbol rule. The ObjectScope::EventSource mapping for that spell already matches the shared prepositional branch.
🤖 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/game/casting_tests.rs` around lines 36400 - 36407, Update
the rules citation in the Pure Reflection test comment from CR 107.4 to CR
107.3, leaving the existing ObjectScope::EventSource mapping and all other
explanatory text unchanged.
Source: Path instructions
|
Maintainer acknowledgement: rechecked current head 1d5376e after the latest follow-up. The reviewed two-line CR 107.3 citation correction is preserved in a local maintainer fixup, but is blocked from publication because the authorized fork-push GH_TOKEN is unavailable. No further author action is requested. |
2 participants
fix(parser): bind "the mana value of that spell" of-form (Ovika, Enigma Goliath)
Closes #1718
Bug
Ovika, Enigma Goliath never creates its Goblin tokens. Its trigger —
"Whenever you cast a noncreature spell, create X 1/1 red Phyrexian Goblin
creature tokens, where X is the mana value of that spell. They gain haste until
end of turn." — fires correctly, but the token-creation effect silently lowers
to
Effect::Unimplemented, so zero tokens are produced (the exact reportedsymptom).
Root cause
parse_object_mana_value_ref(crates/engine/src/parser/oracle_nom/quantity.rs:2945)is the shared recognizer for object mana-value quantity references. It handled
the possessive front-form
"that spell's mana value"→ObjectManaValue { scope: EventSource }(viaparse_object_possessive_scope, line 4535), but theprepositional of-form
"the mana value of that spell"— Ovika's actualprinted wording — was only accepted when it named a
targetobject (yieldingTargetObjectManaValue). For a non-target anaphor the?onparse_target_with_syntax_target_keywordpropagated an error out of the wholefunction, so the quantity failed to bind.
Consequently the token clause's
where X is …binder(
token.rs→parse_where_x_quantity_expression→parse_cda_quantity) gotNone, and per its own honest-failure contract the entirecreate X … tokensclause dropped to
Effect::Unimplementedrather than fabricating a deadcount-0 placeholder.
Verified directly before/after the fix:
parse_cda_quantity("the mana value of that spell")returnedNoneonmainand now returns
ObjectManaValue { scope: EventSource }; the full Ovika parse'strigger effect went from
UnimplementedtoToken { name: "Phyrexian Goblin", 1/1, Red, count: ObjectManaValue { EventSource } }.Fix
Accept the non-target prepositional of-form by mirroring the possessive scope
mapping.
parse_object_mana_value_refnow consumes an optional leading"the "(like the sibling
parse_cost_paid_object_prepositional_ref) and, when thetargeted path does not match, falls back to a new
parse_object_prepositional_anaphor_scopecombinator(
quantity.rs:3017):that spell/the triggering spell→EventSource,that creature/that permanent/that planeswalker→Target,this …/~→Source. The targetedTargetObjectManaValuepath is still tried first,so
"mana value of target creature"is unchanged. No runtime change was needed—
ObjectManaValue { EventSource }already resolves to the triggering spell on aSpellCasttrigger (same scope the possessive form produced).Anchored on
parse_object_possessive_scope(crates/engine/src/parser/oracle_nom/quantity.rs:4535)— the possessive front-form scope table (
"that spell's"→EventSource,"that creature's"→Target, …). The new of-form combinator reproduces thisexact mapping so both surface phrasings bind identically.
parse_cost_paid_object_prepositional_ref(quantity.rs:3094) — the existingsibling that reads
"[the] mana value of the <participle> <noun>"as theprepositional mirror of a possessive cost-paid ref; the optional-
"the "handling and "prepositional mirrors possessive" pattern follow it.
parse_object_color_of_scope(quantity.rs:4556) — an existing of-formanaphor→
ObjectScopecombinator (for color references) with the samethat spell → EventSource/that creature → Targetmapping, confirming thisanaphor-scope shape is the idiomatic building block here.
Regression tests (RED before, GREEN after)
crates/engine/src/game/casting_tests.rs:36295(
mod ovika_noncreature_spell_token_trigger), mirroring the sibling Namorcast-trigger tests:
casting_mana_value_three_spell_creates_three_goblins— Ovika on thebattlefield, cast a mana-value-3 noncreature spell, resolve the stack:
exactly 3 tokens, each a red Goblin that has haste. Pre-fix:
0 tokens.
token_count_tracks_spell_mana_value— control: a mana-value-2 spell makesexactly 2 tokens, proving the count reads the spell's mana value rather than
a fixed number. Pre-fix: 0 tokens.
crates/engine/src/parser/oracle_nom/quantity.rs:10730(
mana_value_of_form_mirrors_possessive_scope): each of-form phrase binds thesame
ObjectManaValuescope as its possessive counterpart, plus a negativecontrol that
"mana value of target creature"still routes toTargetObjectManaValue.RED confirmed by stashing only the
quantity.rsproduction change and re-runningthe runtime tests on the unmodified base: both failed with
got 0tokens.Validation (full CI-parity gate, all green)
cargo fmt --all -- --check— cleancargo clippy --workspace --all-targets -- -D warnings— clean (incl.engine-wasm/draft-wasm)cargo test -p engine— lib 17568 passed / 0 failed / 6 ignored;integration 3835 passed / 0 failed / 2 ignored
./scripts/check-parser-combinators.sh—Gate G PASS+Gate A PASS head=b22369f8a567c4adf62e92716fcfa873beccc146 base=70e4f2d77e0fd8de000c97bc2718924cb2505515./scripts/check-legacy-quantity-callsites.sh— exit 0origin/main(70e4f2d, up to date).Gate A
Model: Claude Opus 4.8
Tier: scoped bugfix (single parser combinator; new enum-free
alt-composed combinator, no runtime change)Summary by CodeRabbit
Bug Fixes
Tests