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 2 commits into
Conversation
![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.
…#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
Review follow-up (phase-rs#1128), all four points: 1. Global scalar removed. GameState::last_scry_look_count is gone. The effective (post-clamp) look count now rides on the scry's own PlayerPerformedAction event (new optional look_count field, serde- defaulted; every construction site updated, patterns widened). Each 'whenever you scry' trigger preserves its own event through target selection and stack resolution (PendingTriggerContext / push_trigger_event_context installs it around build_target_slots), so QuantityRef::TriggeringScryLookCount resolves from the CURRENT trigger's event — current_trigger_event with the detection-time thread-local fallback — never from state a later scry can overwrite. 2. Parser recomposed. The verbatim full-oracle tag is replaced by a composed combinator in oracle_nom::quantity (parse_scry_look_count_ref: count-of-cards subject + 'looked at' participle + 'while scrying' qualifier + reflexive 'this way' suffix), registered ahead of the generic parse_the_number_of in parse_quantity_ref; the where-X binder delegates to it and keeps the whole-expression ownership requirement. 3. Multi-scry regression added ('Scry 3.\nScry 1.' in ONE resolution). With the patch reverted, the queued trigger's target-slot limit reads the LAST scry's count (1) instead of its own (3) — red. Honest finding pinned in the test: the engine currently queues ONLY the first scry's trigger — the second scry's event, emitted between the two interactive ScryChoice pauses of the same resolution, never produces a pending trigger at all (deferred_triggers stays at 1, no OrderTriggers prompt). That second-trigger loss is a separate, pre-existing pause/resume event-collection defect orthogonal to this fix; the test hard-asserts the current single-trigger behavior so a future fix surfaces here and the test can be upgraded to the full two-trigger shape. 4. Scope + citations. The three second-ability tests (issue_1128_elrond_becomes_target.rs) are removed from this bug-fix PR as requested. Citations stay on CR 701.22a (the scry action) and CR 701.22d (post-scry trigger timing) with no claimed clamping subrule; Gemini's 701.22b suggestion was not applied (701.22b is the scry-0 no-event rule). Verified in an isolated CARGO_TARGET_DIR: cargo fmt --all --check clean; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -D warnings clean; engine lib 17515 passed; engine integration 3750 passed.
96b04e7 to
8465765
Compare
|
Addressed all four review points. Per-trigger event provenance (blocker): Composed grammar (blocker): the verbatim full-oracle Multi-scry regression: added ("Scry 3.\nScry 1." in one resolution). With the patch reverted, the queued trigger's slot limit reads the LAST scry's count (1) instead of its own (3) — red. One honest finding pinned in the test rather than papered over: the engine currently queues ONLY the first scry's trigger — the second scry's event, emitted between the two interactive Scope + citations: the three second-ability tests are removed from this PR as requested; citations stay on CR 701.22a/CR 701.22d with no claimed clamping subrule (701.22b not applied — it is the scry-0 no-event rule). Verified locally in an isolated target dir: fmt clean, workspace clippy |
📝 WalkthroughWalkthroughThe engine records effective scry look counts in player-action events, adds a corresponding triggering quantity reference and Oracle parser support, resolves it from preserved trigger context, and adds regression coverage for Elrond’s ability and expanded event patterns. ChangesScry event data and propagation
Estimated code review effort: 3 (Moderate) | ~25 minutes Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ast-grep (0.44.1)crates/engine/src/game/triggers.rsast-grep timed out on this file 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/game/ability_rw.rs`:
- Around line 5666-5669: Model TriggeringScryLookCount as a per-trigger event
dependency: in crates/engine/src/game/ability_rw.rs#L5666-L5669, update the
QuantityRef::TriggeringScryLookCount profile to be member-bound/event-live
rather than RwProfile::empty(); in
crates/engine/src/game/ability_scan.rs#L1795-L1797, update the corresponding
scan handling to mark the event axis rather than Axes::NONE.
In `@crates/engine/tests/integration/issue_1128_elrond_scry_counter.rs`:
- Around line 170-180: Update pause/resume trigger collection so every scry
event in the multi-scry resolution queues its PlayerPerformedAction trigger,
rather than preserving the single-trigger limitation documented near the
regression test. Extend the test flow to process both queued triggers through
target selection and assert their distinct slot limits are 3 and 1.
🪄 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: 6900cf97-138c-4502-abe2-794527815248
📒 Files selected for processing (31)
crates/engine/src/analysis/sim.rscrates/engine/src/game/ability_rw.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/coverage.rscrates/engine/src/game/effects/change_zone.rscrates/engine/src/game/effects/collect_evidence.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/investigate.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/proliferate.rscrates/engine/src/game/effects/scoped_library_search.rscrates/engine/src/game/effects/scry.rscrates/engine/src/game/effects/search_library.rscrates/engine/src/game/effects/shuffle.rscrates/engine/src/game/effects/surveil.rscrates/engine/src/game/engine.rscrates/engine/src/game/layers.rscrates/engine/src/game/log.rscrates/engine/src/game/quantity.rscrates/engine/src/game/targeting.rscrates/engine/src/game/trigger_matchers.rscrates/engine/src/game/triggers.rscrates/engine/src/parser/oracle_effect/lower.rscrates/engine/src/parser/oracle_nom/quantity.rscrates/engine/src/types/ability.rscrates/engine/src/types/events.rscrates/engine/tests/integration/cost_zone_pipeline.rscrates/engine/tests/integration/issue_1128_elrond_scry_counter.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/natural_balance.rs
| // CR 701.22a: reads the transient last-scry-look-count scalar, | ||
| // written only by scry resolution (never by a sibling trigger) — no | ||
| // ordering-relevant read/write, mirroring TriggeringDiscoverValue. | ||
| QuantityRef::TriggeringScryLookCount => RwProfile::empty(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Model the scry count as a per-trigger event dependency.
TriggeringScryLookCount reads each trigger’s preserved PlayerPerformedAction::Scry event, so it cannot be modeled as a scalar with no read axes. This can let batching/order analysis treat distinct scry-trigger instances as uniform even when their look counts differ.
crates/engine/src/game/ability_rw.rs#L5666-L5669: mark the reference member-bound/event-live rather than returning an empty profile.crates/engine/src/game/ability_scan.rs#L1795-L1797: mark the event axis instead of returningAxes::NONE.
📍 Affects 2 files
crates/engine/src/game/ability_rw.rs#L5666-L5669(this comment)crates/engine/src/game/ability_scan.rs#L1795-L1797
🤖 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/game/ability_rw.rs` around lines 5666 - 5669, Model
TriggeringScryLookCount as a per-trigger event dependency: in
crates/engine/src/game/ability_rw.rs#L5666-L5669, update the
QuantityRef::TriggeringScryLookCount profile to be member-bound/event-live
rather than RwProfile::empty(); in
crates/engine/src/game/ability_scan.rs#L1795-L1797, update the corresponding
scan handling to mark the event axis rather than Axes::NONE.
| /// Observed engine limitation, deliberately NOT asserted around here: only | ||
| /// the FIRST scry's trigger is queued at all — the second scry's | ||
| /// `PlayerPerformedAction` event (emitted between the first and second | ||
| /// interactive `ScryChoice` pauses of the same resolution) never produces a | ||
| /// pending trigger (`deferred_triggers` stays at 1 and no `OrderTriggers` | ||
| /// prompt appears). That second-trigger loss is a separate, pre-existing | ||
| /// pause/resume event-collection defect, orthogonal to how the look count is | ||
| /// carried; this test pins the per-event provenance for the trigger that DOES | ||
| /// fire, and intentionally hard-asserts the current single-trigger behavior | ||
| /// so a future fix for the loss surfaces here and can upgrade the test to the | ||
| /// full two-trigger shape. |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
Do not codify loss of the second scry trigger.
This regression accepts that the Scry 1 event produces no trigger, contradicting “Whenever you scry” and leaving the multi-scry path broken. Fix pause/resume trigger collection, then drive both queued triggers through target selection and assert distinct slot limits of 3 and 1.
🤖 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/tests/integration/issue_1128_elrond_scry_counter.rs` around
lines 170 - 180, Update pause/resume trigger collection so every scry event in
the multi-scry resolution queues its PlayerPerformedAction trigger, rather than
preserving the single-trigger limitation documented near the regression test.
Extend the test flow to process both queued triggers through target selection
and assert their distinct slot limits are 3 and 1.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The current head still has two merge blockers.
-
The requested multi-scry proof is not achieved. The new regression intentionally asserts that the second Scry event does not trigger, which contradicts
Whenever you scryand the PR's claimed multi-scry correctness. Repair pause/resume event collection and assert that both triggers independently expose 3 and 1 target slots. Evidence:crates/engine/tests/integration/issue_1128_elrond_scry_counter.rs:171. -
TriggeringScryLookCountnow reads the preserved trigger event, butability_rwstill describes it as reading a global scalar and returnsRwProfile::empty(), whileability_scanreturnsAxes::NONE. Classify it consistently as a per-event/event-live dependency withEventContextAmount/ current-trigger-event semantics, and add a focused classifier regression. Evidence:crates/engine/src/game/ability_rw.rs:5666;crates/engine/src/game/ability_scan.rs:1795.
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.
Summary by CodeRabbit
New Features
Bug Fixes