fix(engine): bind Elrond's scry-X to the clamped look count#5872
fix(engine): bind Elrond's scry-X to the clamped look count#5872tryeverything24 wants to merge 1 commit into
Conversation
…#1128) Elrond, Master of Healing's first ability ("put a +1/+1 counter on each of up to X target creatures, where X is the number of cards looked at while scrying this way") had no representation at all: no QuantityRef existed for "cards looked at while scrying", so the where-X binder fell through and the whole ability lowered to Effect::Unimplemented. Add QuantityRef::TriggeringScryLookCount, backed by a new GameState::last_scry_look_count transient scalar set in scry::apply_scry_after_replacement_without_draw at the exact point the requested scry amount is clamped to library size (mirroring the existing last_discover_value / QuantityRef::TriggeringDiscoverValue pattern used by Curator of Sun's Creation's "discover again for the same value"). Wire the new variant through the ~10 exhaustive QuantityRef match sites (quantity resolution, coverage, ability_scan, ability_rw, layers, triggers) and add a parser branch recognizing "the number of cards looked at while scrying this way" in parse_where_x_quantity_expression. The existing "up to X target creatures, where X is ..." multi-target machinery already threads whatever QuantityExpr the where-X binder produces, so no changes were needed there. Elrond's second ability ("whenever a creature you control with a +1/+1 counter on it becomes the target of a spell or ability an opponent controls, you may draw a card") was investigated and found to already parse and resolve correctly end-to-end — both constituent pieces (the controller-scoped "becomes the target ... an opponent controls" source filter and the "with a +1/+1 counter on it" subject filter) already existed and were independently tested, just never combined on one card. No code change needed for this ability; added regression coverage for the previously-untested combination (positive fire, no-counter exclusion, self-cast exclusion). Added crates/engine/tests/integration/issue_1128_elrond_scry_counter.rs (clamped-X end-to-end: scry 3 with only 2 library cards left proves counters land on at most 2 creatures, not 3) and issue_1128_elrond_becomes_target.rs (three scenarios for the second ability). Closes phase-rs#1128
![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 Elrond, Master of Healing's first ability by introducing QuantityRef::TriggeringScryLookCount to track the effective look count of a scry, backed by a new transient last_scry_look_count field in GameState. Feedback highlights a potential high-severity correctness bug where sequential or nested scries could overwrite this global state before triggers resolve. Additionally, the reviewer noted a style guide violation (R1) regarding verbatim string matching in the parser instead of using modular nom combinators, and flagged several incorrect CR citations that should reference CR 701.22b instead of CR 701.22a.
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.
| // way" (Elrond, Master of Healing → `QuantityRef::TriggeringScryLookCount`). | ||
| // Set before the trigger fires on the PlayerPerformedAction event below, | ||
| // mirroring `discover::resolve`'s `last_discover_value` write. | ||
| state.last_scry_look_count = Some(count as i32); |
There was a problem hiding this comment.
[HIGH] Potential correctness bug with sequential/nested scries overwriting last_scry_look_count before triggers resolve.
If a single spell or ability causes multiple sequential scry actions (e.g., Cryptic Annelid's 'Scry 1, then scry 2, then scry 3'), each scry will trigger Elrond. However, because state.last_scry_look_count is a single global scalar on GameState, it will be overwritten by each subsequent scry before any of the triggers are put on the stack. When the triggers are finally put on the stack and targets are selected, all of them will evaluate X using the final scry's look count (3), rather than their respective triggering scry's look count (1, 2, and 3).
Consider associating the scry look count directly with the trigger event (e.g., by adding a payload to PlayerActionKind::Scry or storing it in the event context) rather than using a global transient scalar.
| }); | ||
| } | ||
|
|
||
| // CR 701.22a: record this scry's effective look count — the requested |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for scry clamping behavior.
The clamping of the scry count to the library size is governed by CR 701.22b ("If a player has fewer than N cards in their library, they look at all the cards in their library"), not CR 701.22a (which defines the basic scry action). Please update this citation here and in all other added occurrences.
| // CR 701.22a: record this scry's effective look count — the requested | |
| // CR 701.22b: record this scry's effective look count — the requested |
References
- CR annotations are mandatory and verified. A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| // discover resolution (never by a sibling trigger) — no ordering-relevant | ||
| // read/write, mirroring StartingLifeTotal. | ||
| QuantityRef::TriggeringDiscoverValue => RwProfile::empty(), | ||
| // CR 701.22a: reads the transient last-scry-look-count scalar, |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for scry clamping behavior.
The clamping of the scry count to the library size is governed by CR 701.22b, not CR 701.22a.
| // CR 701.22a: reads the transient last-scry-look-count scalar, | |
| // CR 701.22b: reads the transient last-scry-look-count scalar, |
References
- CR annotations are mandatory and verified. A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| // CR 701.57a: reads a transient game-state scalar (the last discover's | ||
| // mana-value limit); no growing resource, sibling, or projected axis. | ||
| QuantityRef::TriggeringDiscoverValue => Axes::NONE, | ||
| // CR 701.22a: reads a transient game-state scalar (the last scry's |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for scry clamping behavior.
The clamping of the scry count to the library size is governed by CR 701.22b, not CR 701.22a.
| // CR 701.22a: reads a transient game-state scalar (the last scry's | |
| // CR 701.22b: reads a transient game-state scalar (the last scry's |
References
- CR annotations are mandatory and verified. A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| // "whenever you discover" trigger (Curator of Sun's Creation, "the same | ||
| // value"). 0 outside a discover-trigger context. | ||
| QuantityRef::TriggeringDiscoverValue => state.last_discover_value.unwrap_or(0), | ||
| // CR 701.22a: the effective (clamped) look count of the scry that |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for scry clamping behavior.
The clamping of the scry count to the library size is governed by CR 701.22b, not CR 701.22a.
| // CR 701.22a: the effective (clamped) look count of the scry that | |
| // CR 701.22b: the effective (clamped) look count of the scry that |
References
- CR annotations are mandatory and verified. A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| }) | ||
| } | ||
|
|
||
| /// CR 701.22a: "where X is the number of cards looked at while scrying this |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for scry clamping behavior.
The clamping of the scry count to the library size is governed by CR 701.22b, not CR 701.22a.
| /// CR 701.22a: "where X is the number of cards looked at while scrying this | |
| /// CR 701.22b: "where X is the number of cards looked at while scrying this |
References
- CR annotations are mandatory and verified. A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
| fn parse_where_x_scry_look_count(where_x_expression: &str) -> Option<QuantityExpr> { | ||
| let lower = where_x_expression.to_ascii_lowercase(); | ||
| let (rest, _) = | ||
| tag::<_, _, OracleError<'_>>("the number of cards looked at while scrying this way") | ||
| .parse(lower.as_str()) | ||
| .ok()?; | ||
| rest.is_empty().then_some(QuantityExpr::Ref { | ||
| qty: QuantityRef::TriggeringScryLookCount, | ||
| }) | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Verbatim string matching on full Oracle phrase violates style guide R1 and general parsing rules.
Using tag on the entire phrase "the number of cards looked at while scrying this way" followed by rest.is_empty() is effectively a verbatim string equality check. This bypasses the robust nom-based parser and creates fragile matches.
Instead, decompose this compound phrase into modular, reusable parsers or use existing combinators to parse the constituent parts (e.g., parsing "the number of cards looked at" and "while scrying this way" separately).
References
- Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators. Verbatim string equality on full Oracle phrases is prohibited. (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 (e.g., subjects, conjunctions) and compose them using idiomatic combinator aggregates (like nested
altandtagsequences) to prevent combinatorial explosion and improve maintainability.
| /// game state, but serialized so a mid-resolution pause round-trips. | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub last_discover_value: Option<i32>, | ||
| /// CR 701.22a: The number of cards actually looked at by the most |
There was a problem hiding this comment.
[MEDIUM] Incorrect CR citation for scry clamping behavior.
The clamping of the scry count to the library size is governed by CR 701.22b, not CR 701.22a.
| /// CR 701.22a: The number of cards actually looked at by the most | |
| /// CR 701.22b: The number of cards actually looked at by the most |
References
- CR annotations are mandatory and verified. A CR annotation where the cited rule's body does not describe what the code is doing is a finding. (link)
Parse changes introduced by this PR · 2 card(s), 2 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Verdict: changes requested
🔴 Blocker
last_scry_look_count is a global mutable scalar, but the value in Elrond's "this way" clause belongs to each individual triggering scry event. A resolving ability that scries more than once can overwrite it before the queued triggers are constructed; then every Elrond trigger receives the final scry's count. PendingTriggerContext already preserves the individual GameEvent through target selection and stack resolution, so carry the effective look count on that event (or its typed action payload) and resolve TriggeringScryLookCount from the current trigger context instead. Add a production-pipeline regression with at least two scries in one resolution whose look counts differ; it must prove the two queued Elrond triggers expose different target-slot limits and fail with this patch reverted.
The new parser arm is also a verbatim full-Oracle match (tag("the number of cards looked at while scrying this way") plus rest.is_empty()). Split the grammar into composed nom pieces so the reusable quantity phrase and its scry-context suffix are independently represented, rather than adding a one-card literal dispatch.
🟡 Non-blocking
The three tests for Elrond's already-working second ability do not exercise the reported defect or a changed production seam, while the existing parser tests already cover its constituent filters. Please remove them from this bug-fix PR (or move them to a separately justified regression) so each added integration test earns its cost.
Gemini's suggested CR 701.22b correction is not applicable: the checked rules text defines 701.22b as scry 0 produces no event. Keep the actual citations tied to 701.22a (the scry action) and 701.22d (the post-scry trigger timing), with wording that does not claim a nonexistent clamping subrule.
✅ Clean
The parse-diff artifact is scoped to Elrond and Elvish Mariner, matching the new phrase; the current CI is green.
Recommendation: rework the value as per-trigger event provenance, add the multi-scry regression, then request re-review.
2 participants
Fixes #1128.
Elrond, Master of Healing: "Whenever you scry, put a +1/+1 counter on each of up to X target creatures, where X is the number of cards looked at while scrying this way."
Root cause & fix
No
QuantityRefexisted anywhere for "cards looked at while scrying," so the where-X binder fell through and the whole ability lowered toEffect::Unimplemented.Added
QuantityRef::TriggeringScryLookCount, backed byGameState::last_scry_look_count, set inapply_scry_after_replacement_without_drawat the point the requested scry amount is clamped to library size — mirroring the existingQuantityRef::TriggeringDiscoverValueprecedent (Curator of Sun's Creation's "discover again for the same value") rather than inventing new machinery. Wired through the exhaustiveQuantityRefmatch sites plus a new branch inparse_where_x_quantity_expression. The existing "up to X target creatures" multi-target machinery already consumed the new quantity with zero further changes.Elrond's second ability ("becomes the target of a spell or ability an opponent controls" draw trigger) was already correct — verified with new combination test coverage, no fix needed there.
Testing
New end-to-end test covering both abilities, including a clamped-X edge case (library smaller than the scry amount) and the opponent-targeting draw trigger (positive, negative, and self-cast-negative cases).
Verified locally (rebased onto main): fmt clean, clippy clean, 16673 lib tests pass, 3136 integration tests pass.