fix(parser): UNSUPPORTED one-off: The Foretold Soldier - #4579
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Parse changes introduced by this PR · 91 card(s), 82 signature(s) (baseline: main
|
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] BecomesForetold grants a real zero-cost foretell permission when the card does not already have a printed Foretell keyword. Evidence: crates/engine/src/parser/oracle_effect/sequence.rs emits CastingPermission::Foretold { cost: ManaCost::zero(), ... }; crates/engine/src/game/effects/grant_permission.rs only replaces that placeholder when the object already has a printed Keyword::Foretell; and data/mtgish-cards.json shows Ethereal Valkyrie's effect grants IsForetoldForCost with "its mana cost reduced by {2}", not a printed Foretell keyword. Why it matters: the coverage bot says this PR newly supports Ethereal Valkyrie, but that card's exiled card would become castable from exile for {0} instead of its mana cost reduced by {2}. Please either parse and carry the effect-defined foretell cost into the granted permission with a discriminating Ethereal Valkyrie test, or keep that cost-bearing form unsupported with a strict failure until the cost is represented.
…ll cost form
Ethereal Valkyrie's trigger says "exile a card from your hand face down.
It becomes foretold. Its foretell cost is its mana cost reduced by {2}."
The PR's BecomesForetold continuation parsed "it becomes foretold" and
emitted CastingPermission::Foretold{cost:{0}} as a sub_ability. Since
the exiled card has no printed Keyword::Foretell, grant_permission.rs
left the zero-cost placeholder untouched — making the card incorrectly
castable from exile for free instead of its mana cost reduced by {2}.
Fix (option B — strict failure as requested by maintainer review):
- Add `is_foretell_cost_override_sentence()` in sequence.rs: nom
combinator that matches "its foretell cost is <anything>".
- In the parse_effect_chain_ir chunk loop (mod.rs), detect this sentence
and clear any immediately-preceding absorbed BecomesForetold
continuation, preventing the wrong {0} permission from being emitted.
- The sentence then falls through to Unimplemented, honestly documenting
that effect-defined foretell cost expressions are not yet supported.
- The Foretold Soldier's plain "it becomes foretold." (no cost sentence)
is unaffected: the grant is still emitted and at runtime derives cost
from the card's own printed Foretell keyword via foretell_cost().
- Add test parse_becomes_foretold_with_cost_override_suppresses_grant
verifying no GrantCastingPermission::Foretold appears in the chain
when a cost-override sentence follows.
Verified: cargo clippy -p engine -D warnings (clean),
cargo test -p engine (14 098 passed, 0 failed).
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
matthewevans
left a comment
There was a problem hiding this comment.
Approved. I re-reviewed the current head after the stale changes-requested review. The prior zero-cost foretell blocker is fixed: effect-driven “becomes foretold” now derives the printed Foretell cost at resolution, effect-defined foretell-cost text strict-fails instead of granting a bogus placeholder cost, and BecameForetold stays distinct from the Foretold special-action event. Current head CI is green.
Summary
Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED one-off: The Foretold Soldier
Cards corrected
Fix
Implemented cluster 63 (The Foretold Soldier, WHO) by recognizing "It becomes foretold" (CR 702.143d) as a post-exile continuation that reuses existing primitives — NO new gated engine variant. Found and completed a partial in-tree implementation from an earlier interrupted run.
PARSER: ast.rs adds ContinuationAst::BecomesForetold; sequence.rs adds parse_becomes_foretold_continuation (pure tag/alt/all_consuming, the one trim line carries an allow-noncombinator annotation), a classifier arm, an application arm that emits Effect::GrantCastingPermission { CastingPermission::Foretold { cost: ManaCost::zero (placeholder), turn_foretold: 0 }, target: exile_designation_grant_target(previous), grantee: ObjectOwner }, a consuming-predicate arm, and renames plotted_grant_target -> exile_designation_grant_target (now serves both plotted and foretold designations).
RESOLVER: grant_permission.rs extends the Foretold arm to set obj.face_down, derive the real cost from the object's printed Foretell keyword via casting::foretell_cost (made pub(crate)), and emit GameEvent::BecameForetold; plus a CR 608.2c guarded ParentTarget->source fallback so the anaphoric "it" (empty target slot) binds to the just-exiled source. casting.rs: foretell_cost -> pub(crate) as the single cost authority.
EVENT PLUMBING: new GameEvent::BecameForetold { object_id } in events.rs, wired into every compiler-flagged exhaustive match — trigger_index.rs (no-op key, CR 702.143c so it does NOT fire foretell triggers), trigger_matchers.rs batched-subject counter (=> 0), public_state.rs dirty-marking, and log.rs (Zone category + message). Distinct from GameEvent::Foretold (the CR 702.143a special action) by design.
TESTS (building-block level): parser unit test (Exile it face down. It becomes foretold. -> ChangeZone->Exile sub-chaining GrantCastingPermission{Foretold}, asserts no Unimplemented anywhere in the chain); resolver unit test (empty targets + source_id -> obj.foretold/face_down set, permission carries the printed {1}{G} cost and stamped turn, BecameForetold fired and Foretold NOT fired); CR 702.143c regression in trigger_matchers (match_foretell returns false for BecameForetold, true for the real Foretold special action). All pass; the cast-from-exile later-turn gate is already locked by existing tests on the identical CastingPermission::Foretold.
VERIFICATION: cargo fmt clean; clippy -p engine --all-targets -D warnings exit 0; cargo check --workspace exit 0; full engine lib suite 14083 passed / 0 failed; oracle-gen re-parse of The Foretold Soldier shows 0 Unimplemented / 0 Unknown. Parser combinator gate is RED but 100% pre-existing baseline drift (baseline ff799f8 = PR #904) across files I never touched — diffing the gate before/after my single comment reword proved zero net-new violations. All CR annotations verified against docs/MagicCompRules.txt (702.143a/c/d/e and 608.2c). No commit made.
JUDGEMENT CALLS: (1) Treated the pre-existing partial in-tree changes as my own prior work since this is a dedicated single-task worktree and they matched the approved plan exactly; completed the missing event/visibility/test pieces. (2) Categorized the log line as Zone (grouped with the analogous Foretold event) per the plan, not State where BecomesPlotted lives. (3) Proved the only NEW runtime code path with a direct resolve() state-delta test rather than a heavyweight combat GameScenario, because the cast gate is pre-tested on the same permission and ChangeZone ObjectId preservation is a pre-existing invariant; end-to-end card re-parse supplies the integration confirmation.
Files changed
CR references
Verification
cargo fmt --all— passcheck-parser-combinators.sh <upstream merge-base>— passcargo clippy -p engine --all-targets -- -D warnings— passcargo test -p engine— passoracle-gen --filter "the foretold soldier"— passCards confirmed re-parsed correctly: The Foretold Soldier
🤖 Generated with Claude Code