fix(engine): recognize bare (unlabeled) Strive-shaped cost surcharge (Fireball)

[rykerwilliams]

Summary

Fixes a parser misparse in docs/parser-misparse-backlog.md's Category #5 ("Dropped 'for each' / dynamic count collapsed to Fixed", 333 cards).

[[Fireball]] ({X}{R}, Sorcery, Limited Edition Alpha 1993 — verified via Scryfall): "This spell costs {1} more to cast for each target beyond the first.\nFireball deals X damage divided evenly, rounded down, among any number of targets."

The first line is a genuine CR 601.2f cost increase — mechanically identical to the "Strive" ability word (CR 207.2c, introduced in Journey into Nyx 2014, purely flavor-text with no independent rules meaning). parse_strive_cost_line already handles this cost shape fully and correctly for ~17 Strive-labeled cards, but it unconditionally requires an em-dash "Strive — " label before attempting the cost-pattern match. Fireball's clause predates the label by 21 years, so it's bare — the function returns None immediately, and the clause is silently dropped (a separate, unrelated catch-all for lines starting with "this spell costs " suppresses any Unimplemented warning, so the bug produces zero signal — exactly the "looks done, is wrong" class this backlog exists to catch).

[[Officious Interrogation]] (Murders at Karlov Manor, 2024 — verified via Scryfall) has the identical bare, unlabeled shape: "This spell costs {W}{U} more to cast for each target beyond the first.\nChoose any number of target players. Investigate X times..." — printed nine years after Strive existed, WotC simply chose not to apply the label. This confirms the bare form is a real, recurring grammatical variant of the mechanic, not a one-off pre-2014 artifact, so the fix is scoped to the class, not just Fireball.

Fix

parse_strive_cost_line now tries the labeled form first (unchanged behavior for existing Strive cards), and on no label, falls back to running the identical nom pipeline directly on the un-stripped line (factored into a shared parse_strive_cost_body helper so neither path duplicates the cost-matching logic). Every downstream consumer of the resulting strive_cost: Option<ManaCost> (IR lowering, CardFace/GameObject field-copy, and game/casting.rs's apply_target_dependent_cost_modifiers/concrete_cost_for_x) already consumes it generically regardless of population path, so the parser function is the sole fix point — confirmed by direct trace, not assumption.

Important scope note — Fireball's own gameplay cost is not fully fixed by this PR

While implementing this, I found a separate, independent engine bug: Fireball's specific casting route ({X} cost + "divided evenly among any number of targets", which pauses at a pre-payment DistributeAmong step) bypasses the cost-surcharge mechanism entirely — the total cost gets locked in back at ChooseXValue time, before targets are known, and is never re-derived once targets are actually chosen. So even with this parser fix, Fireball's actual in-game cost still won't scale with target count — that's a shared casting-pipeline bug (affects ~15 cards' payment-mode/convoke/assist access on the {X} + distribute-among-targets route, not just Fireball's surcharge), scoped to its own follow-up rather than this PR. The runtime discriminating test here uses Officious Interrogation instead, whose casting route does reach the surcharge seam correctly, to prove the parser fix reaches real production payment for at least one card in the class.

Files changed

• crates/engine/src/parser/oracle.rs — the fix
• crates/engine/src/parser/oracle_tests.rs — 3 new unit tests (Fireball's real text, Officious Interrogation's real text as class-coverage proof, a precision guard against over-matching)
• crates/engine/tests/integration/officious_interrogation_bare_strive_surcharge.rs (new) — discriminating runtime test driving the real cast pipeline
• crates/engine/tests/integration/main.rs — registers the new test module
• docs/parser-misparse-backlog.md — list hygiene: removes Fireball, Officious Interrogation, and Ajani's Presence (a Strive-labeled card also listed in this category — confirmed via direct test execution, not inference, that it already parses correctly today; a stale entry unrelated to this fix)

CR references

CR 207.2c, CR 601.2c, CR 601.2f

Verification

• Verified both cards' Oracle text via Scryfall (not paraphrases)
• cargo fmt --all clean
• cargo clippy -p engine --lib -- -D warnings clean
• ./scripts/check-parser-combinators.sh (Gate A): 0 violations
• cargo test -p engine --lib: 15976 passed, 0 failed
• cargo test -p engine --test integration: 2569 passed, 0 failed
• Live-revert-and-rerun: reverting the bare-fallback confirms the new unit + integration tests fail; restored and re-confirmed green
• Two rounds of independent plan review (closed: a fabricated citation, a by-analogy-only stale-backlog claim later confirmed via direct execution, and an under-specified test design) plus a review-impl pass (closed: a leftover debug test, an Instant/Sorcery type-line mismatch on Officious Interrogation)

Co-Authored-By: Claude Sonnet 5 noreply@anthropic.com

https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K

[gemini-code-assist]

Warning

You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again!

[matthewevans]

Thanks for this — the bare (unlabeled) Strive-shaped surcharge is a real parser gap and the seam looks right at a glance.

This branch is currently in conflict with main (mergeStateStatus: DIRTY), so it can't be auto-updated — the conflict is on shared surfaces that move constantly:

• crates/engine/tests/integration/main.rs — the explicit mod …; registration list (a frequent concurrent-edit collision point)
• docs/parser-misparse-backlog.md

Could you rebase onto the latest main and resolve those two, then push? Once it's CLEAN and CI is green I'll do the full engine review (parser combinator seam, CR annotation, and the discriminating test).

[rykerwilliams]

Bonus finding, verified directly: while working on a separate follow-up fix (the {X}+distribute casting-route timing bug, unrelated files), I confirmed this PR's fix also incidentally resolves a second, previously-undocumented misparse for Fireball's exact clause.

On origin/main (before this PR), parsing Fireball's full real Oracle text produces a spurious StaticDefinition::ModifyCost { mode: Raise, amount: {1}, dynamic_count: Some(ObjectCount{filter: Typed(TypedFilter{type_filters: [], controller: None, properties: []})}) } — the "costs {1} more to cast for each target beyond the first" line gets caught by a separate static-cost-modifier classifier that loses "target"/"beyond the first" entirely and replaces it with an unrestricted (empty-filter) object count, i.e. "costs {1} more for each [any object]" — a much worse-looking bug than the silent drop I originally diagnosed.

Verified this PR's fix (parse_strive_cost_line's bare-form recognition) suppresses it: re-parsed the identical text on this branch and got statics: [] (empty) with strive_cost correctly populated — the pre-existing "Priority 8c-strive" line-skip mechanism (oracle.rs, gated on parse_strive_cost_line(&line).is_some()) prevents the same line from also being classified as a static ability, once parse_strive_cost_line recognizes it. No additional code change needed here; noting it for the record since it wasn't part of my original PR description's claimed scope.

[matthewevans]

Maintainer rebase — I've pushed your branch onto current main. No action needed from you, and nothing about your fix changed.

Heads-up that I force-pushed fix/fireball-for-each-cost (8158762374 → 405a376084), since you'd otherwise have had to untangle this yourself.

What was wrong

The branch had no merge base with main at all — git saw 5754 commits "ahead," meaning its history was unrelated to upstream (an orphan branch, or a fork re-created after a history rewrite). That's why it showed CONFLICTING and why a plain rebase couldn't work: there was no common ancestor to rebase from.

What I did

Cherry-picked your one real commit onto current main. Everything in it applied cleanly — parser/oracle.rs, oracle_tests.rs, and the new integration test came across untouched, and your authorship is preserved on the commit.

The only conflict was in docs/parser-misparse-backlog.md, and only on the auto-generated summary counters (your base said 4773 distinct cards, main now says 4760). Your commit never touched those lines — it was pure context drift. I kept main's counters and re-applied your actual edit: Category #5 dropped 333 → 330, with Fireball, Officious Interrogation, and Ajani's Presence removed. That hygiene is intact.

Verified before pushing

Built against current main, then ran the tests that actually discriminate:

• officious_bare_strive_surcharge_adds_wu_per_extra_target — pass (the runtime proof the surcharge reaches real payment)
• 14 parser/casting unit tests — pass, including the pre-existing labeled-Strive ones (strive_ability_word_strip, strive_cost_parsed_from_oracle_text, strive_surcharge_with_two_targets, …)

That last group is the one I cared about: splitting parse_strive_cost_line into a labeled entry point plus a shared parse_strive_cost_body is exactly the right seam, and those tests confirm the ~17 existing labeled Strive cards didn't regress on the way.

Your writeup was unusually good — in particular, calling out that Fireball's own runtime cost is not fixed here (its {X} + DistributeAmong route bypasses the surcharge seam via a separate engine bug) and choosing Officious Interrogation as the runtime test because its casting route actually reaches the seam. Scoping the discriminating test to the path the fix genuinely touches, and saying so out loud, is the thing most PRs get wrong.

Letting CI run now; I'll approve and enqueue once it's green.

For next time

To avoid the no-merge-base problem, re-point your fork at upstream:

git remote add upstream https://github.com/phase-rs/phase.git   # if not already
git fetch upstream
git checkout -B main upstream/main && git push --force origin main
# then branch from main for new work

[github-actions]

Parse changes introduced by this PR · 2 card(s), 1 signature(s) (baseline: main 400a18223a92)

2 card(s) · static/RaiseCost · removed: RaiseCost (affects=self)

Examples: Fireball, Officious Interrogation

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Approved. Rebased onto current main by maintainer (see above); 13/13 checks green on the rebased head 405a376084.

✅ Clean

• Right seam. Splitting parse_strive_cost_line into a labeled entry point plus a shared parse_strive_cost_body fixes the class, not the card: the em-dash Strive — label was an unconditional requirement, and Fireball's clause predates the ability word by 21 years. Officious Interrogation (MKM, 2024) has the identical bare shape, which is the proof this is a recurring grammatical variant rather than a pre-2014 artifact.
• Scope is exactly the claim. Parse-diff (baseline 400a18223a92): 2 cards, 1 signature — Fireball and Officious Interrogation, nothing else. The removed RaiseCost (affects=self) static corroborates the diagnosis: the bare clause was being swallowed into a bogus self-targeting static instead of a strive_cost, with no Unimplemented signal to flag it. Textbook "looks done, is wrong."
• Discriminating runtime test at the right seam. officious_bare_strive_surcharge_adds_wu_per_extra_target proves the parsed cost reaches real production payment. And the pre-existing labeled-Strive tests still pass, which is the evidence that the ~17 existing labeled Strive cards didn't regress on the way through.
• Honest scoping. Calling out that Fireball's own runtime cost is not fixed here — its {X} + DistributeAmong route bypasses the surcharge seam via a separate engine bug — and then picking Officious Interrogation as the runtime test because its casting route actually reaches the seam. Choosing the test card by which path the fix genuinely touches, and saying so in the commit message, is the part most PRs get wrong.

Enqueueing.