Fix Lictor opponent-scoped "entered this turn" intervening-if

[keloide]

Lictor's Pheromone Trail — "When this creature enters, if a creature entered the battlefield under an opponent's control this turn, create a 3/3 green Tyranid Warrior creature token with trample" — dropped its intervening-"if" condition (parsed to None), so the ETB trigger fired unconditionally and the token was created every time Lictor entered.

The "under your control" surface of this class was already supported; this adds the opponent-scoped, past-tense mirror. A new parse_entered_this_turn_under_opponent_control combinator reuses the shared parse_or_more_entered_count / parse_entered_this_turn_subject helpers and carries the scope via PlayerScope::Opponent { Max } — the existential "an opponent" reading already documented on parse_opponent_had_entered_this_turn — over the CR 608.2i BattlefieldEntriesThisTurn snapshot. No new engine variant.

The opponent surface in the QUANTITY path stays honestly Unimplemented (no printed card); only the condition path, which Lictor uses, is added.

Adds parser unit tests (singular + count forms) and a discriminating runtime test that casts Lictor and asserts the token is gated on an opponent's entry (fails on revert). Removes Lictor from the parser misparse backlog.

Claude-Session: https://claude.ai/code/session_01Gw63XPowSxhpJaS9xM8Xyj

Summary

Implementation method (required)

Method: /engine-implementer

Note

Any change to crates/engine/ game logic — parser, effects, resolver,
targeting, rules behavior — is expected to go through /engine-implementer.
The "not used" box is for changes that genuinely fall outside that scope.

CR references

Verification

• Required checks ran clean, or the exact CI-owned alternative is stated below.

• Gate A output below is for the current committed head.

• Final review-impl below is clean for the current committed head.

• Both anchors cite existing analogous code at the same seam.

• <exact command or CI check> —

Gate A

Gate A PASS head=<40-hex-sha> base=<40-hex-sha>

Anchored on

• path/to/existing.rs:123 — analogous authority/pattern
• path/to/existing.rs:456 — second analogous authority/pattern

Final review-impl

Final review-impl PASS head=<40-hex-sha>

Claimed parse impact

Summary by CodeRabbit

• Bug Fixes
  • Corrected parsing of Lictor’s Flash condition for creatures that enter the battlefield under an opponent’s control this turn.
  • Ensured Lictor’s token is created only when the opponent-controlled entry requirement is satisfied (including proper handling of the “1 or more” vs “2 or more” thresholds).
  • Preserved the intended creature type restrictions during evaluation.
• Tests
  • Added integration coverage to prevent regressions for both token-created and token-not-created scenarios.
• Documentation
  • Updated the parser misparse backlog metrics and removed the implicated card entry.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro Plus

Run ID: 682bd82d-f053-404c-86a0-0439b90ca5d7

📥 Commits

Reviewing files that changed from the base of the PR and between 3d76725 and 03e8046.

📒 Files selected for processing (3)

• crates/engine/src/parser/oracle_nom/condition.rs
• crates/engine/tests/integration/lictor_opponent_entered_this_turn.rs
• crates/engine/tests/integration/main.rs

🚧 Files skipped from review as they are similar to previous changes (3)

• crates/engine/tests/integration/main.rs
• crates/engine/src/parser/oracle_nom/condition.rs
• crates/engine/tests/integration/lictor_opponent_entered_this_turn.rs

---

📝 Walkthrough

Walkthrough

The condition parser now handles opponent-controlled battlefield entries this turn, preserving opponent scope and thresholds. Unit and integration tests cover parsing and Lictor’s conditional token creation, while the misparse backlog removes Lictor from the affected category.

Changes

Lictor opponent entry condition

Layer / File(s)	Summary
Opponent-scoped condition parsing crates/engine/src/parser/oracle_nom/condition.rs	Routes the opponent-control surface to a dedicated parser, supports singular and “or more” forms, and adds unit coverage for opponent scope, thresholds, and creature filtering.
Lictor integration coverage crates/engine/tests/integration/lictor_opponent_entered_this_turn.rs, crates/engine/tests/integration/main.rs	Adds parsing and gameplay tests confirming tokens are created after an opponent creature entry but not after only player-controlled entries.
Misparse backlog update docs/parser-misparse-backlog.md	Removes Lictor from the dropped intervening-if category and updates its aggregate counts.

Estimated code review effort: 3 (Moderate) | ~20 minutes

Sequence Diagram(s)

sequenceDiagram
  participant OracleParser
  participant GameState
  participant LictorTrigger
  participant TokenCreation
  OracleParser->>LictorTrigger: Parse opponent-scoped entry condition
  GameState->>LictorTrigger: Provide battlefield entries this turn
  LictorTrigger->>TokenCreation: Create Tyranid Warrior token when condition is met

Loading

Possibly related PRs

• phase-rs/phase#6323: Refactors the ledger-backed BattlefieldEntriesThisTurn quantity semantics used by this parser change.

Suggested labels: bug

Suggested reviewers: matthewevans

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title clearly summarizes the main change: fixing Lictor's opponent-scoped entered-this-turn intervening-if parsing.
Docstring Coverage	✅ Passed	Docstring coverage is 100.00% which is sufficient. The required threshold is 80.00%.
Linked Issues check	✅ Passed	Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check	✅ Passed	Check skipped because no linked issues were found for this pull request.

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

_{Comment @coderabbitai help to get the list of available commands.}

[matthewevans]

Held on current head 3d767251. Manual implementation review finds the new opponent-scoped past-tense condition uses the existing per-opponent BattlefieldEntriesThisTurn authority and the integration tests cover both the trigger and its false branch. The parser/engine change still has no current-head <!-- coverage-parse-diff --> artifact because the card-data/Rust CI jobs are queued. That artifact is required evidence for this surface, so no approval or queue action yet.

[coderabbitai]

Actionable comments posted: 1

🤖 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/parser/oracle_nom/condition.rs`:
- Around line 12589-12614: Update
test_entered_this_turn_under_opponent_control_count to match aggregate:
AggregateFunction::Max within BattlefieldEntriesThisTurn, in addition to the
opponent player scope. Keep the existing comparator and threshold assertions
unchanged.

🪄 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: 6ea2206b-717a-432e-b8f3-c9b1d48dfbb6

📥 Commits

Reviewing files that changed from the base of the PR and between 1ddb30c and 3d76725.

📒 Files selected for processing (4)

• crates/engine/src/parser/oracle_nom/condition.rs
• crates/engine/tests/integration/lictor_opponent_entered_this_turn.rs
• crates/engine/tests/integration/main.rs
• docs/parser-misparse-backlog.md

[github-actions]

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

🟡 Modified fields (1 signature)

• 1 card · 🔄 trigger/ChangesZone · changed field condition: ∅ → battlefield entries this turn (creature, max of opponents) ≥ 1
  • Affected (first 3): Lictor

1 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

Right seam, correct scope carrier, and a genuinely discriminating runtime pair — one fixup-sized test-adequacy gap (the aggregate wildcard) stands between this and enqueue.

Reviewed at head 3d767251. Card premise, bug premise, blast radius, and all five CR citations independently verified below.

---

✅ Clean

Card premise is real. Scryfall !Lictor (Warhammer 40,000 Commander) returns verbatim:

Flash
Pheromone Trail — When this creature enters, if a creature entered the battlefield under an opponent's control this turn, create a 3/3 green Tyranid Warrior creature token with trample.

The PR body quotes this exactly. The apostrophe in the printed text is U+0027 straight (the only non-ASCII codepoint in the Oracle text is the U+2014 em-dash in the ability-word prefix), so the straight-apostrophe-only anchor at condition.rs:7838 matches real data and is not inert.

Bug premise is confirmed against generated card data, not taken on trust. In today's data/card-data.json, Lictor's ETB trigger carries the full intervening-"if" in its description while "condition": null — so the Effect::Token resolved unconditionally, exactly as claimed.

Right seam. parse_entered_this_turn_under_opponent_control is registered in the parse_event_history_conditions alt() chain (condition.rs:307), which is reached through parse_inner_condition — the single authority for game-state conditions. The combinator re-implements nothing: both branches delegate to the shared parse_or_more_entered_count / parse_entered_this_turn_subject helpers, so the count-form and singular-form grammar are inherited rather than duplicated. This is the delegation the misparse-backlog entry for root cause #2 asks for.

Ordering is safe. The earlier parse_entered_this_turn arm anchors on "entered the battlefield under your control this turn" (condition.rs:7639), a disjoint suffix, so take_until fails there and falls through cleanly. No shadowing.

Scope carrier is the correct one, and the reasoning in the doc comment is accurate. Verified against the runtime at game/quantity.rs:3366: BattlefieldEntriesThisTurn filters record.controller == scoped_player.id and reduces through resolve_per_player_scalar, so PlayerScope::Opponent { aggregate: Max } genuinely produces the per-opponent maximum rather than a cross-opponent sum. Carrying the scope here instead of injecting controller: Opponent into the TargetFilter is the load-bearing choice, and it mirrors parse_opponent_had_entered_this_turn (condition.rs:7806) rather than inventing a parallel idiom. No new engine variant.

All five CR citations grep-verify against docs/MagicCompRules.txt and describe the annotated code:

• CR 102.2 (L252) — two-player opponent definition
• CR 102.3 (L254) — multiplayer/team opponent definition
• CR 603.4 (L2592) — the intervening-"if" clause rule
• CR 608.2h (L2806) — information determined once, LKI fallback
• CR 608.2i (L2808) — the look-back exception that keeps a departed permanent counted

CR 608.2i in particular is the right citation for why the battlefield_entries_this_turn ledger is used over a live-board read.

The negative test is not vacuous. lictor_makes_no_token_without_opponent_entry (lictor_opponent_entered_this_turn.rs:139) asserts zero tokens, and its paired positive lictor_makes_token_when_opponent_creature_entered drives the same Oracle const through the same runner.cast(lictor).resolve() path and asserts one token. The positive is the reach-guard, so a parse failure or a dead resolve path cannot make the negative pass for the wrong reason. Both drive the real cast pipeline. Reverting the production change flips the negative from 0 to 1 — genuinely discriminating.

mod lictor_opponent_entered_this_turn; is registered at crates/engine/tests/integration/main.rs:648, so the file is not an inert false-green.

Blast radius measured directly, and it matches the claim. Across the full 35,477-card pool in data/card-data.json, exactly one card contains "entered the battlefield under an opponent's control this turn" — Lictor. Zero cards carry a curly-apostrophe variant of the phrase. The claimed parse impact of one card is therefore correct, and the alt-chain insertion cannot reach any other card.

Backlog bookkeeping is consistent — docs/parser-misparse-backlog.md decrements 4734→4733 distinct cards, 4768→4767 appearances, and root cause #2 591→590, with Lictor removed from that cluster's list. The arithmetic is internally consistent.

---

🟡 Non-blocking

[MED] The three shape assertions wildcard the very aggregate the design rests on. Evidence: crates/engine/src/parser/oracle_nom/condition.rs:12573 and :12603, and crates/engine/tests/integration/lictor_opponent_entered_this_turn.rs:85 — all three match player: PlayerScope::Opponent { .. }. AggregateFunction has three variants (Max, Min, Sum — types/ability.rs:6086), so { .. } also admits Sum, which is precisely the cross-opponent summation the doc comment at condition.rs:7826 identifies as wrong and only visible at three or more seats. Why it matters: if parse_entered_this_turn_under_opponent_control were later changed to emit Sum, every test this PR adds would still pass. Worth noting that the runtime semantics of Opponent { Max } is already pinned by an existing three-seat discriminator, opponent_entry_threshold_is_per_opponent_not_summed_across_opponents (game/restrictions.rs:3078) — but that test drives the Whiplash Trap phrase, not Lictor's, so nothing currently binds the new combinator's aggregate. This confirms CodeRabbit's inline finding, and extends it to the third site CodeRabbit did not flag. Suggested fix: replace { .. } with { aggregate: AggregateFunction::Max } at all three sites.

[LOW] record_entry_now stamps a field the condition never reads. Evidence: crates/engine/tests/integration/lictor_opponent_entered_this_turn.rs:38 sets entered_battlefield_turn = Some(turn) alongside record_battlefield_entry. The BattlefieldEntriesThisTurn resolver (quantity.rs:3372) reads only state.battlefield_entries_this_turn, so that stamp is inert for these assertions and the helper's "exactly as record_zone_change does" doc claim is broader than what the test needs. Harmless as written; drop the stamp or narrow the comment if you touch the file.

[LOW] Parse-diff baseline is still pending. The <!-- coverage-parse-diff --> sticky (posted 18:27 UTC, unchanged since) reports Baseline pending for base 1ddb30cc, which landed on main at 17:41 UTC; main has since advanced six commits. Normally that would block on regenerated evidence, but the direct pool measurement above establishes the card-level impact set independently and exactly (one card, Lictor), so this is bookkeeping rather than a genuine evidence gap. Pushing the fixup above will re-run it against a current base and close it out.

---

Recommendation: push one fixup commit tightening the three PlayerScope::Opponent { .. } patterns to { aggregate: AggregateFunction::Max }, and this is ready to enqueue. That single commit also re-triggers the parse-diff against a current base, resolving the pending baseline at the same time. Nothing else needs to change.

Specific credit: routing the new surface through parse_or_more_entered_count / parse_entered_this_turn_subject instead of writing a fresh recognizer, carrying "under an opponent's control" on PlayerScope rather than in the TargetFilter, holding the QUANTITY-path opponent surface at honest Unimplemented because no printed card needs it, and shipping a revert-discriminating runtime pair with a real reach-guard — that is the shape this seam is supposed to grow in.

[matthewevans]

Approving at head 03e8046914a7b1689654e8bdef0563fa0e647d47.

Right seam and idiomatic at it: the new parse_entered_this_turn_under_opponent_control combinator routes through the existing parse_or_more_entered_count / parse_entered_this_turn_subject helpers rather than standing up a fresh recognizer, and carries the opponent scope on PlayerScope::Opponent { aggregate: Max } — the existential "an opponent" reading already established on parse_opponent_had_entered_this_turn — instead of smuggling it into the TargetFilter. No new engine variant. The QUANTITY-path opponent surface is deliberately left at an honest Effect::Unimplemented because no printed card needs it, which is the correct call over speculative surface.

Test discrimination verified: the runtime pair drives the real cast pipeline and gates the Tyranid Warrior token on an opponent's entry, with a paired positive reach-guard so the negative assertion cannot pass vacuously.

CodeRabbit's single actionable finding (pin AggregateFunction::Max in the shape assertions) was filed against 3d767251 and is already resolved at this head by 03e80469 — confirmed against the head, not the review timestamp: both test_entered_this_turn_under_opponent_control_singular and _count now pin aggregate: AggregateFunction::Max, closing the wildcard that also admitted Sum (wrong cross-opponent summation, observable only at 3+ seats).

All required checks green on this head.

[matthewevans]

Temporarily dequeued by a maintainer — nothing is wrong with this PR, and it will be re-enqueued shortly.

main is currently broken in a way that reds the merge queue for everyone. The release: v0.35.2 commit (21a53d50cb) bumped client/src-tauri/Cargo.toml to 0.35.2 without regenerating client/src-tauri/Cargo.lock, which still pins phase-tauri 0.35.1. The Tauri compile check job builds with cargo check --locked, so it fails:

error: cannot update the lock file /home/runner/work/phase/phase/client/src-tauri/Cargo.lock
       because --locked was passed to prevent this

That job is a needs: dependency of the required Rust (fmt, clippy, test, coverage-gate) aggregator, so it hard-blocks merges even though GitHub reports it as not required. Any merge-group ref that does not contain the fix fails and gets ejected — which is what was happening to the PRs in the queue, including this one.

The fix is #6568. It was queued behind the PRs it needs to unblock, which is a deadlock: the queue merges in order, so it could not land until those cleared, and they could not clear until it landed. Dequeuing the entries ahead of it is how that gets broken.

No action needed from you. Your PR keeps its approval and its head is unchanged. Once #6568 merges, this will be re-enqueued and should go through on the same green it already had. Apologies for the churn — this was our breakage, not yours.