fix(parser): recognize "the active player has controlled continuously since the beginning of the turn" target clause (Nettling Imp, Norritt, Arcum's Whistle)

[rykerwilliams]

Summary

Fixes a real 3-card class where the target-selection relative clause "the active player has controlled continuously since the beginning of the turn" was silently dropped by the parser, over-broadening the target filter to any non-Wall creature instead of one continuously controlled by the active player.

Affected cards (all share the identical clause verbatim, confirmed via a full-corpus grep of data/mtgjson/AtomicCards.json — exactly 3 hits, no more):

• Nettling Imp
• Norritt
• Arcum's Whistle

Total War shares the same continuity phrase but in a structurally different "except for" mass-exemption grammar (a DestroyAll exclusion clause, not a target-selection clause) — confirmed out of scope by tracing parse_except_for_type_list_suffix's decline-guard (oracle_target.rs:6279-6339), which correctly declines Total War's filtered-subset clause rather than silently mishandling it. Left untouched.

Root cause & fix

parse_ownership_or_controller_suffix (crates/engine/src/parser/oracle_target.rs) had no recognition arm for this clause, so it fell through and the whole relative clause was dropped. Added a new arm that:

1. Sets controller = Some(ControllerRef::ActivePlayer)
2. Pushes FilterProp::ControlledContinuouslySinceTurnBegan

Both types are pre-existing, reused verbatim — zero new enum variants. FilterProp::ControlledContinuouslySinceTurnBegan is already fully wired through runtime evaluation at game/filter.rs:4124 (!obj.summoning_sick — CR 302.6's summoning-sickness flag, cleverly reused since "controlled continuously since the beginning of the turn" is exactly that rule's definition).

No ControllerRef::You variant was added for a speculative "you've controlled continuously..." form — grepped all 8 occurrences of the continuity phrase across the corpus; the only real-card class using it as a target-selection clause is these 3 cards (all ActivePlayer-scoped). Building that branch ahead of a card that needs it would be exactly the kind of unexercised surface area this project's "build for the class, not the card" principle warns against in the other direction.

Gate A — parser combinator compliance

$ ./scripts/check-parser-combinators.sh
(exit 0, no violations)

The new arm is a single tag::<_, _, OracleError<'_>>("the active player has controlled continuously since the beginning of the turn").parse(own_ctrl) — pure nom combinator, mirroring the five neighboring arms in the same function. No contains()/starts_with()/find() dispatch.

Gate B — anchored citations (≥2)

• crates/engine/src/game/filter.rs:4124 — FilterProp::ControlledContinuouslySinceTurnBegan => !obj.summoning_sick, the pre-existing runtime evaluation this fix's parser output now reaches.
• crates/engine/src/parser/oracle_target.rs:5456 — the new arm itself, placed after the "owned by" passive block and before the parse_controller_suffix delegate fallback (the same seam as the existing "you own and control" precedent arm a few lines above it).

CR references

• CR 102.1 — defines the active player (the controller scope).
• CR 302.6 — summoning-sickness rule; !obj.summoning_sick is exactly this rule's continuity test.
• CR 508.1a — near-verbatim match to the card text: "...must either have haste or have been controlled by the active player continuously since the turn began."
• CR 608.2i — cited to contrast: this is a live target-selection restriction, not the past-tense look-back pattern parse_controller_suffix's existing "the active player controlled" arm handles.

Tier

Tier: Standard

Validation performed

• cargo fmt --all — clean
• ./scripts/check-parser-combinators.sh — exit 0
• Parser dispatch grep gate (no string-dispatch primitives in added lines) — clean
• CR-annotation diff gate — all 4 citations verified against docs/MagicCompRules.txt, zero UNVERIFIED
• cargo clippy -p engine --all-targets -- -D warnings — clean, zero warnings (re-verified post-rebase onto current main)
• Two discriminating tests, both passing via a full cargo test -p engine compile pre-rebase:
  • parse_target_active_player_controlled_continuously_since_turn_began (parser shape test — asserts the exact TargetFilter emitted)
  • build_target_slots_active_player_controlled_continuously_since_turn_began (runtime test through the real build_target_slots production entry point — 3 fixtures proving both the controller and continuity predicates gate legality independently: only a continuously-active-player-controlled creature is legal; a summoning-sick active-player creature and a continuously-controlled non-active-player creature are each excluded by their respective predicate)
• Independent review (/review-impl) — clean, no blocking findings

Validation NOT performed (disclosed honestly)

cargo coverage and cargo semantic-audit — the corpus-wide audits that would directly confirm Nettling Imp/Norritt flip to supported: true in the full card database — could not be run due to an environment issue unrelated to this code change: ./scripts/gen-card-data.sh's token-set-fetch step (fetch-token-sets.sh) failed 100% of the time, but only when invoked through its specific nested-subprocess execution chain. This was diagnosed exhaustively today:

• Not resource contention — new failures kept appearing at the same rate even with the system independently confirmed fully idle.
• Not a server-side block or 404 — the exact failing URLs return HTTP 200 in <0.3s via direct manual curl.
• Not a bug in the shared fetch library — mtgjson_download/mtgjson_curl (scripts/lib/mtgjson-fetch.sh) succeeded when invoked interactively, inside a set -euo pipefail subshell, via a detached background task, and in a 7-code sequential loop — every isolated reproduction succeeded.
• Not session-specific networking — no proxy env vars, and the exact same session's manual curl calls succeeded.

The failure is isolated to something specific to the deep-nested subprocess chain gen-card-data.sh → fetch-token-sets.sh → curl, and remains unexplained. Given the additive, narrowly-scoped nature of this change (a single new tag() arm reusing two fully-wired pre-existing types) and the strength of the verification that did complete (clean clippy under -D warnings, passing discriminating tests through a full compile, all parser gates clean), the regression risk from skipping the corpus-wide audit is low — but I'm not claiming that verification happened.

Post-rebase, the targeted test suite itself (cargo test -p engine --lib active_player_controlled_continuously) also could not be re-run to completion — 5 consecutive attempts were killed mid-compile by what appears to be an unrelated background-task environment issue (not a build/test failure; no error output, always killed at the identical "Compiling engine v0.20.0" stage, with the system confirmed idle each time). The tests were confirmed passing pre-rebase via a full compile, and post-rebase clippy (which compiles the same code under the stricter lint pass) is clean, so I'm confident in the fix, but flagging this honestly rather than silently treating an unconfirmed post-rebase re-run as confirmed.

Backlog hygiene

Removed Nettling Imp and Norritt from docs/parser-misparse-backlog.md's category 1 list, decrementing that category's count (752→750) and the top-summary totals (4778→4776, 4812→4810) accordingly. Arcum's Whistle was never listed there — it has a separate, unrelated "unless controller pays X" cost-side gap, out of scope for this fix.

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request adds parsing and game engine support for target restrictions requiring continuous control by the active player since the beginning of the turn (such as for Nettling Imp and Norritt). Feedback on the changes suggests refactoring the parser implementation to avoid verbatim string matching on the full Oracle phrase, recommending instead to decompose it using modular nom combinators like 'preceded' to align with the repository's architectural rules.

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] Verbatim string matching on full Oracle phrases.

Evidence: crates/engine/src/parser/oracle_target.rs:5455-5463.

Why it matters: Matching the entire phrase verbatim bypasses the robust, modular design of the parser, violates the sibling coverage lens (L2) by failing to support other potential controller variants, and risks combinatorial explosion.

Suggested fix: Decompose the phrase into modular components using nom combinators like preceded to separate the subject from the continuity predicate.

Suggested change

	if let Ok((rest, _)) = tag::<_, _, OracleError<'_>>(
	"the active player has controlled continuously since the beginning of the turn",
	)
	.parse(own_ctrl)
	{
	*controller = Some(ControllerRef::ActivePlayer);
	properties.push(FilterProp::ControlledContinuouslySinceTurnBegan);
	return own_ctrl_offset + (own_ctrl.len() - rest.len());
	}
	if let Ok((rest, _)) = preceded(
	tag("the active player"),
	tag(" has controlled continuously since the beginning of the turn"),
	)
	.parse(own_ctrl)
	{
	*controller = Some(ControllerRef::ActivePlayer);
	properties.push(FilterProp::ControlledContinuouslySinceTurnBegan);
	return own_ctrl_offset + (own_ctrl.len() - rest.len());
	}

References

1. Every new parser dispatch under crates/engine/src/parser/ must use nom 8.0 combinators or delegate to existing helpers. Verbatim string equality on full Oracle phrases is the single most prohibited pattern in the codebase. ^(link)
2. 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 alt and tag sequences) to prevent combinatorial explosion and improve maintainability.

[matthewevans]

[MED] The added runtime test does not exercise the parser-to-target-selection path. Evidence: crates/engine/src/game/ability_utils.rs:9181-9194 constructs FilterProp::ControlledContinuouslySinceTurnBegan directly and calls build_target_slots, while crates/engine/src/parser/oracle_target.rs:8025-8038 only checks the isolated parse_target AST. Why it matters: reverting the new parser arm at crates/engine/src/parser/oracle_target.rs:5455-5462 leaves the runtime test green, so the claimed Nettling Imp/Norritt/Arcum's Whistle support can regress back to an over-broad parsed filter without a production-path test. Suggested fix: add an integration test that parses the real Oracle text through parse_oracle_text, reaches the target-selection pipeline, and proves the active-player and continuity exclusions fail on revert.

[MED] Required current-head parse-diff evidence is missing. Evidence: this head changes crates/engine/src/parser/oracle_target.rs, but the current GitHub checks are still IN_PROGRESS and no <!-- coverage-parse-diff --> sticky comment exists for head 2d7ecccf12055a2dbd536556fffcd08628284ba6. Why it matters: the review protocol requires the card-level parser blast-radius diff before accepting an engine/parser PR, and the current branch is also behind origin/main. Suggested fix: let the current checks complete (or rerun them after bringing the branch current), then attach/verify the parse-diff for this exact head before requesting re-review.

[matthewevans]

[MED] The new parser arm matches the entire Oracle clause as one verbatim tag, contrary to the repository's parser architecture rule. Evidence: crates/engine/src/parser/oracle_target.rs:5455-5458 uses tag("the active player has controlled continuously since the beginning of the turn"); the same file documents the decomposed nom approach for the analogous continuity clause at crates/engine/src/parser/oracle.rs:2570-2588. Why it matters: the one-string arm handles only this exact wording and duplicates a phrase axis that should be composable, making equivalent subject/verb/timing variants silently fall through and increasing parser special-case debt. Suggested fix: compose the subject, has controlled, and continuity-tail atoms with nom combinators (or extract a shared continuity suffix helper) while preserving the typed ActivePlayer plus ControlledContinuouslySinceTurnBegan result.

[github-actions]

Parse changes introduced by this PR · 4 card(s), 3 signature(s) (baseline: main 3b9e44ce3f6d)

3 card(s) · ability/TargetOnly · field target: creature non-Wall → controlled continuously since turn began the active player controls creature non-Wall

Examples: Arcum's Whistle, Nettling Imp, Norritt

1 card(s) · ability/ChangeZone · removed: ChangeZone (target=parent target, to=battlefield)

Examples: Nissa, Who Shakes the World

1 card(s) · ability/SearchLibrary · added: SearchLibrary (count=up to 2147483647, find=land Forest)

Examples: Nissa, Who Shakes the World

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

[matthewevans]

Re-reviewed head 2d7ecccf12055a2dbd536556fffcd08628284ba6 after CI completed. The required <!-- coverage-parse-diff --> evidence is now present and reports exactly the three intended cards (Arcum's Whistle, Nettling Imp, and Norritt), so that earlier evidence blocker is cleared.

The PR remains changes-requested for the two existing MED findings: the parser arm still matches the full Oracle clause as one verbatim tag instead of composable nom atoms, and the added runtime test still constructs the filter directly rather than parsing real Oracle text through the production target-selection path. No new finding was added.

[rykerwilliams]

Both MED findings addressed in d2b998b:

Verbatim tag() decomposed — parse_ownership_or_controller_suffix's new arm now composes three separate nom atoms (subject "the active player" / verb " has controlled" / continuity tail " continuously since the beginning of the turn"), sequenced via the same tuple-.map().parse() idiom the "owned by" arm directly above it uses — mirroring parse_continuity_exemption_clause (oracle.rs) as you cited. Behavior unchanged; ActivePlayer stays the only recognized subject.

Production-path regression test added — new crates/engine/tests/integration/nettling_imp_continuity_target.rs parses Nettling Imp's real Oracle text through parse_oracle_text, drives the parsed AbilityDefinition through build_resolved_from_def → build_target_slots, and asserts the legal-target set against the same 3 fixtures (continuous-active-player = legal; summoning-sick-active-player = illegal; continuous-non-active-player = illegal). This fails if the new parser arm regresses — the two pre-existing tests are retained as complementary layers, not replaced.

cargo fmt, check-parser-combinators.sh, clippy -p engine --tests -- -D warnings, and the full test suite (including the new integration test) are all green on this head.

[matthewevans]

Maintainer approval — clears the stale CHANGES_REQUESTED

This review supersedes my two prior CHANGES_REQUESTED reviews, both filed against the older head 2d7ecccf. Re-reviewing the current head d2b998be, both MED findings are resolved. Approving.

Finding 1 (verbatim tag → composable atoms) — RESOLVED

The continuity clause is now recognized by sequencing three composable nom atoms in a tuple parser (parse_ownership_or_controller_suffix, oracle_target.rs), not one full-clause tag:

let active_player_continuity = (
    tag("the active player"),
    tag(" has controlled"),
    tag(" continuously since the beginning of the turn"),
).map(|_| ()).parse(own_ctrl);

It reuses the pre-existing FilterProp::ControlledContinuouslySinceTurnBegan (present on main at types/ability.rs:3711, runtime-evaluated at filter.rs:4124 as !obj.summoning_sick) plus ControllerRef::ActivePlayer — no new enum variant is introduced.

Finding 2 (test bypassed the parser) — RESOLVED

New integration test nettling_imp_continuity_target.rs parses Nettling Imp's real Oracle text through the top-level parse_oracle_text, then drives the PARSER-PRODUCED AbilityDefinition through the production build_resolved_from_def → build_target_slots path. It has a positive reach-guard (asserts the parse produced both the ActivePlayer controller scope and the continuity property) and three discriminating fixtures — continuous-active (legal), summoning-sick active (illegal), continuous non-active (illegal) — with a revert-failing legal_targets == [continuous_active] assertion. Reverting the parser arm degrades the filter to non-Wall-creature-only, admitting the two hostile fixtures and failing the test. No vacuous negatives.

Parse-diff sticky: the Nissa delta is baseline drift, not blast radius (recorded so it is not re-litigated)

The sticky lists 4 cards. Three (Arcum's Whistle, Nettling Imp, Norritt) are the intended scope. The 4th — Nissa, Who Shakes the World (removed ChangeZone, added SearchLibrary) — is stale-baseline drift, confirmed:

• Parse-diff CI diffs the merge ref (pull/5463/merge, which contains main) against baseline 3b9e44ce.
• git merge-base --is-ancestor 42bb113537 3b9e44ce → false: the baseline predates 42bb113537 ("close the sentence after an emblem's granted-ability quote (Nissa)", #5465), which IS in current main.
• This PR's own diff (origin/main...d2b998be) touches zero Nissa/search/emblem/ChangeZone code — it is causally incapable of producing that delta.

CR citations (102.1 active player, 302.6 summoning-sickness continuity, 508.1a, 608.2i) verified against docs/MagicCompRules.txt. Oracle text of all three cards verified against Scryfall. Gemini's HIGH "verbatim string matching" finding was against the old head and is refuted on the current head.