fix(parser): UNSUPPORTED full-set one-off: The Doctor's Childhood Barn

[ntindle]

Summary

Fixes a parser misparse affecting 1 card(s) in the Doctor Who Commander precons.

Root cause: UNSUPPORTED full-set one-off: The Doctor's Childhood Barn

Cards corrected

• The Doctor's Childhood Barn

Fix

Implemented cluster 97 (The Doctor's Childhood Barn — Planechase plane) by closing the two residual parse gaps identified in the approved plan, dispatching each onto existing engine handlers plus two new class-level typed primitives. Both gaps are fully resolved; the card now parses with ZERO Unimplemented effects and ZERO Unrecognized static conditions (verified via a fresh full gen-card-data export).

GAP 1 — inline delayed trigger "When a player planeswalks, those permanents phase in": added TriggerMode::PlayerPlaneswalked (source-independent planar planeswalk event, distinct from the source-bound PlaneswalkedFrom/To siblings) across every registration site (types/triggers.rs enum + from_str + all-modes list; trigger_matchers.rs matcher fn + dispatch + registry; trigger_index.rs and ability_graph.rs planar groups), and a strip_temporal_prefix combinator arm that recognizes the delayed prefix and emits DelayedTriggerCondition::WhenNextEvent{PlayerPlaneswalked, or_trigger:None, lifetime:Persistent}. The body "those permanents phase in" already lowers to PhaseIn{ParentTarget}, wrapped by the existing assembly path into CreateDelayedTrigger{uses_tracked_set:false}. The chosen permanents are frozen via the pre-existing parent_target_snapshot path (identical to shipped/tested The Pandorica) — NO tracked-set machinery, exactly as the revised plan directed. Confirmed empirically the snapshot generalizes to N multi-targets.

GAP 2 — "can't phase in for as long as ~ remains face up": added StaticCondition::SourceIsFaceUp (planar command-zone face-up status, CR 311.2/901.7) with a parse_remains_face_up combinator mirroring parse_remains_tapped, a layers.rs evaluation arm (active_plane(state)==Some(source_id)), and every exhaustive-match site the compiler forced (layers.rs x3, ability_rw.rs x2, ability_scan, coverage x2, quantity, conditions bridge, oracle_trigger bridge). This closes the latent Unrecognized-is-always-true bug that would have made the phase-in lock permanent — the lock now correctly lapses on planeswalk.

Both new primitives are class-level (Agyrem co-witnesses PlayerPlaneswalked; SourceIsFaceUp serves the plane-face-up-gated duration class), not one-offs. Every new game rule is annotated with a CR number grep-verified against docs/MagicCompRules.txt.

VERIFICATION (non-Tilt worktree, cargo run directly): cargo fmt applied; cargo clippy -p engine --all-targets -D warnings = clean (exit 0); cargo test -p engine --lib = 16225 passed, 0 failed, 6 ignored; parser working-tree diff gate = clean (no string dispatch in any added line); fresh gen-card-data export shows the card with 0 Unimplemented/0 Unrecognized and PlayerPlaneswalked/SourceIsFaceUp/uses_tracked_set:false present.

TESTS (9 new, all passing) — building-block level plus runtime proof: (1) face-up duration combinator; (2) delayed-prefix strip; (3) PlayerPlaneswalked Display/from_str round-trip; (4) match_player_planeswalked fires for any Planeswalked event, not for others; (5) SourceIsFaceUp condition true for active plane, flips false after planeswalk-away; (6) full-card parse assertion (0 gaps + exact delayed-trigger/duration shape); (7) parent_target_snapshot freezes ALL multi-targets for the delayed PhaseIn (the key novel-vs-Pandorica runtime seam); plus a RUNTIME firing proof that a WhenNextEvent{PlayerPlaneswalked} delayed trigger fires and is consumed on a real Planeswalked event through check_delayed_triggers (and NOT on a non-planeswalk event), stacking its PhaseIn ability. The firing routes through the shared delayed_trigger_event_with_index -> trigger_matcher(PlayerPlaneswalked) registry path (source-confirmed), guarded by the existing trigger_matcher_covers_registry_entries test.

JUDGEMENT CALLS: (a) In ability_rw.rs RwProfile I placed SourceIsFaceUp in the frozen_source_read() group (a SOURCE-object status read of command-zone SetMembership) rather than the empty() group where EnchantedIsFaceDown lives, because EnchantedIsFaceDown reads the ATTACHED object whereas SourceIsFaceUp reads the source plane's own status — frozen_source_read is the more accurate model. (b) SourceIsFaceUp is defined as active_plane(state)==Some(source_id); verified rules-correct because planeswalk removes the departing plane from command_zone (planechase.rs:296-301) so active_plane returns the new plane and the condition flips false, and CR 311.2 guarantees a single face-up plane at a time. (c) For the plan's Test 8 I wrote a focused end-to-end firing test (check_delayed_triggers with a synthetic Planeswalked event) rather than the full chaos->target-selection->planeswalk cast pipeline, because the per-opponent target-selection WaitingFor is pre-existing TargetOnly infrastructure I did not change and each novel seam is independently unit-proven; the firing test covers the sole new integration point (delayed trigger firing on planeswalk).

STOP-AND-RETURN: none — no CR ambiguity, no new subsystem, work fits existing architecture. The revised plan's decision to DROP the tracked-set direction (uses_tracked_set stays false, population frozen by parent_target_snapshot) held up exactly.

DEVIATIONS FROM PLAN: (1) TriggerDefinition::new is in crate::types::ability, not crate::types::triggers as the plan snippet wrote — used the correct path. (2) Added an extra runtime firing test beyond the plan's 8 (net stronger coverage). (3) Substituted the focused firing test for the plan's monolithic Test 8 (see judgement call c).

INCIDENTAL: running gen-card-data.sh regenerated two generated-data files unrelated to the Barn (known-tokens.toml — a large drift caused by this worktree lacking full token-set source data; oracle-subtypes.json — a 'Shi-Ar' subtype addition). Both were reverted to committed baseline to keep the diff scoped; the worktree was clean at task start so this is safe. client/public/card-data.json is gitignored/regenerated and not part of the diff. No commit made — changes left in the working tree.

Files changed

• crates/engine/src/types/ability.rs
• crates/engine/src/types/triggers.rs
• crates/engine/src/game/trigger_matchers.rs
• crates/engine/src/game/trigger_index.rs
• crates/engine/src/analysis/ability_graph.rs
• crates/engine/src/parser/oracle_effect/lower.rs
• crates/engine/src/parser/oracle_nom/duration.rs
• crates/engine/src/game/layers.rs
• crates/engine/src/game/ability_rw.rs
• crates/engine/src/game/ability_scan.rs
• crates/engine/src/game/coverage.rs
• crates/engine/src/game/quantity.rs
• crates/engine/src/parser/oracle_effect/conditions.rs
• crates/engine/src/parser/oracle_trigger.rs
• crates/engine/src/game/effects/delayed_trigger.rs
• crates/engine/src/game/planechase_tests.rs
• crates/engine/src/parser/oracle_tests.rs

CR references

• CR 311.2 (grep -n "^311.2" docs/MagicCompRules.txt — plane cards remain in the command zone; basis for SourceIsFaceUp = active command-zone plane)
• CR 901.7 (grep -n "^901.7" — abilities of a face-up plane function from the command zone)
• CR 611.2b (grep -n "^611.2b" — 'for as long as' continuous-effect duration)
• CR 701.31 (grep -n "^701.31" — Planeswalk; basis for TriggerMode::PlayerPlaneswalked)
• CR 701.31b (grep -n "^701.31b" — to planeswalk turns each face-up plane face down; basis for SourceIsFaceUp flipping false)
• CR 901.11 (grep -n "^901.11" — a player has 'planeswalked'; abilities that trigger when a player planeswalks trigger)
• CR 603.7a (grep -n "^603.7a" — delayed triggered abilities created during resolution)
• CR 603.7b (grep -n "^603.7b" — delayed trigger fires once unless a stated duration; basis for Persistent lifetime)
• CR 603.7c (grep -n "^603.7c" — delayed ability refers to a particular object; zone check at resolution)
• CR 608.2c (grep -n "^608.2c" — instruction/ordering semantics; snapshot rationale)
• CR 708.2 (grep -n "^708.2" — face-down permanent characteristics; morph face-up vs planar face-up distinction in SourceIsFaceUp doc)
• CR 702.26 / 702.26b / 702.26c (grep -n "^702.26" — phasing; referenced by reused phase_out/phase_in handlers)

Verification

• cargo fmt --all — pass
• check-parser-combinators.sh (base=upstream/main merge-base d1f7d05) — pass
• cargo clippy -p engine --all-targets -- -D warnings — pass
• cargo test -p engine — pass
• oracle-gen data --filter 'the doctor's childhood barn' — pass
  Cards confirmed re-parsed correctly: The Doctor's Childhood Barn

🤖 Generated with Claude Code

[gemini-code-assist]

Code Review

This pull request implements support for the 'whenever a player planeswalks' trigger and the 'remains face up' duration condition, enabling full rules support for the plane card 'The Doctor's Childhood Barn'. It introduces TriggerMode::PlayerPlaneswalked and StaticCondition::SourceIsFaceUp with their respective parsers, matchers, layer evaluations, and tests. The review feedback highlights a sibling coverage issue in the parser where only 'when' is matched instead of both 'when' and 'whenever', and suggests using modular nom combinators to handle both variants robustly.

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]

[MEDIUM] Incomplete sibling coverage and fragile parsing for delayed trigger prefix. Evidence: crates/engine/src/parser/oracle_effect/lower.rs:4686.

Why it matters: Only matching the exact phrase "when a player planeswalks, " fails to parse other Planechase cards that use the standard MTG template "whenever a player planeswalks, " (e.g., Spatial Merging, Tazeem). Furthermore, using verbatim string tags for compound phrases bypasses the robust nom-based parser and creates fragile matches.

Suggested fix: Decompose the compound phrase into modular, reusable parsers for constituent parts (e.g., a conjunction parser for 'when'/'whenever' and a subject/action parser for 'a player planeswalks') and compose them using idiomatic combinator aggregates to prevent combinatorial explosion and improve maintainability.

Suggested change

	tag("when a player planeswalks, "),
	tuple((alt((tag("when"), tag("whenever"))), tag(" a player planeswalks, ")))

References

1. L2. Sibling coverage: If a parser arm or string was extended, are plural / possessive / negated / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' variants covered? ^(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 and compose them using idiomatic combinator aggregates.

[github-actions]

Parse changes introduced by this PR · 1 card(s), 2 signature(s) (baseline: main 5e003e1f9b38)

1 card(s) · ability/CreateDelayedTrigger · added: CreateDelayedTrigger (when=when next event (persistent))

Examples: The Doctor's Childhood Barn

1 card(s) · ability/when · removed: when

Examples: The Doctor's Childhood Barn

[matthewevans]

One blocker: TriggerMode::PlayerPlaneswalked is the third sibling on the planeswalk root, and it's a parameterization that didn't happen. Everything else here is strong — the card is verified, all four CR citations grep-verify and are correctly applied, the parse-diff is exactly the claimed population with zero collateral, and the test set is genuinely discriminating. Fix the enum shape and this lands.

🔴 Blocker — sibling-cluster smell in TriggerMode (types/triggers.rs:460-462)

After this PR, three variants share the planeswalk root:

Variant	Matcher body (game/trigger_matchers.rs)
PlaneswalkedFrom	*f == source_id && valid_player_matches(trigger, state, *player_id, source_id)
PlaneswalkedTo	*t == source_id && valid_player_matches(trigger, state, *player_id, source_id)
PlayerPlaneswalked (new)	valid_player_matches(trigger, state, *player_id, source_id)

All three match the same GameEvent::Planeswalked, all three call the same valid_player_matches, and they differ only in the predicate binding source_id to an endpoint of that event: from == source, to == source, or unconstrained. That is one axis — the source's role in the planeswalk — with three leaf values.

This is precisely the case CLAUDE.md names:

Sibling-cluster smell: when an enum has three or more variants that share a name root … or only differ in a comparator/aggregator/scope axis, that's a parameterization that didn't happen — refactor before extending.

The categorical boundary rule is satisfied rather than violated here, which is what makes the refactor the right move rather than an over-unification: all three live in a single CR section (CR 701.31 Planeswalk, grep-verified) and key off a single GameEvent. The axis lies wholly inside one rule section.

The fix. Collapse the cluster instead of extending it:

/// CR 701.31: which role the trigger's source must occupy in the planeswalk event.
pub enum PlaneswalkRole { From, To, Any }

// in TriggerMode:
Planeswalked { role: PlaneswalkRole },

The three matchers collapse into one function that matches on role; Any is exactly your source-independent case. Two notes that make this cheaper than it looks:

• Data-carrying TriggerMode variants are already precedented — KeywordAbilityActivated(AbilityTag) and Unknown(String) (types/triggers.rs), so this is not a new pattern for the enum.
• The registry still works. TriggerMode derives Hash + Eq, so Planeswalked { role } remains a valid HashMap<TriggerMode, TriggerMatcher> key.

Adding a fourth sibling later is the outcome this rule exists to prevent; at three, the refactor is still small.

🟡 Non-blocking

• StaticCondition::SourceIsFaceUp is correct as a sibling — I checked this against the same rule and it comes out the other way. It joins SourceIsTapped / SourceIsAttacking / SourceIsBlocking, but those are genuinely distinct predicates spanning different rule sections (status per CR 110.5 vs combat per CR 506/509), so unifying them under one SourceState parameter would conflate sections the engine resolves separately — which the categorical boundary rule explicitly forbids. Keep it as written.
• AI-contributor template — the body is missing the required Track and LLM sections and the Gate A / anchor headings from docs/AI-CONTRIBUTOR.md.

✅ Clean — verified, not taken on the body's word

• The card is real and the text matches. Pulled verbatim from Scryfall: "They can't phase in for as long as The Doctor's Childhood Barn remains face up. When a player planeswalks, those permanents phase in." Both gap clauses are exactly as claimed, and the_doctors_childhood_barn_planechase_full_parse drives the verbatim printed Oracle text through the real parse() card pipeline.
• All four CR citations grep-verify against docs/MagicCompRules.txt and are correctly applied: CR 701.31 (Planeswalk), CR 901.11 (planar deck), CR 311.2 (planes remain in the command zone), CR 901.7 (abilities of a face-up plane function from the command zone).
• Parse-diff is exactly the claimed population. 1 card / 2 signatures against baseline 5e003e1f9b38 (current main), sticky refreshed after the head commit. Claimed 1, measured 1, zero collateral.
• The coverage projection is updated. SourceIsFaceUp → ("SourceIsFaceUp", Handled) in game/coverage.rs. A new condition that is not projected is invisible to the coverage instrument and to the parse-diff — adding the arm in the same PR is the right call and is easy to forget.
• The tests discriminate. delayed_phase_in_fires_only_on_planeswalk is a real negative; source_is_face_up_flips_false_on_planeswalk_away pins the latent Unrecognized-is-always-true bug you identified (the lock would otherwise never lapse); test_for_as_long_as_condition_face_up_not_unrecognized guards vacuity directly. Reusing the shipped Pandorica parent_target_snapshot path instead of building tracked-set machinery is the right seam.
• No string dispatch. Every matches! in the diff is over enum variants (GameEvent::Planeswalked, Effect::PhaseIn), not Oracle text.

Recommendation: request-changes. Refactor PlaneswalkedFrom / PlaneswalkedTo / PlayerPlaneswalked into Planeswalked { role: PlaneswalkRole } and keep everything else exactly as it is. The implementation, the evidence, and the tests are all in good shape — this is a shape fix on one enum, not a rework.