fix(parser): parse "Activate only during an opponent's upkeep" (Trade…#6284
fix(parser): parse "Activate only during an opponent's upkeep" (Trade…#6284keloide wants to merge 2 commits into
Conversation
… Caravan)
Trade Caravan's activated ability "Remove two currency counters from ~:
Untap target basic land. Activate only during an opponent's upkeep." dropped
its timing restriction to `Effect::Unimplemented`, leaving the ability
activatable at any time.
`parse_activation_during_role_gate` handled "during an opponent's turn" and
"during your upkeep" but not "during an opponent's upkeep". Rather than add a
`DuringOpponents*` sibling to `ActivationRestriction` (which expresses
opponent-turn scope via `RequiresCondition { Not(IsYourTurn) }`, not a
dedicated variant), compose from building blocks:
- Add `ParsedCondition::IsDuringUpkeep` (CR 503.1) — a scope-free upkeep-step
predicate, orthogonal to every existing `ParsedCondition` variant.
- Map "during an opponent's upkeep" to
`RequiresCondition { And([Not(IsYourTurn), IsDuringUpkeep]) }` (CR 602.5b +
CR 102.1 + CR 503.1), reusing the existing opponent-turn composition idiom.
- Evaluate `IsDuringUpkeep` against `state.phase == Phase::Upkeep`; classify it
memo-safe in the AI condition filter (reads only apply()-constant state).
Removes Trade Caravan from the parser-misparse backlog (root cause 28).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01HwvJgpQpXqutChWLLLGB8E
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for the 'Activate only during an opponent's upkeep' restriction (specifically for the card Trade Caravan) by introducing the ParsedCondition::IsDuringUpkeep condition, updating the condition evaluator, adding parser support, and updating the misparse backlog. The review feedback highlights a violation of Rule R1, where verbatim string matching is used in the parser instead of decomposing the phrase into modular, reusable nom combinators.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| value( | ||
| opponents_upkeep_activation_restriction(), | ||
| alt(( | ||
| tag("during an opponent's upkeep"), | ||
| tag("during an opponents upkeep"), | ||
| )), | ||
| ), |
There was a problem hiding this comment.
Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead of adding verbatim string variants to the alt combinator, decompose compound phrases into modular, reusable parsers for constituent parts (e.g., the preposition, determiner, subject, and phase) and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
value(
opponents_upkeep_activation_restriction(),
preceded(
tag("during "),
tuple((
alt((tag("an"), tag("each"))),
tag(" "),
alt((tag("opponent's"), tag("opponents"))),
tag(" upkeep"),
)),
),
),References
- L2. Sibling coverage: If a parser arm or string was extended, ensure plural, possessive, negated, and other variants are covered. (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] This engine-mechanic change has no verifiable higher-level frontier-model planning provenance. Evidence: the PR claims /engine-implementer but records only generic Claude Code generation, with no planning-model identity or frontier-level review evidence. Why it matters: maintainer policy requires a higher-level frontier model for planning any engine mechanic; an unverified inline claim cannot satisfy that gate. Suggested fix: redo the planning/review cycle with the required frontier model and attach the model provenance, current-head Gate A, and final review-impl result.
[MED] The runtime test bypasses the production activation gate. Evidence: crates/engine/src/game/restrictions.rs:3099-3144 constructs ParsedCondition and calls evaluate_condition directly, while real activation legality flows through activation_restriction_applies at restrictions.rs:994-996 and check_activation_restrictions. Why it matters: the test proves only the new leaf evaluator, not that Trade Caravan's parsed ActivationRestriction::RequiresCondition blocks/permits the real activation path. Suggested fix: drive check_activation_restrictions (or the scenario activation pipeline) with Trade Caravan's parsed restriction for opponent-upkeep allowed, opponent-non-upkeep denied, and controller-upkeep denied cases.
[MED] The new Oracle phrase is encoded as two full-string alternatives instead of composed grammar. Evidence: crates/engine/src/parser/oracle.rs:6785-6789; the existing casting parser decomposes the same possessive axis in oracle_casting.rs:684-715. Why it matters: adding a second full phrase makes sibling forms accumulate as cartesian strings rather than sharing the opponent-possessive and phase axes. Suggested fix: compose during , the opponent possessive variants, and upkeep with nested nom combinators, then cover both supported possessive spellings.
Parse changes introduced by this PR · 1 card(s), 1 signature(s) (baseline: main
|
|
@coderabbitai review |
✅ Action performedReview finished.
|
📝 WalkthroughWalkthroughAdds an ChangesOpponent Upkeep Activation
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant RulesText
participant OracleParser
participant ActivationRestriction
participant RestrictionEvaluator
participant GameState
RulesText->>OracleParser: parse opponent-upkeep activation timing
OracleParser->>ActivationRestriction: create Not(IsYourTurn) and IsDuringUpkeep
ActivationRestriction->>RestrictionEvaluator: evaluate composed restriction
RestrictionEvaluator->>GameState: read turn and phase
GameState-->>RestrictionEvaluator: current scope and phase
RestrictionEvaluator-->>ActivationRestriction: allow or deny activation
Suggested labels: 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.rs`:
- Around line 6784-6790: Update opponents_upkeep_activation_restriction so
“opponent” is determined by team membership rather than comparing the activator
directly with state.active_player; use the existing active-team/opponent
predicate and preserve both accepted upkeep phrases. Add a Two-Headed Giant
regression case covering a non-active teammate during that team’s upkeep, and
annotate the rule implementation with the verified applicable Comprehensive
Rules citation and description.
In `@docs/parser-misparse-backlog.md`:
- Around line 5074-5076: The summary table’s entry for root cause 28 is stale.
Update its ranked count from 16 to 13 to match the heading “Trigger/activation
timing or ordinal restriction dropped,” without changing the heading or other
prioritization data.
🪄 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: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 46c9725e-90c8-46ba-bb54-04765abab270
📒 Files selected for processing (6)
crates/engine/src/ai_support/filter.rscrates/engine/src/game/restrictions.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/types/ability.rsdocs/parser-misparse-backlog.md
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the new head still has a team-scope rules defect, and the original parser and runtime-test gaps remain.
🔴 Blocker
[HIGH] “Opponent’s upkeep” treats a teammate as an opponent in shared-team turns. Evidence: crates/engine/src/parser/oracle.rs:7027-7034 emits Not(IsYourTurn), while crates/engine/src/game/restrictions.rs:1520-1523 defines IsYourTurn solely as active_player == player; in Two-Headed Giant, a non-active teammate can act during its team’s upkeep, so that composition becomes true even though teammates are not opponents. The engine’s team-aware authority is crates/engine/src/game/players.rs:169-172 → topology::is_opponent. Why it matters: the ability is illegally activatable during its controller’s teammate’s upkeep. Suggested fix: replace the active-player inequality representation for the “opponent’s turn” class with a team-aware condition at the ParsedCondition/restriction evaluator seam, migrate the existing opponent-turn parser users, and add a 2HG production-gate regression that denies the teammate case while allowing the opposing team’s upkeep.
[MED] The parser still encodes the phrase as two complete strings. Evidence: crates/engine/src/parser/oracle.rs:6783-6790; the existing composed possessive/phase grammar is in crates/engine/src/parser/oracle_casting.rs:684-717. Why it matters: apostrophe and phase axes will accumulate as full-string permutations instead of sharing grammar. Suggested fix: compose during , the opponent-possessive parser, and upkeep using the shared nom building block (extract it to the appropriate parser-combinator authority if necessary).
[MED] The new runtime test bypasses the activation-restriction production entry. Evidence: crates/engine/src/game/restrictions.rs:3099-3145 calls evaluate_condition directly, whereas check_activation_restrictions reaches activation_restriction_applies at crates/engine/src/game/restrictions.rs:542-557, 907-997. Why it matters: it does not prove Trade Caravan’s parsed RequiresCondition gates a real activation. Suggested fix: parse the card restriction and test check_activation_restrictions for opposing-upkeep allow, own-upkeep deny, opposing non-upkeep deny, and the 2HG teammate-upkeep deny case.
🟡 Non-blocking
The unresolved CodeRabbit documentation finding is valid: the root-cause table still says 16 while the updated heading says 13 (docs/parser-misparse-backlog.md:42,5074). Update the table in the same follow-up.
Recommendation: request changes. This needs a small but cross-cutting rules-model correction and production-path coverage; it is not safe to enqueue or to patch as a one-line maintainer fixup.
|
@coderabbitai full review |
✅ Action performedFull review finished. |
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/parser/oracle.rs`:
- Around line 6784-6790: Update opponents_upkeep_activation_restriction() to
compose the phrase from existing parsers for the “during” preposition, opponent
player role, and upkeep step instead of matching complete Oracle strings with
tag arms. Preserve support for both possessive spellings through the typed role
grammar and remove the special-case full-clause alternatives.
🪄 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: 946abb8c-07bc-4ddc-9597-e3d9a93fd8f6
📒 Files selected for processing (6)
crates/engine/src/ai_support/filter.rscrates/engine/src/game/restrictions.rscrates/engine/src/parser/oracle.rscrates/engine/src/parser/oracle_tests.rscrates/engine/src/types/ability.rsdocs/parser-misparse-backlog.md
| value( | ||
| opponents_upkeep_activation_restriction(), | ||
| alt(( | ||
| tag("during an opponent's upkeep"), | ||
| tag("during an opponents upkeep"), | ||
| )), | ||
| ), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Compose the role and step grammar instead of tagging whole clauses.
These full-clause tag arms create another special-case parser path and do not generalize possessive/role variants. Factor “during” + player role + step into the existing typed grammar building blocks.
As per coding guidelines, “Never dispatch parsing by matching verbatim Oracle strings” and “Compose nom combinators across independent dimensions instead of enumerating full-string permutations.”
🤖 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.rs` around lines 6784 - 6790, Update
opponents_upkeep_activation_restriction() to compose the phrase from existing
parsers for the “during” preposition, opponent player role, and upkeep step
instead of matching complete Oracle strings with tag arms. Preserve support for
both possessive spellings through the typed role grammar and remove the
special-case full-clause alternatives.
Sources: Coding guidelines, Path instructions
3 participants
Summary
Trade Caravan's activated ability
Remove two currency counters from ~: Untap target basic land. Activate only during an opponent's upkeep.dropped its timing restriction toEffect::Unimplemented, leaving the ability activatable at any time.parse_activation_during_role_gatehandledduring an opponent's turnandduring your upkeepbut had no arm forduring an opponent's upkeep. This adds that arm by composing existing building blocks rather than proliferating a newDuringOpponents*restriction sibling.Implementation method (required)
Method: /engine-implementer (gates applied inline:
add-engine-variantexistence/parameterization/boundary gate + mandatory finalreview-impl)CR references
CR 503.1— the upkeep step (newParsedCondition::IsDuringUpkeeppredicate + its evaluator arm).CR 102.1— the active player is the player whose turn it is (opponent scope viaNot(IsYourTurn)).CR 602.5b— an activated ability's use restriction persists across control changes.All three grep-verified against
docs/MagicCompRules.txt.Design
Rather than add a
DuringOpponentsUpkeepsibling toActivationRestriction(which already expresses opponent-turn scope viaRequiresCondition { Not(IsYourTurn) }, not a dedicatedDuringOpponentsTurnvariant), the fix composes:ParsedCondition::IsDuringUpkeep— a scope-free upkeep-step predicate, orthogonal to every existingParsedConditionvariant (there was no prior phase predicate).during an opponent's upkeep→RequiresCondition { And([Not(IsYourTurn), IsDuringUpkeep]) }, reusing the same opponent-turn composition idiom as the existing bare opponent-turn gate.add-engine-variantgate result forParsedCondition::IsDuringUpkeep: Stage 1 DOES_NOT_EXIST (no phase predicate inParsedCondition), Stage 2 EXTEND_OK (orthogonal — not a sibling cluster), Stage 3 WITHIN_SECTION (CR 503, upkeep step).Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— clean.cargo clippy -p engine --all-targets -- -D warnings— clean, 0 warnings (full compile; confirms every exhaustiveParsedConditionmatch is satisfied).cargo test -p engine --lib—ok. 17440 passed; 0 failed; 6 ignored.cargo test -p engine --test integration—ok. 3676 passed; 0 failed; 2 ignored.New tests:
activated_ability_opponents_upkeep_restriction_composes_scope_and_step(parser building block, drives productionparse(), asserts composed restriction + zeroEffect::Unimplemented) andopponents_upkeep_activation_condition_gates_on_step_and_scope(runtime, drives realevaluate_conditionacross the full turn-scope × step matrix) — both pass.cargo coverage/cargo semantic-audit— deferred to CI: both require a full MTGJSONcard-data.jsonexport (156 MB+ multi-request per-set fetch) that isn't feasible in this environment (only a test fixture is present locally). Direct evidence instead: the parsed AST for Trade Caravan now contains zeroEffect::Unimplementedand the correctRequiresCondition { And([Not(IsYourTurn), IsDuringUpkeep]) }restriction. Sincecard_face_has_unimplemented_parts(game/coverage.rs) derivessupportedprecisely from the absence ofUnimplementedparts, Trade Caravan is nowsupported: true, gap_count: 0.Gate A
Gate A PASS head=c5024c97678be3ffdcbac252e235cd8ed145fdab base=ee061e5c7485619644dc6089976b94a1d29a0202
Anchored on
crates/engine/src/parser/oracle.rs:6792—value(opponents_turn_activation_restriction(), alt((tag("during an opponent's turn"), tag("during an opponents turn")))): the samevalue(computed_restriction, alt((tag, tag)))during-role arm the new opponent's-upkeep arm mirrors (combinator family, naming, placement).crates/engine/src/parser/oracle.rs:6803—value(ActivationRestriction::DuringYourUpkeep, alt((tag("during your upkeep"), tag("during their upkeep")))): the sibling upkeep-step during-role arm, immediately adjacent, establishing the apostrophe/possessive variant pattern reused verbatim.crates/engine/src/game/restrictions.rs:1520ParsedCondition::IsYourTurn => state.active_player == playerandrestrictions.rs:1066CastingRestriction::DuringOpponentsUpkeep => state.active_player != player && state.phase == Phase::Upkeep— the evaluator arms the newIsDuringUpkeep => state.phase == Phase::Upkeeparm mirrors.Final review-impl
Final review-impl PASS head=c5024c97678be3ffdcbac252e235cd8ed145fdab
Claimed parse impact
docs/parser-misparse-backlog.mdroot cause 28)🤖 Generated with Claude Code
Generated by Claude Code
Summary by CodeRabbit
IsDuringUpkeepactivation condition predicate to support abilities restricted to an opponent’s upkeep.