fix(parser): scope count-form "would draw N or more" draw replacements (Alms Collector #5678)

[Yurii214]

Closes #5678.

Problem

parse_replacement_line's draw-antecedent alt matched only "would draw a card" and a hardcoded "would draw one or more cards". Alms Collector's antecedent is the count-form "would draw two or more cards", so it matched neither and the line produced no replacement at all — the card silently did nothing.

Fix

Two parts, both at the parser seam (crates/engine/src/parser/oracle_replacement.rs):

1. Recognize the count-form antecedent as a class. Replace the hardcoded "one or more cards" tag with "would draw " + parse_number + " or more cards", so every <N> or more threshold (one, two, three, …) is recognized — build-for-the-class rather than a per-card tag. parse_number already covers digit and word forms.

2. Derive draw_scope from the substitute, not the antecedent. This is the subtle part, and it corrects the issue's stated expectation. The issue predicted InstructionCount, but that contradicts DrawReplacementScope's own doc and scripts/draw_replacement_census.py::classify_scope: InstructionCount is reserved for count-modifier substitutes that read the replaced instruction's own count ("draw that many cards plus one instead" — Quantum Riddler, a QuantityRef::EventContextAmount; the only such card). Alms Collector's substitute is a fixed substitution ("instead you and that player each draw a card") — the Notion Thief / Hullbreacher class the enum doc explicitly scopes as IndividualDraw (CR 121.6b: cards are drawn one at a time).

   So the scope is finalized after the execute chain is parsed. Both antecedent forms seed the IndividualDraw default; execute_draw_reads_replaced_count then inspects the execute's top-level Draw effect — the exact surface classify_scope reads (execute["effect"]) — for a count that references EventContextAmount, and promotes to InstructionCount only then. Emitting the scope from the same signal the census reads keeps producer and cross-check in agreement by construction: Alms Collector → IndividualDraw, Quantum Riddler → InstructionCount, and no existing card's scope changes.

Why the corpus baseline moves

scripts/draw-replacement-corpus.tsv gains exactly one row (alms collector … IndividualDraw) now that the card produces a Draw replacement. Re-frozen with scripts/draw_replacement_census.py --corpus --write in this commit; --corpus --check is green (51 rows), and the diff is that single added row — no existing row moved.

Verification

• New parser unit test count_form_draw_antecedent_is_recognized_and_scope_follows_the_substitute drives the real entry (parse_replacement_line) and asserts scope follows the substitute: Alms Collector + a fixed "three or more" → IndividualDraw; a "that many … plus one" count-modifier → InstructionCount; singular "a card" → IndividualDraw.
• cargo test -p engine --lib (the new test) — green.
• draw_replacement_census.py --corpus --check — green after re-freeze (against a freshly regenerated card-data.json).
• cargo clippy -p engine --lib -- -D warnings — clean.
• CR 121.2a / CR 121.6b verified against docs/MagicCompRules.txt.

Model: claude-opus-4-8[1m]

[gemini-code-assist]

Code Review

This pull request improves the draw replacement parser to recognize count-form antecedents (e.g., "would draw or more cards") and dynamically determine the replacement scope (IndividualDraw vs. InstructionCount) based on the substitute's behavior rather than the antecedent's grammatical number. Feedback focuses on ensuring that the newly added comments strictly adhere to Rule R6's formatting requirements for CR annotations to prevent breaking automated regex verification.

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.

[github-actions]

Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main 2b8d5ac35137)

1 card(s) · replacement/Draw · added: Draw (condition=OnlyIfQuantity { lhs: Ref { qty: EventContextAmount }, comparator: GE, rhs: Fixed { value: 2 }, active_player_req: None }, draw scope=Instructi…

Examples: Alms Collector

1 card(s) · ability/replacement_structure · removed: replacement_structure

Examples: Alms Collector

[matthewevans]

Verdict: changes requested — the parser recognizes the count form, but the resulting definition cannot enforce its N or more threshold at runtime.

🔴 Blocker

crates/engine/src/parser/oracle_replacement.rs:426-443 parses the numeric antecedent but retains only IndividualDraw; :526-542 then derives the scope solely from the substitute. No field on ReplacementDefinition carries the parsed N, and draw_scope only selects instruction versus individual-draw matching. Consequently, Alms Collector's two or more condition is discarded: an individual-draw matcher sees one card at a time, while an instruction matcher has no typed >= 2 predicate to enforce. This would either never apply to the intended two-card instruction or apply to the wrong draw class.

The earlier #5678 investigation reached the same implementation boundary: issue comment. CR 121.2a applies the relevant replacement before individual draws; CR 121.6b then governs resuming the individual sequence. The current parser-only test proves AST recognition and chosen scope, not that a pending two-card draw is gated correctly or that a one-card draw is left alone.

Please carry the antecedent through a typed replacement condition (for example, EventContextAmount >= N), teach the census classifier to recognize that typed signal as InstructionCount, and verify the replacement matcher resolves it against the pending instruction count. Add an engine-pipeline regression that proves: opponent draw 1 is unchanged; opponent draw 2 is replaced; and the replacement's two draws occur for the correct players.

🟡 Non-blocking

The fixed-substitute versus count-modifier distinction is real, but it does not eliminate the antecedent threshold. It decides what the substitute does; the typed antecedent still decides whether the instruction is eligible.

✅ Clean

parse_number is the right reusable combinator for the open-ended N or more grammar axis, rather than a two or more special case.

Recommendation: rework this through the draw-replacement model/runtime path, then request re-review on the new head.

[Yurii214]

Thanks for the detailed review — the blocker is right: the parsed N isn't carried, so as-is the replacement doesn't gate on "two or more." Before reworking, I traced the draw-replacement runtime to wire the threshold through OnlyIfQuantity { lhs: EventContextAmount, comparator: GE, rhs: N } (reusing the existing condition rather than adding a variant), and I wanted to confirm the intended approach, because it looks like draw_scope isn't yet load-bearing at runtime:

• draw_scope has no behavioral read in game/. Grep finds it only at game/coverage.rs:4347 (display) and validate_draw_scope / the consult-seam debug_assert! — no matcher branches on InstructionCount vs IndividualDraw.
• draw_matcher is scope-agnostic (game/replacement.rs:1941): matches!(event, ProposedEvent::Draw { count, .. } if *count > 0).
• The pipeline decomposes before offering replacements. start_draw_sequence → resume_draw_sequence draws one card at a time, calling draw_through_replacement_with_applied(state, player, 1, …) (game/effects/draw.rs), so every draw replacement is offered against ProposedEvent::Draw { count: 1 }.
• The condition path can't see the count. evaluate_replacement_condition's OnlyIfQuantity arm resolves through replacement_condition_quantity_ctx, whose QuantityContext carries no event amount, so EventContextAmount doesn't resolve to the pending draw's count there (unlike the execute path's resolve_event_replacement_quantity).

So an EventContextAmount >= 2 gate would see 1 per unit and never fire — enforcing the threshold seems to require building the CR 121.2a instruction stage that draw_scope anticipates: offer InstructionCount defs against the whole instruction before per-card decomposition, make matching scope-aware, and resolve EventContextAmount for the gate. Since that's a central change to the draw pipeline (and Quantum Riddler's InstructionCount is currently unimplemented at the matcher too), I wanted to check the intended design before touching it.

I'm happy to build it end-to-end: parser (OnlyIfQuantity{EventContextAmount>=N} + InstructionCount), census (classify_scope recognizing the condition-subtree signal), the instruction-stage matcher + condition resolution, and a GameScenario regression (opponent draw 1 unchanged; draw 2 replaced; both substitute draws to the correct players). Before I do — does that match how you'd want the instruction stage wired, or would you prefer a narrower scope (or to take the core-pipeline piece yourselves)? And please point me at any existing scope-aware seam if I've missed one.

[matthewevans]

@Yurii214 yes, we just refactored how draw works in the engine. If you rebase you should be able to leverage the new approach to correctly complete this PR

• Confirmed blocker: PR 5867 parses “two or more” but discards N. Its draw_scope is not used for runtime matching, and the current sequence reduces every draw instruction to one-card events. So Alms Collector’s “two or more” condition cannot be enforced.
• The new centralized start_draw_sequence / DrawSequenceFrame flow provides the right instruction-level seam to evaluate the full pending draw count before it decomposes into individual draws.
• Required rework: retain N as a typed replacement condition; add instruction-stage matching/context for the pending draw count; apply it before individual draws; add end-to-end one-card vs two-card regression tests. The current “infer scope from execute shape” approach conflicts with the new scope contract.

[Yurii214]

Rebased onto the new draw engine and pushed the parser + condition + census layer:

• Parser — the count-form antecedent now captures N via parse_number (build for the class: any <N> or more cards, not a "two or more" special case) and, for N ≥ 2, retains it as a typed ReplacementCondition::OnlyIfQuantity { lhs: EventContextAmount, comparator: GE, rhs: N } (reusing the existing condition — no new variant), composed (And) with any as-long-as/while gate. Scope is InstructionCount; opponent player-scope and the "you + that player each draw" substitute already lower correctly. Dropped the old "infer scope from execute shape" approach per your note.
• Census — classify_scope now treats a Draw definition whose condition references EventContextAmount as InstructionCount (the instruction-count signal lives in the antecedent threshold, not just the execute's count); corpus re-frozen. Quantum Riddler is unchanged (still InstructionCount via its execute).
• Parser unit test asserts Alms Collector → Draw / InstructionCount / valid_player: Opponent / OnlyIfQuantity(EventContextAmount ≥ 2).

What's left is the instruction-stage runtime, and I'd like to match your intended design before touching the core seam. Tracing the new flow: start_draw_sequence_with_origin pushes the frame with the full count then calls resume_draw_sequence — the seam is right there — but three pieces are still unwired: (1) no instruction-level replace_event offer against Draw { count: N } exists yet (every draw is still offered per-unit at count: 1); (2) draw_scope still doesn't gate matching, so enforcing it newly activates Quantum Riddler's InstructionCount too; (3) EventContextAmount isn't threaded into condition evaluation (replacement_condition_quantity_ctx's QuantityContext carries no event amount), so OnlyIfQuantity{EventContextAmount ≥ 2} currently resolves against 0.

Since you just refactored this and it's high blast radius, how would you prefer the instruction offer wired — e.g. a DrawStage { Instruction, Unit } discriminator on ProposedEvent::Draw gated against draw_scope, or a scope post-filter of the candidate list at the seam? I'm happy to build the whole runtime to your contract (offer + scope gate + QuantityContext.event_amount threading + a one-card-vs-two-card GameScenario regression), or if you'd rather own the core matcher hook I'll finish the condition-threading and tests on top. Whichever you prefer.

[matthewevans]

Verdict: changes requested — the parser/census work now models the threshold correctly, but it advertises a supported instruction-stage replacement before the runtime has the required instruction-stage authority.

🔴 Blocker

crates/engine/src/parser/oracle_replacement.rs:424-460 lowers Alms Collector to InstructionCount with OnlyIfQuantity(EventContextAmount >= N), while crates/engine/src/game/effects/draw.rs:248-307 still decomposes the frame and consults only ProposedEvent::Draw { count: 1 }. The full parse-diff consequently reports Alms Collector as added support, but no candidate can see its count-two instruction and the condition cannot be true in the per-unit consultation. This is coverage-incorrect rather than a parser-only intermediate.

The intended runtime shape is the Plan 03 draw authority: keep DrawReplacementScope as the definition-side classifier, add a separate explicit event-side DrawEventStage (instruction-count versus individual draw), and consult scope-compatible definitions at the matching stage. The instruction consultation must happen once against the full count before units are created; each surviving unit then gets its own individual-draw consultation. Thread that stage event's amount into condition quantity resolution, rather than adding a post-filter or inferring scope from the execute body. This is the design needed for both Alms Collector and Quantum Riddler and preserves CR 121.2a's ordering.

03-draw-and-zone-authority.md is also clear that this belongs in the centralized, pause-safe draw sequence and should follow its Plan-02 preflight, not as a parser-only acceptance change. Please either complete that scoped runtime work with one-card/two-card end-to-end GameScenario coverage (including the correct replacement draws), or keep Alms Collector honestly unsupported until the Plan-03 implementation lands.

✅ Clean

The new antecedent lowering is at the right parser seam: parse_number plus a typed OnlyIfQuantity condition represents the full N or more class without a card-specific branch.

Recommendation: retain the typed parser direction, but land it only with the planned staged runtime authority and discriminating runtime coverage; do not add a generic candidate post-filter.

[matthewevans]

Clarification to my prior review: you do not have access to the internal remediation plan I referenced, and I am not asking you to implement a hidden core-pipeline design.

Your diagnosis is correct: the current engine has no public, complete instruction-stage replacement contract, so this PR cannot safely finish the runtime portion from the available contributor context alone. Please do not add a speculative candidate post-filter or a new ad-hoc draw discriminator. We will own the centralized staged draw work and publish the relevant contract before requesting contributor changes in that area.

For this PR, the actionable state is simply that parser/census acceptance cannot merge while the runtime is absent, because it would mark Alms Collector supported without enforcing its threshold. We will follow up after the shared runtime lands; no additional implementation is expected from you now.

[matthewevans]

Changes requested — parser coverage claims a runtime capability that the draw pipeline does not implement.

🔴 Blocker

• crates/engine/src/parser/oracle_replacement.rs:408-580 parses Alms Collector as InstructionCount and records EventContextAmount >= 2; scripts/draw_replacement_census.py:304-326 then freezes that parser classification. But the draw execution authority is still the per-unit sequence driver in crates/engine/src/game/effects/draw.rs:138-221; this PR adds no instruction-stage consult/apply path for DrawReplacementScope::InstructionCount. The new test at oracle_replacement.rs:17076-17120 asserts only the AST/coverage shape, so it cannot prove that an opponent's two-card draw becomes the required one-card-for-each substitute (or that a one-card draw remains unchanged).

Please hold this parser/coverage change until the core draw-replacement contract has a real instruction-stage authority, then land it with registered runtime regressions for 1-card and 2+-card opponent draws.

✅ Clean

• The antecedent grammar is compositional and the threshold is typed rather than a card-name special case.

Recommendation: request changes — do not mark Alms Collector supported before the instruction-count runtime path exists.

[Yurii214]

Understood, and thanks for the clear guidance — I'll hold on the runtime and won't add a speculative post-filter or draw discriminator. Glad the threshold modeling (parser + OnlyIfQuantity{EventContextAmount >= N} + the census condition rule) is on the right track.

Happy to keep this PR open so that layer is ready to combine once the shared staged-draw runtime + contract land, or to close it and re-open against the published contract — whichever keeps your queue cleaner. Just let me know. Appreciate the detailed engagement.