feat(parser): support "whenever you discover, discover again for the same value"#5515
Conversation
…same value" Curator of Sun's Creation (phase-rs#5270): "Whenever you discover, discover again for the same value. This ability triggers only once each turn." parsed to an Unknown trigger mode with an Unimplemented effect — the ability did nothing. Both the discover TRIGGER and the discover EFFECT already exist in the engine; this wires the two missing parser paths and the "same value" quantity. 1. Trigger: "whenever/when <player> discover[s]" (bare) → `TriggerMode::Discover` (matched on the `EffectResolved{Discover}` event by `match_discover`). The discover keyword action is NOT a `PlayerActionKind`, so a dedicated `is_bare_discover_subject` gate routes it before the action-list path. A compound "discover, investigate, …" subject (Val) is left to that path. 2. Effect: "discover again for the same value" → `Effect::Discover` with `mana_value_limit = QuantityRef::TriggeringDiscoverValue`. 3. `QuantityRef::TriggeringDiscoverValue` (new): the mana-value limit N of the discover that fired the current "whenever you discover" trigger. Threaded via `GameState::last_discover_value`, set in the discover resolver (discover.rs) before the EffectResolved event fires; resolves to 0 outside a discover context. Classified conservatively in the ability-scan (Axes::NONE), read/write profiler (empty — written only by discover resolution, never a sibling trigger), and coverage sites; the cast-time board-state classifier correctly treats it as trigger-event context (non-evaluable before activation). CR 701.57a. The `OncePerTurn` constraint ("triggers only once each turn") already parsed, so `Curator` no longer loops. Tests: `trigger_curator_discover_again_for_the_same_value` (parser), `triggering_discover_value_reads_last_discover_scalar` (resolver). Full engine lib suite green (16026 passed / 0 failed), combinator gate clean, clippy clean, parser gap analysis unchanged.
![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 MTG Comprehensive Rule 701.57a (discovering again for the same value, as seen on Curator of Sun's Creation) by introducing a new QuantityRef::TriggeringDiscoverValue variant and tracking the transient last_discover_value in GameState. The parser is also updated to recognize bare discover triggers. Feedback on this PR highlights a potential race condition and stale read vulnerability from using a global GameState field for transient discover values, recommending retrieving this from the triggering event context instead. Additionally, a violation of Rule R1 was identified in the parser where .starts_with() is used instead of 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.
| // CR 701.57a: record this discover's mana-value limit so a "whenever you | ||
| // discover" trigger's effect can reference "the same value" (Curator of | ||
| // Sun's Creation). Set before the trigger fires on the EffectResolved event. | ||
| state.last_discover_value = Some(limit as i32); |
There was a problem hiding this comment.
[HIGH] Global transient state last_discover_value is prone to race conditions and stale reads. Evidence: crates/engine/src/game/effects/discover.rs:40.
Why it matters: A single global field in GameState will be overwritten if multiple discover events occur before the triggers resolve, leading to incorrect values during resolution.
Suggested fix: Retrieve the discover value from the triggering event context (e.g., state.current_trigger_events) instead of a global GameState field.
| let rest = rest.trim_start(); | ||
| rest.is_empty() || rest.starts_with(['.', ';']) |
There was a problem hiding this comment.
[MEDIUM] Use of starts_with in parser dispatch violates repository rules. Avoid redundant string prefix checks (such as '.starts_with()') before calling parsing functions that already perform leading character validation and whitespace trimming. Replace starts_with with a character check or a nom combinator.
| let rest = rest.trim_start(); | |
| rest.is_empty() || rest.starts_with(['.', ';']) | |
| let rest = rest.trim_start(); | |
| rest.is_empty() || rest.chars().next().map_or(false, |c| c == '.' || c == ';') |
References
- R1. Nom combinators on the first pass — no exceptions. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators... any new .starts_with() used for parsing dispatch in non-test parser code is a finding. (link)
- Avoid redundant string prefix checks (such as '.starts_with()') before calling parsing functions that already perform leading character validation and whitespace trimming.
Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approving. The architecture pass recorded on this head (1d2aa09) stands: new QuantityRef::TriggeringDiscoverValue + transient GameState::last_discover_value is the defensible seam given EffectResolved{Discover} carries no value, the variant is justified (no sibling cluster to parameterize), it's wired exhaustively, and the once-per-turn loop guard is both parsed and asserted (CR 701.57a). All 13 checks are green and the branch is clean.
One non-blocking watch item for a follow-up (not gating this PR): the discover value is read at re-discover resolution time rather than snapshotted at trigger time, so two overlapping discovers in the same on-stack window could feed the wrong value. It's narrow and fails safe (0 outside a discover context); the proper fix is a value-carrying EffectResolved{Discover} event, which is a separate change. Thanks for the clean fix and the loop-guard test.
2 participants
Fixes #5270 (Curator of Sun's Creation). CR 701.57a.
"Whenever you discover, discover again for the same value. This ability triggers only once each turn." parsed to an Unknown trigger with an Unimplemented effect — the ability did nothing. The discover trigger (
TriggerMode::Discover+match_discover) and effect (Effect::Discover) both already exist; this wires the two missing parser paths plus the "same value" quantity.1. Trigger — "whenever you discover"
whenever/when <player> discover[s](bare) →TriggerMode::Discover(matched on theEffectResolved{Discover}event). The discover keyword action is not aPlayerActionKind, so a dedicatedis_bare_discover_subjectgate routes it before theparse_player_action_listpath. A compounddiscover, investigate, scry, …subject (Val, Marooned Surveyor) is deliberately left to the action-list path.2. Effect — "discover again for the same value"
→
Effect::Discover { mana_value_limit: QuantityRef::TriggeringDiscoverValue, player: Controller }(composed from tags, not a verbatim match).3.
QuantityRef::TriggeringDiscoverValue(new)The mana-value limit
Nof the discover that fired the current "whenever you discover" trigger. Threaded via a transientGameState::last_discover_value, set in the discover resolver before theEffectResolvedevent fires; resolves to0outside a discover context. Classified per the engine-variant checklist:Axes::NONE(reads a bounded game-state scalar; no growing/sibling/projected axis).empty()(written only by discover resolution, never a sibling trigger)._ => falsetrigger-event-context arm (correctly non-evaluable before activation).The
OncePerTurnconstraint already parsed, soCuratorno longer loops on itself.Verification
trigger_curator_discover_again_for_the_same_value(parser),triggering_discover_value_reads_last_discover_scalar(resolver).game/quantity,ability_scan,ability_rw,layers,triggers,coverage).