ENGINE: Support Court of Ambition - #7261
Conversation
Court of Ambition's monarch rider silently did nothing. The first sentence
("each opponent loses 3 life unless they discard a card") already worked, but
the second ("If you're the monarch, instead each opponent loses 6 life unless
they discard two cards") lowered to
Effect::Unimplemented { name: "Unsupported unless clause" }.
Root cause: two hand-written mirrors of the same grammar drifted.
parse_unless_discard_cost (the "you" payer) had grown a numeric-count axis;
parse_unless_they_discard_cost (the anaphoric-player payer) had not — it
hard-coded count: 1 and only accepted the singular article. Every card printing
a plural discard against a "they" / "that player" payer fell out of the grammar
entirely.
Collapse both into one authority, parse_unless_discard_cost_phrase, with the
count and type axes composed rather than enumerated:
[<count> | "a" | "an"] [<type phrase>] ("card" | "cards") ["at random"]
so "discard two nonland cards" needs no new arm and the two payer forms cannot
diverge again. parse_number spans both the numeral and the article, so one call
covers the whole count axis.
Two deliberate calls, both documented at the authority:
* CR 118.12a — fail closed on a zero count. parse_number folds a bare X to 0,
and a zero-card unless-cost is free, so a punisher would silently never fire.
An unresolvable count stays visible as an unsupported clause instead.
* CR 701.9b — an "at random" tail is accepted but deliberately does NOT set
CardSelectionMode::Random. The resolution-time unless-payment path
(engine_payment_choices.rs) discards the selection field and always prompts,
so emitting Random would claim behavior the engine does not implement. This
preserves both mirrors' pre-existing mapping exactly.
The runtime plumbing this card needs already existed: the per-opponent
player_scope fan-out, the ScopedPlayer unless-payer arm, and apply_instead_swap
preserving player_scope / unless_pay across the CR 614.15 self-replacement swap.
Court of Ambition now parses to BecomeMonarch on the ETB plus an upkeep trigger
whose base branch is LoseLife 3 / Discard 1 and whose
ConditionInstead { IsMonarch } rider is LoseLife 6 / Discard 2, both scoped
Opponent with a ScopedPlayer payer. No Unimplemented, no parse warnings.
Tests: four parser building-block tests (count x type axes across both payers,
the "or"-chain branch boundary, the zero-count fail-closed guard, and the full
Court of Ambition AST) and seven runtime tests driving the real upkeep trigger —
decline, pay, monarch decline (6, not 3 and not 9), monarch pay, unpayable with
one card in hand, a three-player game where one opponent pays and another
declines independently, and the ETB monarch grant via an actual cast.
Every CR citation verified against docs/MagicCompRules.txt.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
|
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 (1)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThe parser now shares discard-unless cost parsing for both payer forms. Tests cover discard counts, card types, unsupported random-discard phrases, invalid counts, chained branches, and Court of Ambition resolution. ChangesDiscard-unless parsing and Court of Ambition
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant OracleText
participant OracleTriggerParser
participant GameEngine
participant Opponent
OracleText->>OracleTriggerParser: parse discard-unless trigger
OracleTriggerParser->>GameEngine: create chosen discard cost
GameEngine->>Opponent: present scoped discard prompt
Opponent->>GameEngine: discard cards or decline
GameEngine-->>Opponent: apply discard or life-loss branch
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: 3
🧹 Nitpick comments (1)
crates/engine/src/parser/oracle_trigger.rs (1)
3362-3382: 🎯 Functional Correctness | 🔵 Trivial | 💤 Low valueReplace the
count as i32cast with a checked conversion.
parse_numberreturns an unsigned count from untrusted Oracle text. Theascast wraps silently for any value abovei32::MAXand produces a negativeQuantityExpr::Fixed. Usei32::try_from(count).ok()?so an out-of-range numeral fails closed, consistent with the zero-count arm directly above.♻️ Proposed change
- let discard = |filter| AbilityCost::Discard { - count: QuantityExpr::Fixed { - value: count as i32, - }, + let value = i32::try_from(count).ok()?; + let discard = |filter| AbilityCost::Discard { + count: QuantityExpr::Fixed { value }, filter,As per path instructions: "
as-casts or unchecked conversions at trust boundaries" are findings.🤖 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/parser/oracle_trigger.rs` around lines 3362 - 3382, Replace the unchecked count as-cast in the discard closure with a checked i32 conversion using i32::try_from(count).ok()? so out-of-range Oracle numerals return None. Preserve the existing zero-count rejection and construct QuantityExpr::Fixed only from the validated value.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/engine/src/parser/oracle_trigger_tests.rs`:
- Around line 13399-13408: Add a positive reach-guard in
unless_discard_cost_phrase_rejects_zero_count that directly verifies
parse_number returns the zero count and the correct remainder for the tested
input, adjusting the remainder to parse_number’s actual contract. Keep both
existing negative parser assertions so the test proves the count == 0 guard
rejects the phrase after reaching that code path.
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 3384-3397: Update the untyped-noun handling around the cards/card
parser so the “at random” branch succeeds only when the remaining text is
exhausted aside from trailing whitespace or punctuation, rejecting tails such as
additional effects. Preserve random-discard semantics for “unless you discard a
card at random”: do not convert these costs to CardSelectionMode::Chosen, and
fail closed until the unless-cost resolver supports CardSelectionMode::Random.
In `@crates/engine/tests/integration/court_of_ambition.rs`:
- Around line 219-239: Add an opponent-monarch integration test alongside
court_of_ambition_monarch_branch_demands_two_cards_and_drains_six using
build_runner(2, Some(P1), 3). After upkeep, assert P1 receives a one-card
discard prompt; decline the cost, advance the stack, and assert P1 loses three
life, confirming the branch depends on the ability controller being monarch.
---
Nitpick comments:
In `@crates/engine/src/parser/oracle_trigger.rs`:
- Around line 3362-3382: Replace the unchecked count as-cast in the discard
closure with a checked i32 conversion using i32::try_from(count).ok()? so
out-of-range Oracle numerals return None. Preserve the existing zero-count
rejection and construct QuantityExpr::Fixed only from the validated value.
🪄 Autofix
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: 3f18f0bf-1fce-4705-a32e-f1f7ae928f50
📒 Files selected for processing (4)
crates/engine/src/parser/oracle_trigger.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/court_of_ambition.rscrates/engine/tests/integration/main.rs
|
Generated for head Parse changes introduced by this PR · 5 card(s), 6 signature(s) (baseline: main
|
|
Maintainer fixup applied; holding for current-head evidence. I pushed |
56ab02a to
cdc0586
Compare
|
Maintainer fixup refreshed; holding for current-head evidence. The final maintainer-fixup head is |
…nto claude/court-of-ambition-cbd3aa
|
Maintainer test fixup applied; holding for current-head evidence. The new head c3a6ad7 updates the stale Balduvian Horde regression to assert intentional fail-closed handling of random discard rather than player-chosen lowering. The prior head failed Rust test shard 1 solely because the old assertion contradicted that behavior. Current-head CI and the coverage parse-diff artifact have not published yet. I will recheck this exact head when they settle; if the parser/runtime evidence is clean, it will proceed to approval and the merge queue. |
…nto claude/court-of-ambition-cbd3aa
|
Current-head evidence pending. The current head is cf10111. Manual re-review found the earlier parser and runtime findings resolved on this tree, but required CI is still pending and the only coverage parse-diff comment identifies prior head c3a6ad7. I will resume approval and merge-queue handling only after the checks and a parse-diff artifact bound to cf10111 settle. |
Heads-up on the coverage delta: Balduvian Horde is an intentional dropThe It comes from Why fail closed is right here. CR 701.9b distinguishes a random discard from a player-selected one, and the unless-payment path ( This also matches how the rest of the engine already treats random discard as a cost rather than as an effect.
So the pre-existing lenient parser mapping was the outlier, and this PR brings the parser in line with the cost layer. Scope note. Court of Ambition itself has no random discard anywhere in it — its clauses are Tracked. Follow-up to restore the class correctly rather than by loosening the mapping again: teach the cost layer to honor |
|
Current-head parse evidence pending. Required CI is now green for |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review clean: shared unless-discard grammar preserves Court of Ambition’s per-opponent, monarch-replacement behavior and fails random-discard costs closed until their resolver supports random selection.
2 participants
Implements Court of Ambition (CMR,
{2}{B}{B}Enchantment).Oracle text, verbatim:
What was broken
The first sentence already worked end to end. The monarch rider did not — it lowered to
Effect::Unimplemented { name: "Unsupported unless clause" }, so the monarch half of the card silently did nothing.Root cause is two hand-written mirrors of the same grammar that drifted apart.
parse_unless_discard_cost(theyoupayer) had grown a numeric-count axis;parse_unless_they_discard_cost(the anaphoric-player payer) had not — it hard-codedcount: 1and only accepted the singular article. Every card printing a plural discard against athey/that playerpayer fell out of the grammar entirely.The fix
Collapse both into one authority,
parse_unless_discard_cost_phrase, with the count and type axes composed rather than enumerated:parse_numberalready spans both the numeral and the singular article, so one call covers the whole count axis. The two payer forms now share one vocabulary and cannot diverge again, and"discard two nonland cards"composes for free with no new arm.Two deliberate calls, both documented at the authority:
parse_numberfolds a bareXto0, and a zero-card unless-cost is free, so a punisher would silently never fire. An unresolvable count now stays visible as an unsupported clause rather than lowering to a cost every player can always pay. This also tightens the pre-existingyouform."at random"is accepted as a tail but deliberately does NOT setCardSelectionMode::Random. The resolution-time unless-payment path (engine_payment_choices.rs) discardsselectionand always prompts, so emittingRandomwould claim behavior the engine does not implement. Preserves both mirrors' pre-existing mapping exactly.No runtime changes were needed — the per-opponent
player_scopefan-out, theScopedPlayerunless-payer arm, andapply_instead_swappreservingplayer_scope/unless_payacross the CR 614.15 self-replacement swap all already existed.Resulting parse
BecomeMonarchon the ETB, plus an upkeep trigger:LoseLife 3OpponentDiscard 1ScopedPlayerConditionInstead { IsMonarch }LoseLife 6OpponentDiscard 2ScopedPlayerNo
Unimplemented, no parse warnings.Tests
Four parser building-block tests:
" or …"branch for the disjunction combinatorConditionInsteadswap, not an additive subSeven runtime tests in
crates/engine/tests/integration/court_of_ambition.rsdriving the real upkeep trigger:Verification
cargo fmt --all --checkcleancargo clippy --all-targets -- -D warningsexit 0Every CR citation was verified against
docs/MagicCompRules.txt.🤖 Generated with Claude Code
Summary by CodeRabbit