fix(engine): Halana and Alena Partners where-X counter count (#1993)

[kiannidev]

Summary

Fixes #1993 — Halana and Alena, Partners triggered at beginning of combat with correct targeting, but no +1/+1 counters were applied.

Root cause: Multi-sentence oracle text (… where X is ~'s power. That creature gains haste …) made strip_trailing_where_x capture the haste sentence into the X binding. Runtime resolved that bogus Variable name as 0 counters.

Changes:

• Truncate where-X defining clauses at the next sentence boundary (. ) in strip_trailing_where_x.
• Map printed card-name possessives (Halana and Alena's power) to Power { scope: Source } before self-ref normalization.

Test plan

• cargo test -p engine --lib strip_trailing_where_x_stops_at_next_sentence
• cargo test -p engine --lib where_x_printed_name_possessive_power_is_source
• cargo test -p engine --lib halana_alena_partners_combat_trigger_puts_source_power_counters
• Manual: Halana and Alena (e.g. 3 power) + another creature you control → 3 +1/+1 counters and haste at beginning of combat on your turn

[gemini-code-assist]

Code Review

This pull request implements support for parsing printed-name possessives in where-X bindings (e.g., "Halana and Alena's power") to refer to the ability source under CR 107.3i, and updates strip_trailing_where_x to handle where-X clauses that are not the final sentence. Feedback highlights violations of Rule R1 regarding the use of non-nom string operations (.starts_with(), .strip_suffix(), and .split_once()) for parsing dispatch and processing, and flags a duplicate #[test] attribute in the unit tests.

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.

[matthewevans]

🤖 Architecture Review (automated)

Verdict: ⚠️ Changes requested

Seam: CONCERN — semantic binding (printed name → ability source via ObjectScope::Source/QuantityRef::Power) is correct and consumed at runtime, but the new dispatch is hand-rolled starts_with/strip_suffix instead of nom, the one peer (parse_event_context_quantity) it sits beside is fully combinator-built.
Idiomatic: CONCERN — parse_where_x_printed_name_possessive_stat violates the repo's #1 recurring rule (nom combinators for parsing dispatch); a duplicate #[test] attribute will fail CI under -D warnings.
Value: Covers a small class ("where X is <printed name>'s power/toughness" — partner/legendary self-referential counter cards), not just Halana and Alena, but the denylist gate is fragile (see below).

Reconciled with existing reviews (Gemini):

• CONFIRMED [HIGH] parse_where_x_printed_name_possessive_stat uses .starts_with() (BLOCKED_PREFIXES) + .strip_suffix() for parsing dispatch. Read oracle_quantity.rs:1102+ — the adjacent parse_event_context_quantity is built entirely from tag/alt/all_consuming. This is parsing dispatch, so the nom mandate applies. Gemini's suggested not(alt(...)) + terminated(take_until(...)) rewrite is the right shape (drop the unused subject binding once take_until is in place).
• CONFIRMED [MEDIUM] duplicate #[test] on strip_trailing_where_x_stops_at_next_sentence (diff lines 123-124 / file ~4933). I reproduced with rustc --test: it emits duplicate_macro_attributes (warn-by-default). CI runs cargo clippy --workspace --all-targets -- -D warnings (.github/workflows/ci.yml:50), which promotes it to an error. This is merge-blocking, not cosmetic.
• CONFIRMED [MEDIUM] .split_once(". ") on the String in strip_trailing_where_x is parsing-dispatch string surgery; prefer after.split_around(". ") on the TextPair before .to_string() (as Gemini suggests) to stay on the combinator/helper path and avoid the extra allocation.

Findings

• [BLOCKER] crates/engine/src/parser/oracle_effect/lower.rs:~4933 — duplicate #[test] attribute fails CI (duplicate_macro_attributes + -D warnings --all-targets). Delete one.
• [HIGH] crates/engine/src/parser/oracle_effect/lower.rs:~4344-4391 (parse_where_x_printed_name_possessive_stat) — replace starts_with/strip_suffix dispatch with nom combinators per the repo's non-negotiable parser rule. The power/toughness branch is exactly an alt((terminated(take_until("'s power"), tag("'s power")).map(..), ...)).
• [MEDIUM] Same function — the BLOCKED_PREFIXES denylist is an allow-by-default heuristic: any subject NOT starting with a listed determiner binds to ObjectScope::Source. Determiner-led forms outside the list (e.g. "each creature's power", "another creature's power", "twice ~'s power") would mis-bind to Source. Most are intercepted earlier by parse_cda_quantity/the "the " prefix today, so this is latent — but the gate's correctness depends on a denylist that doesn't enumerate the real complement. An allowlist (proper-noun / no-leading-determiner) is the class-correct shape; at minimum document why the upstream parse_cda_quantity ordering makes the residual forms unreachable.
• [MEDIUM] crates/engine/src/parser/oracle_effect/lower.rs:4174 and :4327 — CR citation accuracy. CR 107.3i ("all instances of X have the same value") is not the rule that (a) binds a where-X value or (b) resolves a printed-name possessive to the source. The value definition is CR 107.3f; the source reference is CR 113.7 (already cited in ObjectScope::Source's own doc-comment). The sentence-boundary strip is a parser concern, not a 107.3i concern. Re-cite to 107.3f / 113.7.

Semantics verified correct otherwise: QuantityExpr::Ref { QuantityRef::Power { scope: ObjectScope::Source } } is the right binding for "Halana and Alena's power", ObjectScope::Source resolves to the ability source (CR 113.7), and PutCounter.count reads the QuantityExpr at runtime (not a parsed-but-unused field).

[kiannidev]

Hi, @matthewevans
Thanks for the detailed review — all three actionable items are in the latest push:

Duplicate #[test] — removed (CI blocker).
parse_where_x_printed_name_possessive_stat — rewritten with nom combinators (not(alt(...)) + verify(take_until(...)) + all_consuming), same shape as the adjacent quantity parsers.
strip_trailing_where_x — mid-sentence truncation now uses TextPair::split_around(". ") instead of .split_once() on a String.
On the denylist vs allowlist note: agreed the gate is heuristic. The blocked-prefix list mirrors determiner/participle forms already owned by parse_cda_quantity and parse_event_context_quantity; printed-name possessives like Halana and Alena's power are the residual class this hook covers. Documented in the function doc comment; happy to tighten further if you want an explicit allowlist shape.

CR citations updated to 107.3f (where-X value definition) and 113.7 (ability source) instead of 107.3i for the possessive binding. Sentence-boundary stripping is cited as 608.2c (instruction order), not 107.3i.

Semantics unchanged: Power { scope: Source } for the Partners trigger, with the existing regression tests still passing.

[matthewevans]

Maintainer update pushed in 38f104014c.

What changed:

• Merged current origin/main into the contributor branch and resolved the oracle_effect/lower.rs import conflict by preserving both main's attack imports and this PR's ObjectScope use.
• Added a runtime regression for Halana and Alena, Partners that builds from Oracle text, enters beginning of combat, resolves the trigger, and asserts the receiver gets 3 +1/+1 counters from source power.

Review notes:

• Confirmed the earlier Gemini/review findings are resolved at head: printed-name possessive parsing uses nom, the where-X sentence split uses TextPair::split_around, and the duplicate test attribute is gone.
• Verified CR references locally against docs/MagicCompRules.txt: 107.3f, 107.3i, 113.7, 122.1, 603.2b, and 608.2c.
• No ai-contribution label applied: this was a bounded maintainer test/merge cleanup, not significant refactoring.

Local verification run:

• cargo fmt --all
• ./scripts/check-parser-combinators.sh
• cargo test -p engine --test integration halana_alena_partners_runtime_puts_source_power_counters -- --nocapture
• cargo test -p engine --lib where_x_printed_name_possessive_power_is_source -- --nocapture
• cargo test -p engine --lib halana_alena_partners_combat_trigger_puts_source_power_counters -- --nocapture
• git diff --check / git diff --cached --check

[matthewevans]

Approved after maintainer pass: review comments resolved, focused parser/runtime regressions pass locally, and CI is running on the pushed head.