fix(parser): count "for each <type> you control with <keyword>" (Skycat Sovereign, Aven Gagglemaster)

[luciferlive112116]

Problem (#5018)

The for each quantity grammar could not parse a controller-scoped population count that carries a trailing keyword qualifier — for each <type> you control with <keyword>:

• Skycat Sovereign — "This creature gets +1/+1 for each other creature you control with flying."
• Aven Gagglemaster — "you gain 2 life for each creature you control with flying."
• Aerial Assault — "You gain 1 life for each creature you control with flying."
• Alert Heedbonder — "you gain 1 life for each creature you control with vigilance."
• Overgrown Battlement — "{T}: Add {G} for each creature you control with defender."

The for each dispatch already had the any-controller combinator parse_for_each_battlefield_type_with_keyword (<type> on the battlefield with <keyword>, backing Radiant, Archangel / Pride of the Clouds — #5008), but never got the controller-scoped sibling. The bare parse_for_each_controlled_type arm matched creature you control and stopped, stranding with flying as an unconsumed remainder. Since the clause must fully consume, the whole quantity failed and the dependent effect (dynamic pump / life gain / mana) was silently dropped — the same failure mode #5008 fixed, one scope over.

Fix (oracle_nom/quantity.rs)

• Add parse_for_each_controlled_type_with_keyword, the you control sibling of parse_for_each_battlefield_type_with_keyword: [other|another] <type> you [already] control with <keyword>, generalized over the full KEYWORDS table via parse_keyword_name + FilterProp::WithKeyword, with the optional other/another self-exclusion lowered to FilterProp::Another. The count is scoped to the source's controller (controller: Some(ControllerRef::You), CR 109.4 — only objects with a controller are counted).
• Register it in the for each alt() immediately before parse_for_each_controlled_type, since the bare arm's shorter you control match would otherwise win and strand the with <keyword> clause.

No new AST/variant — reuses existing QuantityRef::ObjectCount / TypedFilter / FilterProp shapes.

Before / after

for each other creature you control with flying
  before: (clause fails to consume → whole for-each quantity dropped → pump lost)
  after:  ObjectCount{ Typed{ Creature, controller: You, [Another, WithKeyword(Flying)] } }

Result

Skycat Sovereign now parses to a continuous self-static with a dynamic +1/+1 per other creature you control with flying (CR 604.1 / 611.3a / 613.4c), and the life-gain / mana cards in the class recover their count. The combinator covers the whole <type> you control with <keyword> for-each class, not just these cards.

Implementation method (required)

• Produced via the /engine-implementer pipeline
• Not /engine-implementer — explain why below

A pure-parser, additive combinator that mirrors the already-merged battlefield-wide sibling (#5008) one scope over (you control instead of on the battlefield). No engine game-logic, AST, or variant change — it reuses existing QuantityRef/FilterProp shapes the resolver already evaluates. A /review-impl self-check was run against the two mandatory gates (correct seam: the for each combinator dispatch; most idiomatic form: a composed nom sibling, not a string match).

CR references

• CR 109.4 — only objects on the battlefield or stack have a controller, so a "you control" count is over the source controller's permanents.
• CR 604.1 / 611.3a / 613.4c — Skycat Sovereign's dynamic pump is a static continuous layer-7c P/T modification whose value tracks the current count.

Testing

Local toolchain has no MSVC/mingw linker (verified: no link.exe/cl.exe/gcc), so I could not link and run the test binaries — only the non-linking checks were run:

• cargo fmt --all — clean.
• cargo check -p engine --lib --tests — clean.
• cargo clippy -p engine --lib --tests -- -D warnings — clean (0 warnings).

New tests (written to the same conventions as the passing suite; please let CI's nextest be the gate):

• parse_for_each_controlled_type_with_keyword_scoped_count (oracle_nom/quantity.rs) — nom-level: asserts controller: Some(You), the keyword-table generalization (flying / vigilance / defender), and the other/another/bare prefix variants.
• parse_for_each_controlled_type_bare_still_parses — guards that the new arm does not shadow the bare you control count.
• static_self_dynamic_pump_for_each_other_creature_you_control_with_keyword (oracle_static/tests.rs) — end-to-end, mirroring fix(parser): parse dynamic pump "for each ... on the battlefield with <keyword>" (Radiant Archangel, Pride of the Clouds) #5008's ...on_battlefield_with_keyword regression for the you control scope.

Model: claude-opus-4-8[1m] (via Claude Code)

Closes #5018

[gemini-code-assist]

Code Review

This pull request introduces parsing support for controller-scoped "for each [other] you control with " clauses (e.g., Skycat Sovereign, Aven Gagglemaster, Overgrown Battlement) to prevent them from being shadowed by the simpler "you control" parser. It also adds comprehensive unit and integration tests. The review feedback highlights a critical improvement opportunity to avoid using unwrap() in the parser code, suggesting a safer approach using map_res to prevent potential engine panics.

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.

[gemini-code-assist]

[HIGH] Avoid panicking with unwrap() in parser code. Evidence: crates/engine/src/parser/oracle_nom/quantity.rs:4031. Why it matters: If a keyword name is parsed but fails to convert to the Keyword enum, the parser will panic and crash the engine instead of failing gracefully. Suggested fix: Use map_res to handle the parsing error gracefully and return a nom error.

Suggested change

	let (rest, keyword_name) = parse_keyword_name(rest)?;
	let keyword: Keyword = keyword_name.parse().unwrap();
	let (rest, keyword) = map_res(parse_keyword_name, |s: &str| s.parse::<Keyword>()).parse(rest)?;

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. Nom combinators on the first pass — no exceptions. ^(link)
2. Avoid swallowing all errors or panicking when only specific errors are expected to be handled or ignored. Propagate unexpected errors or map them to proper parser errors to prevent masking critical bugs or crashing the engine.

[luciferlive112116]

Applied in 96f65749b — routed the keyword conversion through map_res(parse_keyword_name, |s: &str| s.parse::<Keyword>()), so an unconvertible name now fails the parse as a graceful nom error instead of panicking. (Every KEYWORDS entry has a valid FromStr, so in practice this is a defensive guard, not a reachable path.)

[luciferlive112116]

resolved

[matthewevans]

Holding this for maintainer review evidence: this PR changes parser/engine files, but the required parse-diff sticky comment is not present yet and the only reported checks are the triage-label checks. I’m not approving or enqueueing until the parser/card-data CI evidence is available for this head.

[matthewevans]

Holding this current head for the same evidence gate: it changes parser files, but the required parse-diff sticky is still absent and the only reported check on this head is the triage-label workflow. I’m not approving or enqueueing until parser/card-data CI has run and the parse-diff evidence is available for this head.

[matthewevans]

Holding current head ab43e0a059f28f5636865dc958c5d76d2d8a297f for required parser/card-data evidence. This PR changes parser files, but the parse-diff sticky is still absent and the only visible check on this head is the triage-label workflow, so I cannot review or enqueue it yet.

[luciferlive112116]

Rebranched onto latest main — the merge conflict is resolved and the branch is now conflict-free (mergeable: true). Net PR diff vs main is unchanged (the two parser files). Locally re-verified against current main: cargo fmt --all -- --check and cargo check -p engine --lib --tests both clean. Ready for a workflow-run approval so the parser/card-data CI can post its parse-diff evidence.

[matthewevans]

Holding current head 3fc7033928a640089a1c35d2dbcbeea88faea150 for required parser/card-data evidence. This PR still changes parser files, but the parse-diff sticky is absent and the only visible check on this head is the triage-label workflow; local cargo check is not a substitute for the repo parse-diff/card-data evidence, so I cannot complete review or enqueue it yet.

[matthewevans]

Holding this head for required proof before review/approval. The PR packet has proof_gap=true with unchecked verification items; please complete the AI-contributor proof/verification sections and provide concrete current-head test evidence.

[matthewevans]

Holding current head da17016acd015205dc9e521febc8c862319c63da for required parser/card-data evidence. This PR changes parser files, but the parse-diff sticky is absent on the current head and Rust/card-data checks are still in progress, so I cannot complete review or pass it to the handler yet. The local proof gate also still reports missing AI-contributor proof/verification sections; please keep the PR proof current with concrete current-head test evidence.

[matthewevans]

Processed current head da17016acd015205dc9e521febc8c862319c63da again after CI completed.

I’m still holding this rather than reviewing/approving: this PR changes parser source, but the required coverage-parse-diff sticky is absent for the current head even though CI is now green, so the card-level parser evidence is still missing. The local packet also still reports a proof gap: missing AI-contributor proof/verification sections and unchecked /engine-implementer evidence. Please update the PR proof with concrete current-head verification and get the parse-diff evidence posted for this head.

[matthewevans]

Requesting changes on current head da17016acd015205dc9e521febc8c862319c63da.

[MED] The required parser/card-data evidence is still missing after CI completed. Evidence: this PR changes crates/engine/src/parser/oracle_nom/quantity.rs, but there is still no <!-- coverage-parse-diff --> sticky for this head even though the current Rust/card-data CI checks are green. Why it matters: this parser change claims to recover a class of card parses, and without the card-level parse-diff I cannot verify the blast radius is limited to the claimed for each <type> you control with <keyword> class. Suggested fix: get the coverage parse-diff posted/regenerated for this exact head and confirm it matches the PR scope.

[MED] The PR proof is still incomplete for the current head. Evidence: the local review packet reports proof_gap=true, missing AI-contributor proof/verification sections, and no checked test evidence for this head. Why it matters: this contributor is on elevated scrutiny and the PR changes parser support, so green CI alone is not enough review proof. Suggested fix: update the PR body with concrete current-head verification evidence for the new parser branch and the production static parsing path.

[matthewevans]

Parse changes introduced by this PR

✓ No card-parse changes detected.

[GonuDvc]

Good improvement overall. One edge case: empty string vs null aren't treated the same downstream — might need a normalization step.

[matthewevans]

Reviewed current head da17016. This clears the stale blocker: the prior unwrap issue is fixed, the change lands at the canonical nom quantity seam before the shorter bare controlled-type arm, and the tests cover both the reusable keyword/count class and the production static P/T route. Parse-diff reports no card-output changes, which I’m treating as no blast radius rather than no value because the PR moves the class into the canonical parser path. Approved for merge queue.

[matthewevans]

Thanks for rebasing. I can't keep the prior approval on this current head because the merge left the parser/test files structurally broken and also appears to drop an existing parser branch.

[HIGH] Current head does not compile after the latest merge. Evidence: crates/engine/src/parser/oracle_nom/quantity.rs:4083 has parser statements at module scope, followed by an unmatched closing brace at quantity.rs:4107. Why it matters: this is current-head breakage, so the parser tests and parse-diff evidence cannot be trusted until the file parses again. Suggested fix: restore the intended parse_for_each_controlled_type_with_keyword function wrapper/body and rerun current-head CI/parse-diff.

[HIGH] The merge also leaves a broken partial test body at module scope. Evidence: crates/engine/src/parser/oracle_static/tests.rs:24063 references def after the test setup that created it has been removed. Why it matters: this both prevents compilation and removes the production parse_static_line discrimination this PR needs. Suggested fix: restore a complete production static test for ~ gets +1/+1 for each other creature you control with flying. that asserts the dynamic P/T uses the keyword-filtered count.

[MED] The current diff drops the existing counter-qualified for each parser seam while adding the keyword seam. Evidence: crates/engine/src/parser/oracle_nom/quantity.rs:3055 now dispatches directly to parse_for_each_controlled_type, and the removed/partial block around quantity.rs:4083 is where the qualified branch was split. Why it matters: fixing only the syntax would still risk regressing High Sentinels of Arashin / Armorcraft Judge / Inspiring Call style counts. Suggested fix: keep the counter-qualified arm/function and insert the new keyword-qualified sibling before the bare controlled-type arm.