Fix Orcish Farmer

[jofortin]

Summary

Fixes the dropped-effect-duration misparse (docs/parser-misparse-backlog.md root cause 25) for Orcish Farmer, whose {T}: Target land becomes a Swamp until its controller's next untap step. silently lost its duration and defaulted to Duration::Permanent (CR 611.2a) — no Effect::Unimplemented, no parse warning, the card read as fully supported while turning an opponent's land into a Swamp forever.

The root cause is that oracle_nom/duration.rs — the documented single authority for the phrase→Duration grammar — recognized the step-deadline family only as two hardcoded strings ("your next end step", "the next end step"). This factors it into one production over its two real axes (possessor × step), which also picks up the until [the beginning of] your next upkeep phrasing (9 cards print it). Every emitted pair reuses the existing Duration::UntilNextStepOf variant with existing Phase / PlayerScope values — no new engine variant, field, or serialized shape — and the one deadline that had no runtime expiry authority (Phase::Upkeep) gets one, mirroring the existing end-step pair.

Recognizing that duration also took Grinning Totem to zero Unimplemented, which would have read as fully supported while its required delayed cleanup stayed nonfunctional. Per review round 2 this PR keeps that clause honestly unimplemented instead: demote_unbound_delayed_sweeps is a post-lowering invariant that marks any delayed graveyard sweep whose swept objects were never bound to a concrete set. It covers the whole impulse-cleanup class — Grinning Totem, Bank Job, Glimpse the Impossible, Three Wishes, Elkin Lair — not one card.

Backlog list hygiene: Orcish Farmer removed from root cause 25 (29 → 28 cards; file totals 4733 → 4732 distinct, 4767 → 4766 appearances). It is the only backlog card this change fixes — Cycle of Life and Gabriel Angelfire also gain the duration but stay listed, because the misparses they are listed for (root cause 1's target restriction, root cause 12's modal split) are untouched.

Files changed

• crates/engine/src/parser/oracle_nom/duration.rs
• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/src/game/layers.rs
• crates/engine/src/game/turns.rs
• crates/engine/tests/integration/until_next_step_deadline_durations.rs
• crates/engine/tests/integration/main.rs
• docs/parser-misparse-backlog.md

Track

Non-developer

LLM

Model: claude-opus-5
Tier: Frontier
Thinking: high

Implementation method (required)

Method: not-applicable — /engine-implementer could not be run as specified. Its pipeline requires isolated planner/reviewer subagents, and subagent spawning was not authorized in this session; the skill is explicit that silently degrading to same-context self-review is the failure mode it exists to prevent. Every step of its checklist was executed inline instead — plan, CR verification against docs/MagicCompRules.txt, implementation, verification, and a /review-impl checklist pass against the committed diff — and the missing context isolation is recorded under Validation Failures rather than papered over.

CR references

All base numbers grep-verified against docs/MagicCompRules.txt (CR of 2026-06-19).

• CR 500.4 — the authorizing rule, newly cited: "As a step or phase begins, if there are effects that last until that step or phase, those effects expire." This is what every UntilNextStepOf prune implements; the pre-existing code cited only the per-step rules, which say when a step happens, not that durations end there.
• CR 502.1 / CR 502.3 — the untap step and its untap turn-based action (Orcish Farmer's deadline).
• CR 503.1 — the upkeep step (the new deadline and both of its prunes).
• CR 513.1 — the end step (existing deadline, re-cited at the refactored seam).
• CR 500.6 — "at the beginning of" abilities trigger as the step begins; justifies pruning before the upkeep triggers.
• CR 614.10a — "Anything scheduled for the 'next' occurrence of something waits for the first occurrence that isn't skipped"; justifies placing the prune after the skip-step check (an Eon-Hub-skipped upkeep must not expire the effect).
• CR 611.2a — a continuous effect lasts as long as stated, and with no stated duration lasts until end of game. Two roles here: it is the default that produced the Orcish Farmer bug, and it is the basis for the turn-agnostic reading — "the next <step>" states a step without naming whose it is, so the deadline is the first such step (PlayerScope::AnyTurn).
• CR 109.4 / CR 109.5 — object controller vs. "you/your": the two possessor readings the grammar must keep apart.
• CR 400.7 — "An object that moves from one zone to another becomes a new object with no memory of, or relation to, its previous existence": why a sweep's zone change must name the actual object, and why an unbound ParentTarget cannot.
• CR 305.7, CR 613.1d, CR 502.4, CR 704.5b — test assertions and test-scaffolding rationale.

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.

Track is declared Non-developer per the entry prompt, but the pinned toolchain (nightly-2026-04-19) turned out to be installable without elevated privileges, so the mechanical checks were run locally anyway. What could not be run is the card-data tier: ./scripts/setup.sh --agent requires jq and pnpm, neither installable here, so cargo coverage / cargo semantic-audit / cargo parser-gaps are CI-owned. A direct whole-corpus parse diff is supplied in their place.

• cargo fmt --all --check — clean.
• cargo clippy-strict (clippy --all-targets -- -D warnings, whole workspace) — clean, 0 warnings.
• cargo test -p engine — ok. 17883 passed; 0 failed; 6 ignored (lib) · ok. 4140 passed; 0 failed; 2 ignored (integration) · ok. 8 passed and ok. 9 passed (bin unit tests). No pre-existing test changed behavior.
• ./scripts/check-parser-combinators.sh — see Gate A below.
• cargo coverage — CI-owned; client/public/card-data.json cannot be generated here (jq/pnpm unavailable). Substituted by the whole-corpus parse diff under Claimed parse impact, measured over all 35,316 MTGJSON card faces.
• cargo semantic-audit — CI-owned, same reason. Orcish Farmer's post-fix AST was inspected directly: one Activated ability, SetBasicLandType { Swamp }, duration: UntilNextStepOf { step: Untap, player: Controller }, zero Effect::Unimplemented, empty parse_warnings.
• Revert-discrimination, run once per production hunk (each reverted alone, suite re-run, then restored):
  • revert the duration.rs step axes → the Orcish Farmer test and both upkeep tests fail.
  • revert only the turns.rs prune_until_next_upkeep_effects call → the upkeep continuous-effect test fails on its final assertion; the other two pass.
  • revert only the turns.rs prune_upkeep_step_casting_permissions call → the Elkin Bottle test fails on its final assertion; the other two pass.

Gate A

Gate A PASS head=abcf88deb7225cb9738df0e90ee66eeafe68da1d base=9da919f61039068cfe50fb219207f36e81dd57e5

Anchored on

• crates/engine/src/parser/oracle_nom/duration.rs:290 — parse_until_end_of_next_turn: the existing deadline production composing tag() + a possessive-pronoun sub-combinator + tag() into a Duration. parse_until_next_step mirrors that shape one axis wider, and now shares the very same pronoun combinator.
• crates/engine/src/parser/oracle_nom/duration.rs:126 — parse_current_phase_duration: the existing prefix-dispatch nesting (preceded(tag("this "), alt((..)))) that factors a shared head once and dispatches on the remainder — the pattern opt(tag("the beginning of ")) → possessor → tag(" next ") → step follows.
• crates/engine/src/game/layers.rs:218 — prune_until_next_end_step_effects: the existing UntilNextStepOf step-boundary prune for transient continuous effects, including its two-scope (Controller + AnyTurn) retain shape. prune_until_next_upkeep_effects (:261) is that function one step axis over.
• crates/engine/src/game/layers.rs:537 — prune_end_step_casting_permissions: the existing casting-permission half of a step deadline, including its exiled_by_ability_controller-before-granted_to keying. prune_upkeep_step_casting_permissions (:589) is that function one step axis over.
• crates/engine/src/game/layers.rs:180 — prune_controller_end_combat_step_effects: the existing affected-object-scoped member of the same prune family (it reads TargetFilter::SpecificObject), which is the convention PlayerScope::Controller carries for the untap deadline.
• crates/engine/src/game/turns.rs:2648-2654 — the Phase::End arm's prune_end_step_casting_permissions + prune_until_next_end_step_effects pair; the new Phase::Upkeep calls are placed the same way relative to that step's triggers.

Final review-impl

Final review-impl PASS head=abcf88deb7225cb9738df0e90ee66eeafe68da1d

Lenses applied against git diff upstream/main...HEAD, with the mechanical ones run as commands: correct seam · most-idiomatic-at-seam · class-vs-single-case · sibling coverage · test adequacy · vacuous-negative reach-guards · new-field threading (none added) · claim-to-test map · fixture path-divergence · coverage honesty · selected authority/provenance · snapshot/latch · serialized-surface contracts · target/scope matrix · nom-mandate (grep over added lines for .contains(", .starts_with(", .ends_with(", .find(", .rfind, .split, .splitn, .strip_prefix, .strip_suffix, string-literal match arms, and hand-built Effect::Unimplemented { .. } — zero hits) · CR-annotation diff gate (all 15 base numbers resolve in docs/MagicCompRules.txt) · new-bool scan (zero).

Two findings were raised and fixed before this commit; both are in the diff:

1. Free axis composition could emit a mis-scoped pair. PlayerScope::Controller is resolved against different players by the three prunes — the end-step and upkeep prunes gate on the effect's controller, the untap prune on the affected object's controller. Unconstrained composition therefore accepted until your next untap step and until its controller's next end step, each of which would expire on the wrong player's step. Fixed by step_deadline_scope, which declines any pair without a matching authority so the clause stays honestly Effect::unimplemented; pinned by test_parse_duration_rejects_mismatched_possessor_step_pairings.
2. The upkeep deadline needed a second authority. Elkin Bottle and Grinning Totem lower "Until the beginning of your next upkeep, you may play that card" to CastingPermission::PlayFromExile { duration }, not to a transient continuous effect, so prune_until_next_upkeep_effects alone left the play permission permanently active — strictly worse than the Unimplemented it replaced. Fixed by prune_upkeep_step_casting_permissions, mirroring prune_end_step_casting_permissions; pinned by a layers.rs unit test and by elkin_bottle_play_permission_expires_at_the_controllers_next_upkeep.

A third finding was raised, confirmed pre-existing, and disclosed rather than fixed: promoting Grinning Totem to zero Unimplemented makes an already-dropped intervening-if ("if you haven't played it") invisible to coverage. It belongs to root cause 2, which is in flight as #6089. See Claimed parse impact for the full statement.

Review round 2 — @matthewevans (CHANGES_REQUESTED)

[HIGH] Preserve Grinning Totem's mandatory delayed cleanup instead of silently promoting it to supported. Confirmed by driving the real pipeline (activate → search → exile → advance to my next upkeep): the unplayed opponent-owned card stays in exile and never reaches its owner's graveyard. My earlier reasoning that CR 603.7c would make the sweep a benign no-op was wrong.

Taking Option 2 as directed. demote_unbound_delayed_sweeps (parser/oracle.rs) keeps the clause honestly unimplemented. It runs as a post-lowering invariant beside the existing scrub_granting_placeholder_descriptions degrade net, because several builders emit CreateDelayedTrigger and the honesty requirement is a property of the final tree, not of one grammar arm.

Not a card-name special case — the guard is the shape "delayed graveyard move whose swept objects are an unbound anaphor", which is the whole impulse-cleanup class: Grinning Totem, Bank Job, Glimpse the Impossible, Three Wishes, Elkin Lair. Two of those (Bank Job, Glimpse the Impossible) were falsely supported on main for the same reason and now correctly report the gap; see Claimed parse impact for the measured effect and why it lands in the non-fatal coverage-honesty bucket.

Deliberately narrow, with both negatives reach-guarded: only inside a delayed trigger (the 77-card "search your library … put it into your graveyard" wording never reaches it) and only an unbound target, so Valakut Exploration's ExiledBySource sweep and Necropotence's tracked-set hand recall are untouched.

The parser anaphor/stamping work and the shared delayed-trigger dispatch investigation are split out as you asked, not pushed here.

One inherited nuance, deliberately not changed: prune_controller_untap_step_effects runs after the untap turn-based action rather than as the step begins (CR 500.4), because that ordering is load-bearing for the CantUntap class it was written for ("so the permanent skips exactly one untap"). For a basic-land-type change the difference is unobservable — no player receives priority during the untap step (CR 502.4) — so the shared authority is reused as-is rather than re-ordered under an unrelated card's PR.

Claimed parse impact

Measured, not estimated: the parser was run over all 35,316 card faces in MTGJSON AtomicCards (2026-07-27) on the base commit and on this branch's head, fingerprinting each face's complete parsed AST and counting Effect::Unimplemented. Exactly 14 faces differ.

Gained support (3) — the until … next upkeep clause now parses and nothing else in their text is deferred:

• Elkin Bottle (1 → 0)
• Erhnam Djinn (1 → 0)
• Xenic Poltergeist (1 → 0)

Lost support (2) — intentional, and the point of review round 2. Both were falsely supported on main for the same reason Grinning Totem was: their delayed sweep silently strands the swept card. The honesty net makes the gap visible:

• Bank Job (0 → 1)
• Glimpse the Impossible (0 → 1)

These are parser_regress ("coverage honesty"), not engine_regress, under scripts/coverage-regression-check.sh: the new gap key delayed_unplayed_exile_sweep is distinct from every handler those cards had supported: true for on the baseline, so no previously-supported handler is lost. That is the script's documented non-fatal bucket — "explicit Effect:* gaps for Oracle text the baseline parse tree had already swallowed into a broader supported node".

Duration recorded, support status unchanged (9) — the duration is now correct where it was previously dropped, but an independent second gap in the same clause remains, so none of these is claimed as newly supported:

• Orcish Farmer (0 → 0) — the fix: Permanent → UntilNextStepOf { Untap, Controller }
• Grinning Totem (1 → 1) — the review's requirement: the duration is recorded, and the card does not flip to supported, because the sweep now carries an honest marker
• Gabriel Angelfire (0 → 0) — duration recorded; the modal split is untouched (root cause 12, still listed)
• Cycle of Life (0 → 0) — duration recorded; the target restriction stays over-broad (root cause 1, still listed)
• Ertai's Familiar (1 → 1), Soul Echo (2 → 2), Spatial Binding (1 → 1) — duration recorded; "can't phase out" / damage-replacement still unimplemented
• Three Wishes (1 → 2), Elkin Lair (1 → 2) — already unsupported; they only gain the honest sweep key

Every card using the three phrasings that already worked (until your next end step, until the next end step, until [the end of] your/their next turn) is byte-identical across the diff, as are the 35,302 other faces.

Scope Expansion

The layers.rs + turns.rs runtime hunk goes beyond the parser seam the backlog's fix hint names, and it is required rather than incidental: Duration::UntilNextStepOf { step: Phase::Upkeep, .. } had no expiry authority on either the continuous-effect or the casting-permission side. Recognizing the phrase without adding one would have converted four honest Effect::Unimplemented gaps into four permanently-active effects — a silently-wrong outcome strictly worse than the gap, and exactly the class of harm root cause 25 catalogues. The hunk is two new prunes (each a mirror of its existing end-step counterpart), two call sites, and one AnyTurn arm added to the existing untap prune. No new type, variant, field, trait, or serialized shape; no exhaustive match anywhere in the workspace needed a new arm.

Review round 2 added a second, maintainer-directed piece of scope: the coverage-honesty net in parser/oracle.rs + the shared detector in oracle_effect/mod.rs. It adds no type, variant or field — only an Effect::unimplemented marker on a shape that provably cannot work. Per that review, the parser anaphor/stamping fixes and the shared delayed-trigger dispatch bug are not in this PR and will land as a separate engine-implementer-scoped PR with an end-to-end test proving the unplayed card reaches its owner's graveyard.

Explicitly not taken here: until that player's next turn / next end step (1 card each) and until its controller's next turn would need a target-relative PlayerScope with no current runtime authority. parse_until_next_step declines every such pairing, so those clauses stay honestly unimplemented rather than shipping a wrong deadline.

Validation Failures

The /review-impl pass was not context-isolated. This session could not spawn an independent reviewer agent, so the reviewer was the same context that authored the change. AI-CONTRIBUTOR.md §5 check 3 permits this only with the limitation stated in the PR body, and engine-implementer requires saying so rather than claiming the loop ran clean — hence this section. The pass was a real checklist run against the committed unified diff and did surface the two findings recorded above, both fixed with code before the final commit. Treat the review as author-run and weight maintainer review accordingly.

CI Failures

None. All checks passed on the previous head (afcc611): Rust lint/clippy/parser gate, both test shards, card data (generate, validate, coverage), WASM, Tauri, frontend. The CI parse-diff artifact independently reported the same 10 cards claimed above.

---

Review round 1 — @matthewevans (CHANGES_REQUESTED)

[HIGH] Correct the CR basis for the AnyTurn deadline expiry. Confirmed, not refuted: CR 603.7b reads "A delayed triggered ability will trigger only once—the next time its trigger event occurs", which is authority about delayed triggers, not about when a continuous effect or casting permission expires. Fixed in 367a98b: all 10 citations this PR introduced now read CR 611.2a + CR 500.4 — CR 611.2a fixes what the duration is (the effect lasts as long as stated; "the next <step>" names a step without naming whose it is, hence the first such step), CR 500.4 fixes when it ends (as that step begins). CR 502.3 / CR 503.1 / CR 513.1 remain only as descriptive context naming which step each prune owns, as suggested.

Scope note: the fix is limited to the sites this PR introduced. Pre-existing CR 603.7b citations are untouched — those in turns.rs and effects/rebound.rs annotate genuine delayed triggers and are correct, and layers.rs:229 predates this branch.

Annotations only; no behavior change. Gates re-run against the new head per the AI-contributor flow: Gate A above, full engine suite green (17864 lib / 4134 integration), cargo fmt --check and cargo clippy-strict clean, and the new check-prelowered-ratchet.sh gate passes.

Summary by CodeRabbit

• Bug Fixes
  • “Until your next upkeep” and “until the next untap step” effects now expire at the correct player’s step, including “any turn” behavior.
  • Upkeep-start pruning ensures “until next upkeep” transient effects and temporary casting permissions end at the right time, even when upkeep is skipped.
• Parser Improvements
  • Extended parsing for the full “until (the beginning of) next ” wording with stricter scope validation.
  • Improved handling of certain delayed graveyard sweep cases that should remain unimplemented.
• Tests
  • Added integration coverage for lifecycle and scoping of these “until next step” durations.

[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: 4f3c25c1-756a-4097-ae4e-1fd89721413b

📥 Commits

Reviewing files that changed from the base of the PR and between abcf88d and b5bfc81.

📒 Files selected for processing (3)

• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_tests.rs

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

• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/src/parser/oracle.rs

---

📝 Walkthrough

Walkthrough

The parser now supports validated next-step duration phrases and explicitly demotes unbound delayed sweeps. The game engine prunes upkeep- and untap-scoped state at turn boundaries, with unit and integration tests covering expiration behavior.

Changes

Step-scoped deadlines

Layer / File(s)	Summary
Next-step duration parsing crates/engine/src/parser/oracle_nom/duration.rs	Composes possessor and step parsing, validates scope combinations, and tests valid and invalid deadline phrases.
Runtime deadline pruning crates/engine/src/game/layers.rs, crates/engine/src/game/turns.rs	Prunes upkeep effects and casting permissions at upkeep, and handles turn-agnostic untap deadlines.
Behavioral validation and parser records crates/engine/tests/integration/*, crates/engine/src/game/layers.rs, docs/parser-misparse-backlog.md	Adds unit and end-to-end expiration scenarios, registers the integration module, and updates parser misparse counts and card listings.

Delayed-sweep parser honesty

Layer / File(s)	Summary
Delayed-sweep detection and demotion crates/engine/src/parser/oracle.rs, crates/engine/src/parser/oracle_effect/mod.rs, crates/engine/src/parser/oracle_tests.rs	Detects unbound delayed graveyard sweeps, demotes them to delayed_unplayed_exile_sweep, and tests bound and unbound cases.

Estimated code review effort: 4 (Complex) | ~60 minutes

Suggested labels: enhancement

Sequence Diagram(s)

sequenceDiagram
  participant OracleParser
  participant auto_advance
  participant DeadlinePruning
  participant GameState
  OracleParser->>GameState: create UntilNextStepOf duration
  auto_advance->>DeadlinePruning: process active_player upkeep
  DeadlinePruning->>GameState: remove expired effects
  DeadlinePruning->>GameState: remove expired casting permissions

Loading

🚥 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 is related to one major affected card, but it understates the broader parser and upkeep-deadline changes.
Docstring Coverage	✅ Passed	No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.
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

Warning

There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure.

🔧 ast-grep (0.44.1)

crates/engine/src/parser/oracle_effect/mod.rs

ast-grep timed out on this file

---

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

[matthewevans]

[HIGH] Correct the CR basis for the AnyTurn deadline expiry. Evidence: CR 603.7b governs delayed triggered abilities, not a continuous effect or casting permission. The new upkeep/untap AnyTurn code uses it as the justification for expiring “the next [step]” at the first occurrence (including the new duration.rs and layers.rs annotations/tests). Why it matters: the current citations justify different rules behavior than the continuous-effect/casting-permission durations this PR implements. Suggested fix: please correct or remove the inaccurate citation everywhere introduced by this PR; the expiry belongs under CR 500.4 + CR 611.2a, while any step-specific rule can remain only as descriptive context.

Since this is your first contribution, please use the AI contributor gates when updating the PR.

[github-actions]

Parse changes introduced by this PR · 14 card(s), 18 signature(s) (baseline: main fd59ea48f8b6)

🟢 Added (8 signatures)

• 4 cards · ➕ ability/delayed_unplayed_exile_sweep · added: delayed_unplayed_exile_sweep
  • Affected (first 3): Bank Job, Elkin Lair, Glimpse the Impossible (+1 more)
• 2 cards · ➕ ability/GrantCastingPermission · added: GrantCastingPermission (duration=until next upkeep (you))
  • Affected (first 3): Elkin Bottle, Grinning Totem
• 1 card · ➕ ability/can't · added: can't (duration=until next upkeep (you), kind=activated)
  • Affected (first 3): Spatial Binding
• 1 card · ➕ ability/choose · added: choose (duration=until next upkeep (you))
  • Affected (first 3): Soul Echo
• 1 card · ➕ ability/grant Landwalk · added: grant Landwalk (affects=parent target, duration=until next upkeep (you), grants=grant Landwalk, target=opponent controls creature non-Wall)
  • Affected (first 3): Erhnam Djinn
• 1 card · ➕ ability/set base power dynamic, set base toughness dynamic, add type artifact, add type… · added: set base power dynamic, set base toughness dynamic, add type artifact, add type creature (affects=parent target, duration=until next upkeep (you), grants=add t…
  • Affected (first 3): Xenic Poltergeist
• 1 card · ➕ ability/set land type Swamp · added: set land type Swamp (affects=parent target, duration=until next untap (you), grants=set land type Swamp, kind=activated, target=land)
  • Affected (first 3): Orcish Farmer
• 1 card · ➕ ability/~ · added: ~ (duration=until next upkeep (you), kind=activated)
  • Affected (first 3): Ertai's Familiar

🔴 Removed (8 signatures)

• 3 cards · ➖ ability/until · removed: until (kind=activated)
  • Affected (first 3): Ertai's Familiar, Spatial Binding, Xenic Poltergeist
• 2 cards · ➖ ability/CreateDelayedTrigger · removed: CreateDelayedTrigger (when=at next end step)
  • Affected (first 3): Bank Job, Elkin Lair
• 2 cards · ➖ ability/until · removed: until
  • Affected (first 3): Elkin Bottle, Grinning Totem
• 1 card · ➖ ability/CreateDelayedTrigger · removed: CreateDelayedTrigger (tracked=yes, when=at next end step)
  • Affected (first 3): Glimpse the Impossible
• 1 card · ➖ ability/CreateDelayedTrigger · removed: CreateDelayedTrigger (tracked=yes, when=at your next upkeep)
  • Affected (first 3): Three Wishes
• 1 card · ➖ ability/choose · removed: choose
  • Affected (first 3): Soul Echo
• 1 card · ➖ ability/gain · removed: gain
  • Affected (first 3): Erhnam Djinn
• 1 card · ➖ ability/remove all Land subtypes, add subtype Swamp · removed: remove all Land subtypes, add subtype Swamp (affects=parent target, duration=permanent, grants=add subtype Swamp, grants=remove all Land subtypes, kind=activat…
  • Affected (first 3): Orcish Farmer

🟡 Modified fields (2 signatures)

• 1 card · 🔄 ability/add chosen keyword · changed field duration: ∅ → until next upkeep (you)
  • Affected (first 3): Gabriel Angelfire
• 1 card · 🔄 ability/base power 0, base toughness 1 · changed field duration: ∅ → until next upkeep (you)
  • Affected (first 3): Cycle of Life

[jofortin]

Thanks — confirmed, not refuted. CR 603.7b reads "A delayed triggered ability will trigger only once—the next time its trigger event occurs", so it is authority about delayed triggers, not about when a continuous effect or a casting permission expires. That is exactly what the PlayerScope::AnyTurn deadline in this PR is, so the citation was justifying the wrong rules behavior.

Fixed in 367a98b. All 10 sites this PR introduced now cite CR 611.2a + CR 500.4:

• CR 611.2a fixes what the duration is — the effect lasts as long as stated, and "the next <step>" states a step without naming whose it is, which is precisely why the deadline is the first such step rather than a particular player's.
• CR 500.4 fixes when it ends — as that step begins.
• CR 502.3 / CR 503.1 / CR 513.1 remain only as descriptive context naming which step each prune owns, per your suggestion.

Scoped to what this PR introduced, as you asked. Pre-existing CR 603.7b citations are untouched: the ones in turns.rs and effects/rebound.rs annotate genuine delayed triggers and are correct, and layers.rs:229 predates this branch.

Annotations only — no behavior change, and the parse-diff is unchanged. Per AI-CONTRIBUTOR.md I re-ran the gates against the new head rather than letting the old records stand:

• Gate A PASS head=367a98ba83766c853d63f11a2d718f1dd9509744 base=e001db03558c2554802263e5df5a34c9a97132eb
• Final review-impl PASS head=367a98ba83766c853d63f11a2d718f1dd9509744
• cargo fmt --all --check clean · cargo clippy-strict clean · cargo test -p engine 17864 lib / 4134 integration, 0 failed · scripts/check-prelowered-ratchet.sh Gate P PASS

Both PASS lines in the PR body have been updated to this head. Branch is also merged up to current main (e001db0).

One thing I flagged in the body and want to make sure you see rather than discover in review: this PR takes Grinning Totem to zero Unimplemented, so it will likely read as supported: true, but its second sentence's "if you haven't played it" intervening-if is still dropped (condition: null). That drop is pre-existing — parsing that sentence alone gives the same result on main — but this PR is what makes it invisible to coverage. It looked like root cause 2 territory with #6089 already in flight, so I left it rather than widening scope. Happy to take it here instead if you'd prefer.

[matthewevans]

[HIGH] Preserve Grinning Totem's mandatory delayed cleanup instead of silently promoting it to supported.

Evidence:

• Grinning Totem's Oracle text requires: "At the beginning of your next upkeep, if you haven't played it, put it into its owner's graveyard."
• The current-head parse-diff adds GrantCastingPermission (duration=until next upkeep (you)) for Grinning Totem and removes the prior unsupported until signature.
• crates/engine/src/parser/oracle_nom/duration.rs:166-179 now accepts the first sentence's duration. At the deadline, crates/engine/src/game/layers.rs:589-640 only removes the casting permission; it creates no delayed zone-change instruction for an unplayed card.

Why this matters: the card can now appear fully supported while leaving an unplayed opponent-owned card in exile, rather than putting it into its owner's graveyard at the required upkeep. The prior unsupported marker made this gap visible; this PR removes that protection without implementing the second sentence.

Please either implement the generic delayed "at the beginning of your next upkeep, if [not played], put that card into its owner's graveyard" behavior and add a Grinning Totem pipeline test, or retain an honest unsupported marker until that clause is modeled. Please do not add a card-name special case.

[jofortin]

Confirmed — and investigating it turned up something that I think changes the right scope for this PR, so I want your call before pushing anything.

Your finding is correct, and worse than "the condition is dropped"

I built the pipeline test you asked for and drove the real flow (activate → search → exile → advance to my next upkeep):

exile after resolve                 = [(ObjectId(10), "Bogus Target", PlayerId(1))]
at P0's next upkeep (turn 4)        exile = ["Bogus Target"]   P1 graveyard = []

The opponent's card is stranded in exile permanently. I had guessed CR 603.7c would make the sweep a benign no-op; that was wrong, and I should not have shipped the disclosure in place of a test.

Two real parser bugs, both fixed locally

1. The graveyard-sweep anaphor is not recognized. contains_implicit_tracked_set_pronoun (oracle_effect/mod.rs) knows only hand recall ("put that card into your hand") and battlefield recall ("return it to the battlefield"). A graveyard destination matches neither, so uses_tracked_set stays false and the sweep's zone change falls back to ParentTarget — for Grinning Totem the targeted opponent, not the exiled card.

Fixed by adding a graveyard-sweep branch to that same recall axis. Two structural differences from the hand branch, both load-bearing: it must be scanned at word boundaries rather than start-anchored (the sweep never begins its clause — "At the beginning of your next upkeep, if you haven't played it, put it into …"), and its possessive axis is wider, because a swept card may be owned by another player (Grinning Totem: "its owner's graveyard"; Three Wishes: "your graveyard"). No card-name special case.

2. An intervening clause shadows the exile, so origin: Exile is never stamped. stamp_delayed_returns's zone lookup in oracle_effect/assembly.rs reads only defs.last(). Grinning Totem's sweep is preceded by the play-permission grant, not by the exile, so the stamp never fires. Fixed by scanning all prior clauses for the most recent zone destination — the identical shadowing fix the any_prior_publishes scan ~40 lines above already applies, with the same rationale ("an intermediate non-publishing clause must not shadow an earlier exile clause").

With both, the delayed trigger now registers and fires with exactly the shape the working analog has:

delayed FIRE cond=AtNextPhaseForPlayer{ phase: Upkeep, player: P0 }
  effect=ChangeZoneAll{ origin:"Exile", destination:"Graveyard", target:TrackedSet{1} }

The remaining blocker looks like a pre-existing engine bug, and it isn't mine

The card still doesn't move. A fired delayed trigger with that shape never reaches effects::resolve_effect at all. I isolated the variable:

case	shape	result
Necropotence class ({2}: Exile top card … put that card into your hand at the beginning of your next end step)	ChangeZoneAll{origin:Exile, dest:**Hand**, TrackedSet}	resolves, card reaches hand
same clause, own library, sweep	ChangeZoneAll{origin:Exile, dest:**Graveyard**, TrackedSet}	never dispatched
Grinning Totem verbatim	ChangeZoneAll{origin:Exile, dest:**Graveyard**, TrackedSet}	never dispatched

I separately ruled out the self-sacrifice cost and the opponent-owned card with their own fixtures. The only differing field is the destination: a fired delayed ChangeZoneAll into a graveyard is dropped before dispatch, where the same trigger into a hand resolves.

That is in shared trigger-batch machinery, has nothing to do with this PR's duration change, and likely affects other cards. I've stopped there rather than guess at a fix in that path.

What I'd like you to decide

1. Land the two parser fixes here and fix the graveyard-dispatch bug in a separate PR, leaving Grinning Totem honestly broken until then — but note that on its own this does not resolve your finding, since the card is still stranded and still reads as supported.
2. Option B after all — restore an honest unsupported marker for this PR and let the whole sweep (parser + runtime) land as its own change. Given what the investigation found, this now looks to me like the correctly-scoped choice rather than the lazy one, but it's a coverage-honesty decision that's yours.
3. I keep going and fix the graveyard dispatch too, if you can point me at the intended gate — I don't want to guess in that machinery.

This PR is untouched and still green at 367a98b: Gate A PASS head=367a98ba83766c853d63f11a2d718f1dd9509744 base=e001db03558c2554802263e5df5a34c9a97132eb, full engine suite passing, and the parse diff is still the same 10 cards. The two parser fixes are held locally as a patch, not pushed, so nothing above has changed what you're reviewing.

[matthewevans]

Thanks for tracing this through. Please take Option 2.

Do not push the parser-only fixes to this PR: they would still leave Grinning Totem falsely supported while its required delayed cleanup remains nonfunctional. Restore a generic honest unsupported marker here instead.

Please split the parser anaphor/stamping work and the shared delayed-trigger dispatch investigation into a separate, engine-implementer-scoped PR, with an end-to-end card test that proves the unplayed card reaches its owner's graveyard. Do not add a card-name special case.

The existing current-head changes request remains the acceptance gate for this PR. No additional review action is needed until the head changes.

[jofortin]

Taken Option 2. Pushed as 1c83532 (+ a merge to current main), head now abcf88d.

demote_unbound_delayed_sweeps (parser/oracle.rs) keeps the clause honestly unimplemented. It marks any delayed trigger whose inner chain is a graveyard move on an unbound anaphor — target still ParentTarget/Any, i.e. the parser's "I could not resolve which objects this pronoun names" state — with Effect::unimplemented("delayed_unplayed_exile_sweep", …).

It runs as a post-lowering invariant beside the existing scrub_granting_placeholder_descriptions degrade net rather than inside one grammar arm, because several builders emit CreateDelayedTrigger and the honesty requirement is a property of the final tree. (I first tried it in try_parse_at_next_phase_delayed_trigger and it was never reached on Grinning Totem's path — worth knowing if you touch this later.)

Not a card-name special case. The guard is the shape, and the shape is the whole impulse-cleanup class:

card	before	after
Grinning Totem	falsely supported	honest marker
Bank Job	falsely supported on main	honest marker
Glimpse the Impossible	falsely supported on main	honest marker
Three Wishes	already unsupported	gains the key
Elkin Lair	already unsupported	gains the key

Bank Job and Glimpse the Impossible were falsely supported on main for exactly the reason you identified, so the class guard picks them up too. That's two intentional coverage losses; measured over all 35,316 MTGJSON faces the PR now moves 14 faces — 3 gained, 2 lost, 9 duration-recorded-without-status-change. Grinning Totem is 1 → 1 against main: the duration is recorded and it does not flip to supported.

Those two flips land in coverage-regression-check.sh's non-fatal coverage-honesty bucket, not engine_regress: the new gap key is distinct from every handler those cards had supported: true for on the baseline, so new_engine_lost is empty. That matches the bucket's documented purpose ("explicit Effect:* gaps for Oracle text the baseline parse tree had already swallowed into a broader supported node"). Flagging it explicitly so it isn't a surprise in the parse-diff.

Narrow by construction, so it can't demote anything that works — only inside a delayed trigger (the 77 cards sharing the raw "search your library … put it into your graveyard" wording never reach it) and only an unbound target. Valakut Exploration's ExiledBySource sweep and Necropotence's tracked-set hand recall are both untouched, each pinned by a negative test that carries a positive reach-guard (parses with zero Unimplemented of any kind), so neither can pass vacuously.

As directed, the parser anaphor/stamping fixes and the shared delayed-trigger dispatch investigation are not in this PR. I'll open them separately, engine-implementer-scoped, with the end-to-end test proving the unplayed card reaches its owner's graveyard. For that PR's benefit, the dispatch bug isolates to the destination: ChangeZoneAll{origin:Exile, dest:Hand, TrackedSet} resolves, the same trigger with dest:Graveyard is dropped before effects::resolve_effect is ever called.

Gates re-run against the new head:

• Gate A PASS head=abcf88deb7225cb9738df0e90ee66eeafe68da1d base=9da919f61039068cfe50fb219207f36e81dd57e5
• Final review-impl PASS head=abcf88deb7225cb9738df0e90ee66eeafe68da1d
• cargo fmt --all --check clean · cargo clippy-strict clean · cargo test -p engine 17883 lib / 4140 integration, 0 failed · check-prelowered-ratchet.sh Gate P PASS
• Discrimination: reverting the one-line demote_unbound_delayed_sweeps call flips unbound_delayed_graveyard_sweep_stays_honestly_unimplemented to failing while the bound-recall test stays green.

Both PASS lines in the body are updated to this head, and the body's Claimed parse impact and Scope Expansion sections now carry the numbers above.

[coderabbitai]

Actionable comments posted: 3

🧹 Nitpick comments (2)

crates/engine/src/parser/oracle_effect/mod.rs (2)

13585-13600: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick win

Consider directed subtraction instead of QuantityExpr::Difference for the max-minus-current-lands count.

This file documents elsewhere (balance_clause_effect, and explicitly in the sibling parse_threshold_land_balance_ir a few hundred lines above) that QuantityExpr::Difference resolves via .abs(), and that safe usage requires the caller to guarantee left >= right. The sibling function computing the structurally identical "max lands minus my lands" quantity deliberately avoids Difference for exactly this reason, building Sum[max, Multiply(-1, my_count)] and wrapping it in up_to(...) instead — so a would-be-negative value clamps to 0 rather than getting silently flipped positive by .abs().

Here, left >= right almost certainly holds in practice (the max only rises across the resolution, and no iterating player can exceed it via this ability's own effect), so this is unlikely to manifest as a live bug — but it's inconsistent with the file's own documented defensive precedent for the identical shape, and the fix is a direct copy of the neighboring pattern.

♻️ Proposed fix mirroring `parse_threshold_land_balance_ir`

-    let difference = QuantityExpr::Difference {
-        left: Box::new(maximum_lands.clone()),
-        right: Box::new(QuantityExpr::Ref {
-            qty: QuantityRef::ObjectCount {
-                filter: scoped_lands,
-            },
-        }),
-    };
+    let difference = QuantityExpr::Sum {
+        exprs: vec![
+            maximum_lands.clone(),
+            QuantityExpr::Multiply {
+                factor: -1,
+                inner: Box::new(QuantityExpr::Ref {
+                    qty: QuantityRef::ObjectCount {
+                        filter: scoped_lands,
+                    },
+                }),
+            },
+        ],
+    };

🤖 Prompt for 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.

In `@crates/engine/src/parser/oracle_effect/mod.rs` around lines 13585 - 13600,
Replace the QuantityExpr::Difference used for the max-minus-current-lands
calculation with the directed subtraction pattern from
parse_threshold_land_balance_ir: build a Sum of maximum_lands and Multiply(-1,
the scoped player land count), then wrap it with up_to(...) so negative results
clamp to zero. Keep the existing maximum_lands and scoped_lands semantics
unchanged.

---

13544-13557: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low value

Dead branch: AggregateFunction::Min ("fewest") is parsed then unconditionally rejected.

The alt accepts either "most" or "fewest", but the very next statement rejects any result other than Max via a Verify error. Since "fewest" can never survive the check, this could simply be tag("most") directly. If the intent was to mirror the "parse a general extremum, then verify only Max is coherent" idiom used elsewhere in this file (parse_balance_arm_b, parse_catch_up_draw_ir), consider routing through the same shared extremum combinator those use instead of a fresh inline alt over literal words, per the "reuse existing shared building blocks before adding inline extraction logic" guidance.

As per path instructions for crates/engine/**/*.rs: "Reuse existing shared building blocks before adding utility or inline extraction logic; test the building block and its parameter range rather than a single card case."

🤖 Prompt for 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.

In `@crates/engine/src/parser/oracle_effect/mod.rs` around lines 13544 - 13557,
Remove the dead inline extremum branch in this parser: either parse only the
`"most"` literal when only Max is valid, or reuse the shared extremum combinator
used by `parse_balance_arm_b` and `parse_catch_up_draw_ir`, then retain
validation that only `AggregateFunction::Max` is accepted. Do not continue
parsing `"fewest"` through an unconditional rejection path.

Source: Path instructions

🤖 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_effect/mod.rs`:
- Around line 1693-1753: Update the documentation comment above
delayed_sweep_is_unbound_anaphor to replace the incorrect CR 611.2a citation
with the applicable Comprehensive Rules references covering delayed triggered
abilities and identifying objects across zones. Leave the function logic and the
existing CR 400.7 citation unchanged.

In `@crates/engine/src/parser/oracle.rs`:
- Around line 6728-6731: Update the demotion logic around
delayed_sweep_is_unbound_anaphor so TargetFilter::Any alone does not establish
an unbound anaphor; inspect or propagate typed binding provenance and demote
only cleanup chains lacking an earlier intended exile/permission binding.
Preserve valid unrestricted delayed graveyard effects, and add a regression case
covering a non-anaphoric Any target.
- Around line 6696-6710: Update the delayed-trigger citation for
demote_unbound_delayed_sweeps from CR 611.2a to CR 603.7, retaining CR 400.7
only if needed for the object-identity claim. Apply this documentation change in
crates/engine/src/parser/oracle.rs at lines 6696-6710 and
crates/engine/src/parser/oracle_tests.rs at lines 22946-22956; both sites
require the citation update.

---

Nitpick comments:
In `@crates/engine/src/parser/oracle_effect/mod.rs`:
- Around line 13585-13600: Replace the QuantityExpr::Difference used for the
max-minus-current-lands calculation with the directed subtraction pattern from
parse_threshold_land_balance_ir: build a Sum of maximum_lands and Multiply(-1,
the scoped player land count), then wrap it with up_to(...) so negative results
clamp to zero. Keep the existing maximum_lands and scoped_lands semantics
unchanged.
- Around line 13544-13557: Remove the dead inline extremum branch in this
parser: either parse only the `"most"` literal when only Max is valid, or reuse
the shared extremum combinator used by `parse_balance_arm_b` and
`parse_catch_up_draw_ir`, then retain validation that only
`AggregateFunction::Max` is accepted. Do not continue parsing `"fewest"` through
an unconditional rejection path.

🪄 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: 169e6115-7416-4ff5-af64-8c6967e96f4e

📥 Commits

Reviewing files that changed from the base of the PR and between 367a98b and abcf88d.

📒 Files selected for processing (5)

• crates/engine/src/game/turns.rs
• crates/engine/src/parser/oracle.rs
• crates/engine/src/parser/oracle_effect/mod.rs
• crates/engine/src/parser/oracle_tests.rs
• crates/engine/tests/integration/main.rs

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

• crates/engine/tests/integration/main.rs
• crates/engine/src/game/turns.rs

[coderabbitai]

🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift

Do not treat TargetFilter::Any as proof of an unbound anaphor.

The invoked predicate demotes every delayed graveyard move with ParentTarget or Any, without proving an earlier exile/permission binding was intended. Valid unrestricted delayed graveyard effects can therefore become Unimplemented. Carry or inspect typed binding provenance and demote only genuinely unbound cleanup chains; add a non-anaphoric Any regression case.

🤖 Prompt for 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.

In `@crates/engine/src/parser/oracle.rs` around lines 6728 - 6731, Update the
demotion logic around delayed_sweep_is_unbound_anaphor so TargetFilter::Any
alone does not establish an unbound anaphor; inspect or propagate typed binding
provenance and demote only cleanup chains lacking an earlier intended
exile/permission binding. Preserve valid unrestricted delayed graveyard effects,
and add a regression case covering a non-anaphoric Any target.

Source: Coding guidelines

[matthewevans]

Current-head maintainer review complete. The only follow-up corrects the delayed-trigger/object-identity annotations to CR 603.7a, CR 603.7c, and CR 400.7; no runtime behavior changed. The unsupported delayed-sweep handling remains intentionally honest.