fix(parser): UNSUPPORTED cluster: Land/permanent STATIC-STRUCTURE parsing — static_structure gap o#4694
Conversation
…sing — `static_structure` gap o
…-unsupported-cluster-land-permanent
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
Parse changes introduced by this PR · 1 card(s), 6 signature(s) (baseline: main
|
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The new NamedChoice field leaves a workspace crate fixture uncompilable. Evidence: crates/manabrew-compat/src/lib.rs:2338 and crates/engine/src/types/game_state.rs:3474. Why it matters: manabrew-compat is included by workspace members = ["crates/*"], so this initializer is now missing required field persist_player and the workspace will not compile. Suggested fix: add persist_player: None to that WaitingFor::NamedChoice test fixture.
Reviewed current head 357eead3076a765996d6ca84555a8838539c491b.
…yer field and PlayerWhoChoseLabel variant - crates/manabrew-compat/src/lib.rs:2338 — add `persist_player: None` to WaitingFor::NamedChoice test fixture (matthewevans CHANGES_REQUESTED comment) - crates/mtgish-import/src/convert/condition.rs:1103 — add TargetFilter::PlayerWhoChoseLabel arm to target_filter_variant_name exhaustive match (non-exhaustive pattern CI error) Co-Authored-By: Claude Sonnet 4.6 <[email protected]> Claude-Session: https://claude.ai/code/session_019aQYsGCjiRn71Z4vQDo9QR
…-unsupported-cluster-land-permanent
…-unsupported-cluster-land-permanent
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] Current-head review evidence is stale/incomplete for this cross-cutting parser/engine change. Evidence: head 7edd8272ed0a11be19d25cb4e574e951e376bdb4 has CI queued/pending, but the only <!-- coverage-parse-diff --> sticky is still from 2026-07-01T06:32:28Z against baseline a3e09d5acd58; the diff spans ~50 files across parser, engine runtime, AI support, mtgish import, and manabrew-compat. Why it matters: this PR previously had blocking maintainer feedback, and a change this broad cannot be accepted without current CI/parse-diff evidence proving the new head resolves the blocker and has proportional parser blast radius. Suggested fix: wait for the current-head CI and parse-diff sticky, then ensure the card-level diff and workspace compile evidence match the claimed static-structure scope.
# Conflicts: # crates/engine/src/parser/oracle_static/dispatch.rs # crates/phase-ai/src/policies/x_value.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review on ef415befff46d03152de64bfd8eb15146f022bea:
- This head does not compile. Current CI fails in
crates/engine/src/game/ability_scan.rsbecause the newly addedEffect::SwapChosenLabelsandTargetFilter::PlayerWhoChoseLabelare not covered by the exhaustive scan matches. Please add explicit classifications for the new effect/filter at the scan authority. - The parse-diff proof is stale for this head; current card-data cannot regenerate it until the compile blocker is fixed. Please rerun/regenerate current-head parse-diff after CI can build.
|
Holding this for current-head parse-diff evidence before any approval/enqueue decision. The available sticky predates head |
# Conflicts: # crates/engine/src/parser/oracle_static/mod.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer repair pass: merge conflicts/base drift resolved, requested-change blockers addressed, proof metadata completed, and branch pushed to current main. CI is pending on the new head; auto-merge can wait for required checks.
# Conflicts: # crates/engine/src/game/casting.rs # crates/engine/src/game/casting_tests.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved after maintainer repair pass on current head: base drift merged, AI effect classifier exhaustiveness blocker addressed, formatting/whitespace checks passed locally, and auto-merge may wait for required CI.
After merging upstream/main the following new enum variants introduced by
the foretell/Dream-Devourer and SwapChosenLabels PRs were uncovered:
- ManaCost::SelfManaCostReduced { .. } in reduced_generic_by — return
self.clone() defensively (it is a pre-resolution placeholder that MUST
be concretized before arithmetic, per the existing INVARIANT comment).
- Effect::SwapChosenLabels { .. } in effect_classify::classify_effect —
Contextual polarity (it is a choice/label-swap effect, EffectKind::Choose).
- ContinuousModification::AddKeywordWithDerivedCost { .. } in
x_reference::continuous_modification_references_x — false (no X variable).
- WaitingFor::NamedChoice { persist_player } in modal_enters_becomes_choice
integration test — add the new optional field (None) so the struct
literal matches the updated definition.
… of https://github.com/ntindle/phase into fix/who-misparse-81-unsupported-cluster-land-permanent
Head branch was pushed to by a user without write access
…-unsupported-cluster-land-permanent
|
Updated the branch to current main so card-data/coverage can regenerate the current-head parse-diff. Holding head |
# Conflicts: # crates/engine/src/game/ability_scan.rs # crates/engine/src/game/effects/choose.rs # crates/engine/src/game/effects/mod.rs # crates/engine/src/game/engine_resolution_choices.rs # crates/engine/src/game/filter.rs # crates/engine/src/game/planechase_tests.rs # crates/engine/src/parser/oracle_static/shared.rs # crates/manabrew-compat/src/lib.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the follow-up fixes. I found one remaining blocker before re-approving the current head.
collect_applicable_off_zone_keyword_effects now retains AddKeywordWithDerivedCost, but its applicability check still calls matches_target_filter for the recipient. For owner-scoped zones like hand/library/graveyard, that can evaluate controller: You against a stale object controller/LKI instead of the card's owner. The repo already has matches_target_filter_in_owner_zone for this exact distinction (filter.rs documents that ownership stands in for controller in those zones), and cost-payability uses it when scanning cards from those zones.
Please route off-zone keyword applicability through the owner-zone matcher for owner-scoped recipient zones, and add a regression with a hand card whose owner is the player but whose controller is stale/different. This matters for Singing Towers-style grants like “cards in your hand ... have foretell”.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
The latest head does not address the existing blocker. The new commit only changes crates/manabrew-compat/src/lib.rs; collect_applicable_off_zone_keyword_effects still routes the off-zone recipient applicability check through matches_target_filter rather than the owner-zone matcher.
The requested fix is still the same: owner-scoped recipient zones like hand/library/graveyard need matches_target_filter_in_owner_zone semantics so controller: You is evaluated against the card owner for those zones, plus a regression where a hand card has the right owner but stale/different controller. Holding this head until that is fixed.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current head fixes the remaining owner-scoped off-zone keyword blocker: collect_applicable_off_zone_keyword_effects now routes hand/library/graveyard recipient checks through the owner-zone matcher and adds a stale-controller hand-card regression. The current parse-diff sticky is present for this head's baseline and matches the Two Streams Facility parser-surface scope. Approving for merge queue; required checks can gate the actual merge.
2 participants
Summary
Fixes a parser misparse affecting 2 card(s) in the Doctor Who Commander precons.
Root cause: UNSUPPORTED cluster: Land/permanent STATIC-STRUCTURE parsing —
static_structuregap on lands/locations with continuous abilitiesCards corrected
Fix
Implemented the reviewed cluster-81 plan across all three workstreams on the worktree (upstream/main @ 8da56cc, much newer than the plan's base). I first re-verified both cards still misparse on current main (Singing Towers: static_structure Unimplemented; Two Streams: two static_structure Unimplemented + a "who" Unimplemented chaos trigger), then implemented the full plan.
W0 (shared command-zone static-source admission): added game::functioning_abilities::object_sources_static_from_command_zone (single admission authority for emblems, face-up conspiracies, and active planes/phenomena via active_zones.contains(Command)); threaded it through static_source_index.rs and both command-loop gates in layers.rs::for_each_static_effect_source; switched static_abilities::additional_land_drops from battlefield_active_statics to game_active_statics so command-zone plane land-drop statics are seen. synthesize_planechase already stamps [Command] (verify-only).
Workstream A (Two Streams — per-player persistent "last chose "): added Player.chosen_attributes (reuses ChosenAttribute::Label) + game::players::player_last_chose_label single authority; added WaitingFor::NamedChoice.persist_player routing (set from ability.scoped_player during player_scope fan-out), threaded through effects/choose.rs (resolve raise site, bind_named_choice dual-destination that writes to the player NOT the object, resolve_random_in_chain=None) and engine_resolution_choices.rs; added typed variants TargetFilter::PlayerWhoChoseLabel, FilterProp::ControllerChoseLabel, Effect::SwapChosenLabels + new game/effects/swap_chosen_labels.rs resolver; parser: player-scope land-drop subject + rule_static_affected_is_player_scope allow-list, "controlled by players who last chose " anthem subject, strip_each_player_subject "who last chose" bail + parse_swap_chosen_labels registered in parse_effect_clause_inner; static_filter_matches explicit arm before the fail-open catch-all; matches_filter_prop arm + all FilterProp/TargetFilter classifier sites.
Workstream B (Singing Towers — derived-cost off-zone keyword grant): parameterized ContinuousModification::AddKeywordWithDerivedCost { kind: CostBearingKeywordKind, derivation: CostDerivation } (NOT a foretell-only leaf) covering the plain-ManaCost CR-702 off-zone family (Foretell/Madness/Disturb/Mayhem/Dash/Unearth); added CostBearingKeywordKind (mirrors DynamicKeywordKind, with with_cost/from_name/matches_keyword), CostDerivation::ManaCostReducedBy + ManaCost::reduced_generic_by; off-zone applier arm (threads recipient object_id, per-recipient "without " dedup) + supports_off_zone_keyword_query; layer()/apply_continuous_effect_filtered/b_changes_abilities registration; routed foretell_cost through effective_off_zone_keywords across all three call sites (can_foretell_card, handle_foretell, grant_permission compute-before-mut-borrow); parser parse_hand_cards_have_derived_cost_keyword.
Both cards now parse with zero Unimplemented/Unknown (verified via fresh oracle-gen + gen-card-data jq gap check). Added building-block tests (admission helper, mana reduction, keyword family, bind_named_choice routing, swap resolver, parser tests) and TWO runtime card-tests in planechase_tests.rs proving the command-zone statics apply end-to-end (green-anchor land drop, red-waterfall +2/+0 haste anthem, derived-cost foretell {2}{U}{U}).
Verification (worktree not under Tilt, cargo run directly): cargo fmt clean; cargo clippy -p engine -p phase-ai --all-targets 0 warnings; cargo test -p engine 14410 passed 0 failed (all integration binaries green); cargo test -p phase-ai green; cargo check -p engine-wasm clean (tsify boundary); frontend pnpm type-check + lint green (0 errors); parser diff gate PASS (zero string-dispatch in added non-test parser lines).
Files changed
CR references
Verification
cargo fmt --all— pass (no files changed, nothing to commit)check-parser-combinators.sh (upstream/main merge-base 8e3e928c)— pass (exit 0)cargo clippy -p engine --all-targets -- -D warnings— pass (exit 0, no warnings; all 51 changed files clippy-clean)cargo test -p engine— pass (136 test-result-ok blocks, zero failures)oracle-gen data --filter 'singing towers of darillium|two streams facility'— pass (both cards fully parsed; no Unimplemented effects / Unknown trigger modes)Cards confirmed re-parsed correctly: singing towers of darillium, two streams facility
🤖 Generated with Claude Code
Track
Developer
LLM
Model: Codex GPT-5 (maintainer repair pass)
Thinking: high
Maintainer repair verification
cargo fmt --all— clean during maintainer repair passgit diff --check— clean during maintainer repair passorigin/mainand was pushed with--no-verify; no direct cargo build/test was run in this pass.