Fix Shadowborn Demon - #6
Closed
keloide wants to merge 26 commits into
Closed
Conversation
…copies (phase-rs#6276) * feat(engine,client): badge permanents that a copy effect turned into copies A Phantasmal Image that entered as a copy of Reveillark rendered identically to the real Reveillark — same art, same name, same P/T — so the board gave no way to tell them apart (phase-rs#5932). The board already had a "Copy" badge, but it was gated on a TOKEN-copy heuristic: obj.is_token === true && obj.display_source !== "Token" && !obj.face_down A real card under a copy effect is not a token, so it never qualified. No serialized signal existed for the frontend to use instead. `is_copy` means "not represented by a card" (CR 707.10) and is deliberately cleared when a copy resolves onto the battlefield (`game/stack.rs`, `game/sba.rs`), so it is correctly false here. The copy itself lives in a Layer 1a `CopyValues` continuous effect (CR 613.2a + CR 707.2), not on the object, and the copy overrides `printed_ref` so even image lookup follows the copied card. So the engine now classifies it: `DerivedViews::copied_permanents` lists battlefield permanents whose copiable values a live copy effect supplies, matched through the same `matches_target_filter` the layer engine uses to pick an effect's recipients — the projection and the effect that actually rewrote the object can never disagree. The client unions it with the existing token-copy case, so the badge now covers the whole class: Clone, Vesuvan Doppelganger, and every "enters as a copy" permanent, not just token copies. CR 708.2: face-down permanents are excluded on BOTH sides, since their characteristics are only those the face-down rules grant and surfacing "Copy" would leak what one really is. Neither side alone can leak it. Tests: engine coverage for the projection (flagged under a live effect, empty on an ordinary board, never for a face-down permanent) and client coverage for the badge, including the face-down guard. The positive client test is discriminating — restoring the old token-only condition fails it while the two guards stay green. Closes phase-rs#5932 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(engine): gate the copy badge on the layer engine's liveness predicate Review blocker: the projection treated a STORED copy effect as a live one. A lapsed effect stays in `transient_continuous_effects` until it is swept, so Zygon Infiltrator's "for as long as that creature remains tapped" copy ended the moment its target untapped while the permanent kept displaying the badge. `gather_transient_continuous_effects` already knew the answer — it rejects a departed `UntilHostLeavesPlay` host, a `ForAsLongAs` duration that no longer holds, and a false source condition. Those three gates are now extracted into `transient_effect_is_live`, which both the layer engine and `derive_views` call, so a display projection cannot claim an effect is live after the layer engine has stopped applying it. Extracted rather than duplicated: a second copy of the predicate would be free to drift, which is exactly the class of bug this review caught. `gather_transient_continuous_effects` keeps its existing behaviour — the condition gate now runs inside the shared predicate, and the retained recipient-context condition is unchanged. Also from review: * `copied_permanents` is sorted, matching what its doc comment already claimed. * The client's duplicated `!face_down` check is factored out so the CR 708.2 guard leads and covers both copy sources. Tests: a derived-view regression builds Zygon's target-relative `ForAsLongAs` copy, asserts the badge shows while the target is tapped, then untaps it and asserts the badge is gone while the effect is still stored — so it exercises liveness rather than removal. It fails without the gate. Verified: 17442 lib tests, 3676 integration tests, clippy -D warnings, all clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(PR-6276): exclude merge layer effects from copy badges A merge representation uses CopyValues for its top component but is not a copy effect. Exclude only the tracked merge layer effect and cover the derived view. Co-authored-by: minion1227 <romantymkiv1999@gmail.com> * test(PR-6276): preserve copy badges after merge representation Prove the merge-layer exclusion is scoped to its tracked effect ID, so an independent copy effect on the merged host remains visible. Co-authored-by: minion1227 <romantymkiv1999@gmail.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…hot vintage-proof (phase-rs#6274) * fix(engine): commit W30 MTGJSON token catalog and make coverage snapshot vintage-proof The weekly MTGJSON refresh introduced by phase-rs#6237 (vintage gate 2026-07-20) unfroze a stale token catalog cache. Deterministic regen (byte-identical across two checkouts) against the 2026-07-20 MTGJSON data adds 13 tokens (12 Secret Lair Drop printings: Food x8, Treasure, Ooze, Elf Warrior x2; 1 HOB Goblin Army) and 1184 [[token.source_card_refs]] (8637 -> 9821), and picks up MTGJSON's face-name change 'Undercity // The Initiative' -> 'Undercity'. No tokens removed; coverage stays complete (2858/2858 supported, 1490/1490 rules-text tokens parsed). analyze_token_coverage_treats_source_defined_pt_as_represented pinned vintage-dependent absolutes (2845/1480), which go red every week upstream adds tokens even at 100% coverage. Replace them with invariants (supported == total, parsed == rules_text) plus non-vacuity floors at the last-known-good baseline so an empty or truncated catalog cannot pass vacuously. Assisted-by: ClaudeCode:claude-fable-5 * test(engine): ratchet token-catalog floors to the committed vintage Review feedback (phase-rs#6274): the non-vacuity floors shipped at 2845/1480 sat exactly at the PRE-refresh catalog, so reverting this PR's regen still passed every assertion — the guard could not detect losing the refresh it was added alongside. The pre-PR test asserted the parent counts as equalities (assert_eq!(total_tokens, 2845)) and was green on main, so the parent catalog measures exactly those floors. Ratchet all floors to the committed vintage (2858/1490) and add the third axis already carried on the summary, source_card_refs >= 9821. `>=` still keeps weekly upstream ADDITIONS green — the false-red phase-rs#6237 introduced — while any shrink now fails. The ref floor is the load-bearing one and is deliberately tight: across the ten recorded revisions of known-tokens.toml, tokens and rules_text are monotone, and source_card_refs shrank exactly once — phase-rs#6199, a parser PR that silently dropped 1173 token<->card links (9810 -> 8637) while the token count GREW past a count-based floor. This regen is what repaired it. Each floor is probed in isolation, since sequential assert!s let an earlier conjunct dominate a later one and render it vacuous: - catalog reverted to f0ac543^ -> FAIL "token catalog shrank: 2845 presets < 2858" - 1 rules_text line removed -> FAIL rules_text_tokens >= 1490 - 5 source_card_refs blocks removed -> FAIL source_card_refs >= 9821 Catalog restored byte-identically after each probe. Floors carry the measured count in the panic so a CI red names the axis and the delta instead of requiring a local repro. Assisted-by: ClaudeCode:claude-opus-4.8 * docs(engine): document the provenance seam for the token-catalog floors The floors observe totals; provenance (committed catalog == tokens-gen output for the declared MTGJSON vintage) is structurally untestable at test time (the ~560 MB gitignored generator input is absent; the test sees build.rs's embed of the tracked file) and is verified at the generation seam instead (gen-card-data.sh temp+cmp+vintage gate). Comment documents the seam and the by-hand reproduction recipe, per review 4746023659. Assisted-by: ClaudeCode:claude-opus-4.8
…ring mana payment (phase-rs#5963) (phase-rs#6272) * fix(engine): fire sacrifice/death triggers for mana abilities paid during mana payment (phase-rs#5963) A mana ability with a non-tap cost (e.g. Gilded Goose's "{T}, Sacrifice a Food: Add {C}") activated during mana payment for a spell silently dropped every observer of its cost. Sacrificing a creature-that-is-a-Food never fired Scavenger's Talent's "creatures you control die" (L1) or "you sacrifice a permanent" (L2) triggers. The PayCost -> CostResume::ManaAbility arm never scanned its cost events for triggers. The post-action trigger pipeline only runs for Priority resumes (guarded by waiting_for == Priority), so a ManaPayment/UnlessPayment resume lost the events. This mirrors the inline scans already done by the ChooseManaColor and mid-payment ActivateAbility arms: snapshot events before the cost handler, process_triggers on the new events, surface any OrderTriggers prompt, and claim the scan for Priority resumes so the pipeline does not double-fire. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): move mana-ability cost trigger scan to the settlement authority (phase-rs#5963) The initial fix scanned mana-ability cost events in engine.rs's PayCost -> CostResume::ManaAbility arm. That was the wrong layer and double-fired: finish_mana_ability_cost_payment is the single settlement authority for mana-ability cost events and already scans them for the ChooseManaColor path, so an extra scan in the reducer double-counted Kilo's becomes-tapped proliferate trigger under a standalone Relic activation (Pentad grew 3->5 instead of 3->4, breaking kilo_accept_marks_pentad_charge_as_unbounded_display_target). Revert the engine.rs scan and fix the real gap inside finish_mana_ability_cost_payment: a direct (non-paused) mana ability whose root resume is not Priority (ManaPayment/UnlessPayment, i.e. activated during mana payment) never reaches the post-action trigger pipeline, so its already-paid sacrifice/discard/exile cost events were dropped. Scan them at the settlement authority for non-Priority resumes only; Priority resumes stay with the pipeline so nothing double-fires. Co-authored-by: Cursor <cursoragent@cursor.com> * fix * style(PR-6272): hoist mana payment test imports Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> --------- Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…#6278) The engine embeds three git-tracked, generated data files at compile time (known-tokens.toml, oracle-subtypes.json, mtgjson-vintage) that were regenerated and committed by hand. When MTGJSON publishes new tokens/subtypes weekly they drift, which periodically turned CI red and required a manual catalog PR (e.g. phase-rs#6274). Add a scheduled workflow that force-refreshes MTGJSON, runs the single source-of-truth generator (scripts/gen-card-data.sh), and opens an auto-merge PR only when the tracked catalogs change. Mirrors refresh-feeds.yml. The vintage write-gate makes it self-correcting (no newer input -> empty diff -> no PR), so it is safe on all three triggers: weekly schedule, manual dispatch, and after the Clear Caches workflow (which only deletes caches; this re-fetches, re-processes, and commits the result). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…dexing (Plan 04) (phase-rs#6269) * feat(engine): migrate ability continuation to resolution stack * feat(engine): migrate repeat-for to resolution stack * feat(engine): migrate repeat-until to resolution stack * feat(engine): migrate choose-one-of to resolution stack * feat(engine): migrate vote-ballot to resolution stack * feat(engine): migrate per-player zone choice to resolution stack * feat(engine): migrate per-category zone choice to resolution stack * fix(engine): retain per-player continuation below prompt * fix(engine): preserve nested resolution frame order * fix(engine): migrate resolution stack consumers * fix(engine): keep resolution frame authority documented * refactor(engine): migrate ChangeZone resolution frame * refactor(engine): migrate BatchDelivery resolution frame * refactor(engine): migrate CounterMoves resolution frame * refactor(engine): migrate CounterRemovals resolution frame * refactor(engine): migrate CounterAdditions resolution frame * refactor(engine): migrate CopyToken resolution frame * refactor(engine): migrate EachPlayerCopyChosen resolution frame * fix(engine): parent continuations beneath migrated frames * fix(engine): preserve nested tranche two frame order * fix(engine): retain devour owner at nested boundary * fix(engine): retain nested change zone owners * refactor(engine): migrate optional effect resolution frame * refactor(engine): migrate repeated optional payment frame * refactor(engine): migrate coin flip frame * refactor(engine): migrate proliferate frame * refactor(engine): migrate mutate merge frame * fix(phase-ai): read optional effect resolution frame * fix(engine): retain direct choice frame ownership * fix(engine): reject buried direct choice wire owners * fix(engine): preserve legacy parent wire ordering * refactor(engine): move draw replacement tails into frames * refactor(engine): move connive reentry into frames * refactor(engine): move life assignment into frames * refactor(engine): move spell resolution into frames * fix(engine): preserve optional frame takes * fix(engine): preserve replacement frame nesting * fix(engine): preserve post-replacement frame parents * fix(engine): resume paired draw continuations * fix(engine): retire paused replacement prompt frames * fix(engine): retain replacement context through promoted tails * test(engine): cover paired replacement elimination * refactor(engine): centralize resolution settlement checks * fix(server): hide resolution frames from player views * fix(engine): clear token seed after frame drain * docs(engine): clarify token seed drain authority * test(engine): cover resolution wire allocator migration * test(engine): pin resolution wire compatibility contract * ci: guard resolution-frame boundaries * fix(engine): bind inline mana overrides to trigger identity * perf(engine): index inline mana triggers * fix(ship): restore main's protocol 20 dropped by stale freeze rule The merge resolution pinned the application protocol at 19 per the Plan-04 charter's frozen-constants rule, but that rule predates main's shipped bump to 20 (actor-scoped priority passing, phase-rs#6213). Restore main's constants and version tests; keep the Phase-4 changelog note that the resolution-wire pinning added no protocol change of its own, and retarget the protocol ratchet script to 20. * fix(ci): drop ripgrep dependency from resolution-frame boundary guard The guard's embedded Python shelled out to 'rg -l' to list engine files containing legacy wire keys; ripgrep is not installed on CI runners, so the lint job died with FileNotFoundError after every lint step had passed. Scan with a pure-Python rglob over the same path and pattern instead. Red/green re-verified: clean tree passes, an injected legacy key in production code fails with exact path:line. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ld conditions (Shadowborn Demon)
The existential threshold combinator parse_there_are_conditions had a
suffix-only comparator axis ("N or more" -> GE, bare -> EQ) and no prefix
axis, so "there are fewer than N <countable> <scope>" failed to parse:
- Shadowborn Demon's intervening-if (CR 603.4) was silently dropped and
the upkeep sacrifice resolved unconditionally while the card stayed
coverage-supported (misparse backlog category 2).
- The Warring Triad's "as long as" gate fell to
StaticCondition::Unrecognized (always-true), so the Layer-4 type
removal (CR 611.3a + CR 613.1d) applied unconditionally.
Add parse_strict_comparator_prefix ("fewer than " -> LT, "more than "
-> GT; CR 107.1a) as the single authority for the comparator-prefix
family, shared with parse_put_onto_battlefield_this_way (which
previously inlined the same alt). parse_there_are_conditions now
composes opt(prefix) with the existing "or more" suffix through one
exhaustive (prefix, suffix) match that rejects the ungrammatical
double-marked form and dedupes the previously duplicated GE/EQ
computation. No runtime, type, or bridge changes: Comparator::evaluate
is already generic at both consumer seams.
Tests: 6 parser unit tests across the comparator range incl. hostile
negatives; a trigger-parse test asserting the hoisted intervening-if;
exact-typed strengthening of the Warring Triad static test; 4 runtime
integration tests (trigger fires at 5 with opponent-graveyard noise,
boundary-exact quiet at 6; static gate active at 0, inactive at 8).
Coverage: 31501 -> 31502 (The Warring Triad now supported); Shadowborn
Demon's trigger carries the condition; semantic-audit clean for both.
Both cards removed from docs/parser-misparse-backlog.md.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3jSXUUoGHH2Qh289AMKa9
…tency (phase-rs#6286) The coverage snapshot's ratchet floors (phase-rs#6274) observe three totals, so they catch a catalog that shrank but not one that was hand-edited, spliced, or partially rewritten while keeping its counts. Add the two structural properties of tokens-gen output that ARE checkable at test time: - ids strictly ascending, matching tokens_gen.rs:106's `presets.sort_by(|a, b| a.id.cmp(&b.id))` - `token_image_ref.preset_id == id` wherever a ref exists, since the generator builds both from the same MTGJSON uuid Both are per-entry properties with no pinned totals, so a weekly refresh that only adds tokens stays green — the phase-rs#6237 false-red phase-rs#6274 removed does not come back. Neither establishes provenance (that needs the ~560 MB gitignored generator input, absent at test time); that limit is stated in-code. Consistency is asserted only where an image ref exists: a token with no Scryfall image is legal upstream data, and requiring one would re-introduce a weekly false-red. Scope is stated precisely in-code because it is narrow: neither assertion catches a body-only rewrite (nothing ties body/fidelity/source_card_refs to an identity) nor a deleted entry (removing from the middle keeps the sequence ascending) — deletion is the count floors' job. Also corrects known_token_presets()'s doc comment, which claimed presets are "sorted by category then id for stable display order". Both halves were wrong: tokens_gen.rs:106 sorts by id alone, and nothing displays in engine order — DebugCreateActions.tsx regroups by category and re-sorts by power/toughness/ name. The real reason for id order is a minimal diff on regen. Assisted-by: ClaudeCode:claude-opus-4.8
* feat(server): channel-aware startup data bootstrap and shell lifecycle flags Add configurable bind addressing for loopback shell launches. Add stdin-close lifecycle shutdown for orphan prevention. Add optional exact-Origin WebSocket handshake filtering. Add signed release/preview data bootstrap with verified downloads and dev-only fixture gating. * fix(server): best-effort draft-pools bootstrap, HTTP timeouts, https-only manifests * ci: slim signed server artifacts, content-addressed data, and continuous preview server pipeline Publish content-addressed draft pools in deploy and release data pipelines. Compute and export the deploy engine fingerprint, stamp the preview frontend environment, and dispatch the continuous preview server artifact workflow. Add three-platform preview server builds with Ubuntu minisign signing, signed manifest-last publication, and current-plus-previous R2 retention. Publish signed release data manifests and slim binary-only server assets while retiring bundled data archives. Remove server-data artifact plumbing and Docker data baking so server data bootstraps at runtime. Sequencing: merge together with, or after, the server bootstrap commit; slim artifacts without bootstrap support would strand fresh self-hosters. * fix(ci): globally serialize preview manifest publication * fix(ci): dispatch preview-server.yml from main, not a bare SHA workflow_dispatch refs must be a branch or tag — a 40-hex SHA fails with 422, so every deploy's preview-server dispatch would have silently missed (the failure is deliberately non-fatal). The exact source tree is already pinned by the 'commit' input the workflow checks out. Same pattern as nightly-release.yml. Also documents the preview image's PHASE_CHANNEL identity gap (GH phase-rs#6287) where the old comment claimed runtime bootstrap. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* feat(desktop): native engine lifecycle — download, verify, spawn, adopt, GC (Workstream E) - Add typed remote ensure/stop commands and progress events.\n- Resolve release and preview artifacts with downgrade ratchets.\n- Verify signed manifests and server binaries with the pinned key.\n- Provision writable per-key data from a SHA-256 shared cache.\n- Manage adoption, stdin watchdog shutdown, single-instance, and manifest-diff GC.\n- Scope the commands to remote capabilities and refresh generated schemas. * fix(desktop): non-fatal native-engine GC, single-instance first, fast-fail health poll * test(desktop): de-flake migration temp dirs with a per-process sequence SystemTime nanos alone can collide across parallel test threads on coarse clocks, making concurrent tests share a temp directory and race the channel preference file. * fix(desktop): re-verify PID before SIGKILL, basename process match on BSD, drop redundant Windows pre-delete Addresses the three Gemini review findings on phase-rs#6283: PID-reuse race between TERM and KILL, ps -o comm= reporting bare executable names on BSD-derived systems, and delete-before-rename defeating crash atomicity on Windows (std::fs::rename already replaces the destination there). * fix(desktop): never kill an unowned spawn record on app exit stop_native_engine's exit path fell back to killing whatever PID the shared on-disk spawn record named, even when this process spawned no engine — a record another live instance owns. Today the single-instance secondary hard-exits inside plugin init (verified in tauri-plugin-single-instance 2.4.3 on all three platforms) so the path is dormant, but killing a process this instance did not spawn must never be exit-path behavior. Orphans already self-terminate via --exit-on-stdin-close; stale records resolve at the next ensure_native_engine adopt-or-kill. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…lity gate (phase-rs#6288) The Full-mode server's action-legality gate does an exact containment check against the enumerated candidate set, but candidate enumeration emits every cast-family action with CastPaymentMode::Auto — so any cast submitted with the Manual payment preference could never match, and manual-mana players could tap lands but never cast anything. CR 601.2g: the payment mode selects whether the engine auto-pays or pauses for manual mana activation — a payment preference, not part of the cast's legality. GameAction::with_canonical_payment_mode() (engine- owned, Cow-borrowing for the common case) erases Manual before the membership test; the applied action keeps the submitted mode. Fixes phase-rs#6275 Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Parse changes introduced by this PR · 4 card(s), 5 signature(s) (baseline: main
|
…oors (phase-rs#6290) The refresh-card-data.yml test run regenerated known-tokens.toml from a clean, complete MTGJSON fetch (333/333 token sets, 0 failed) for vintage 2026-07-21, producing 8644 source_card_refs — vs 9821 in the hand-committed catalog (phase-rs#6274), which came from a developer's local data/mtgjson/sets/ dir carrying extra reprint set files beyond the reproducible fetch scope. Every token keeps its source-card names (103078 -> 103033 name-list lines, 0.04%) and >=1 printing; only redundant reprint scryfall_ids (e.g. akr/usg/j25 reprints) drop, which no consumer depends on. - Land the canonical 8644-ref catalog + vintage 2026-07-21. - Recalibrate the token_coverage floors from exact-pins (which rejected the correct clean regen) to catastrophic-loss backstops with headroom (total >= 2700, rules_text >= 1400, source_card_refs >= 8000). - Add a fetch-completeness gate to refresh-card-data.yml: refuse to open a catalog PR unless fetch-token-sets.sh reports 0 failed / 0 skipped, making provenance a build-time guarantee instead of relying on the engine floor to catch a partial regen after the fact. Supersedes the bot-opened phase-rs#6285 (stale base). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…phase-rs#5955) (phase-rs#6291) Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com>
…aphics bug at no performance cost (phase-rs#6293)
…to phase-server (phase-rs#6292) Two distinct causes broke the first executed deploy since phase-rs#6238/phase-rs#6282: 1. setup-rust-toolchain exports RUSTFLAGS='-D warnings' by default, and an env RUSTFLAGS overrides ALL .cargo/config.toml rustflags — silently dropping the [target.wasm32-unknown-unknown] 16 MiB shadow-stack link-arg phase-rs#6238 added, tripping build-wasm.sh's assert_wasm_stack guard. Pass rustflags: '' on every wasm-building job (deploy build-wasm, release wasm + broker-wasm) so config.toml stays authoritative. 2. phase-rs#6282 gave phase-server a rustls-only reqwest, but the CI builds run 'cargo build --bin phase-server' unscoped from the workspace root, so feature unification folds feed-scraper's native-tls reqwest features in, dragging openssl-sys into the musl cross-compile (no OpenSSL → build failure) and dynamic OpenSSL into the Docker image (runtime has no libssl). Scope every server build with -p phase-server: deploy, release (linux + matrix legs), Dockerfile compile stage, Tiltfile. Verified: cargo tree -p phase-server -i openssl-sys --target x86_64-unknown-linux-musl finds no path post-fix (present unscoped); rustflags input semantics confirmed against the action's action.yml. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
) The tauri resource still built phase-server and copied it into client/src-tauri/binaries/ as a sidecar — removed with the thin shell (no externalBin remains in tauri.conf.json), so the cmd compiled a full engine tree to feed a path nothing reads. It also redeclared engine/AI/ wasm/server sources as deps, restarting the whole dev loop on changes tauri dev's own Rust watcher already handles. Local shell testing is the devUrl flow: tauri dev starts vite and hosts the LOCAL frontend in the shell window (frontend's auto_init already yields :5173 when tauri is enabled). Tilt now only restarts the loop on Tauri config/manifest changes. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
) The Dispatch Existing Tag Recovery job runs gh workflow run without a checkout, so gh tried to infer the repository from a local git directory and failed with 'not a git repository' — the mode-D residual: every recovery dispatch for a stranded tag silently died (first hit: v0.32.0 after today's wasm-guard release failure). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…he free review pass (phase-rs#6298) Gemini Code Assist has been sunset, so the review tooling was leaning on a bot that no longer answers. Replace that dead dependency without adding per-PR token cost by promoting the free external reviewer for this public repo. - pr-contribution-handler: rewrite "Gemini review handling" -> provider-neutral "Independent review pass". Drop the @gemini-code-assist trigger and quota logic (summoning a dead bot burns a round-trip and posts noise). CodeRabbit is the free external pass; local /code-review is a risk-scaled fallback only. - review-impl: step 4 now assumes NO external bot pre-screened the PR by default, so its own lenses are the complete review, not a supplement to a backstop. Generalize the remaining Gemini-specific mentions (COMMENTED/reviewDecision behavior, severity-reconcile checklist) to any bot. - Add .coderabbit.yaml: chill profile, generated-artifact path_filters ported from .gemini/config.yaml, and 10 layered path_instructions distilled from CLAUDE.md, the .gemini styleguide, and the review-impl lenses (Rust idioms, CR annotations, engine-owns-logic, parser nom mandate, enum parameterization, frontend display-purity + i18n, AI classifiers, transport hidden-info, feed safety, test adequacy). - Remove now-orphaned .gemini/config.yaml and .gemini/styleguide.md; their review substance is preserved in .coderabbit.yaml. The pr_review.py gemini-case-finding-refuted signal alias is intentionally kept: it normalizes historical events already in the append-only log. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…sys feature unification (phase-rs#6299) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…-rs#5952) (phase-rs#6296) * fix(engine): don't force a MustAttack carrier to attack itself (phase-rs#5952) * fix --------- Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: andriypolanski <andriypolanski@users.noreply.github.com>
…se-rs#6300) * fix(parser): gate DamageDone triggers on combat-status recipient (Kusari-Gama) * fix(PR-6300): harden blocking-recipient review Correct the blocking-creature CR citations and add a parsed-trigger-to-runtime-matcher regression that rejects player damage while accepting damage to a blocking creature. Co-authored-by: andriy-polanski <andriy-polanski@users.noreply.github.com> * test(PR-6300): cover nonblocking damage recipient Co-authored-by: andriy-polanski <andriy-polanski@users.noreply.github.com> --------- Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: andriy-polanski <andriy-polanski@users.noreply.github.com>
…lass) (phase-rs#6302) Self-library peek-and-cast cards (Kiora Sovereign of the Deep, Aetherworks Marvel, Construct a Cosmic Cube, Perception Bobblehead, Svella Ice Shaper, Velomachus Lorehold) previously routed "cast ... from among them" through the LingeringPermission driver: the looked-at cards were exiled, the free cast was never offered, and the rest were never bottomed. - Parser: seed chain_prior_self_library_peek from the raw bare-private-peek Dig shape via a single shared effect_is_bare_private_peek predicate (also used by the assembly pure-peek lowering, so the two sites cannot drift); flip the "from among them" fall-through to CastFromZoneDriver:: DuringResolution when the chain has a self-library peek and no exile producer (CR 608.2g). - Parser: parse the "with mana value N or less/greater" dig-peek suffix into a typed cast-permission constraint (fixes Perception Bobblehead's dropped MV<=3 cap; Founding the Third Path stays constraint-free). - Resolver: park an EffectZoneChoice on the Library for eligible candidates, freeze dynamic constraint refs (X, power) to Fixed at binding time (CR 608.2h), cast the chosen spell during resolution, and bottom the rest in a random order on accept, decline, and zero-eligible paths (CR 401.4). - Resolver: gate the pre-injected-target library route on references_exiled_by_source() so Planetarium of Wan Shi Tong's ParentTarget flow keeps its direct free cast. - Visibility: the prompt player sees the looked-at Library cards; opponents get redacted placeholders across serde round-trips (CR 701.20e, CR 400.2). Fixes the Discord bug report (thread 1529132485668245575). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: keloide <keloide@users.noreply.github.com>
Stop the negative runtime regression at the first empty-stack upkeep state instead of passing into later phases and an unrelated deck-out. Correct the new integer-number annotations from CR 107.1a to the grep-verified CR 107.1. Co-authored-by: keloide <keloide@users.noreply.github.com>
keloide
pushed a commit
that referenced
this pull request
Aug 8, 2026
* fix(parser): compose the cast-type gate axes, and enforce it at runtime (Epic Experiment) Closes phase-rs#6960. `parse_cast_type_disjunction` handled exactly one shape: `" or "` between two bare core types, with an optional article and a required trailing "spell(s)"/ "card(s)". Everything else in the "cast from among them" class kept a bare `TargetFilter::ExiledBySource` with no card-type leg, so any card type could be cast from the exiled set. Rewritten as a per-axis composed grammar: opt(quantifier) opt(article) leg (sep leg)* head_noun reusing `oracle_nom::target::parse_type_filter_word` for the leg alphabet (core types AND subtypes) and mirroring `oracle_nom/enchant.rs` for the separator/list pair. `and`, `or`, `and/or` and serial commas all lower to `TypeFilter::AnyOf`: per CR 205.2b conjunction is a property of ADJACENT type words ("artifact creature"), while a connector enumerates alternatives. A literal `And` would be a total no-op — no card carries both Instant and Sorcery. Seven cards gain the gate they were missing: Epic Experiment, Ral Leyline Prodigy, Kylox "instant and/or sorcery spells" Collected Conjuring "up to two sorcery spells" Sanwell, Avenger Ace "a Vehicle or artifact creature spell" Wand of Wonder "up to X instant and/or sorcery spells" Scarlet Witch, Chaotic Avenger "a Hero or noncreature spell" Acceptance requires either two or more legs or a consumed quantifier, which is also the anti-swallow guard: "cast a spell from among them" (Aetherworks Marvel, Svella, Apex of Power) still yields no gate. The parser half alone was inert. The chain seam forwards every exiled card as the sub-ability's targets, so `target_ids` arrived non-empty and skipped the one site where the cast filter was applied — every exiled card got the permission regardless of type. `cast_from_zone` now retains only forwarded ids matching the clause's own legs, using a new `TargetFilter::without_exile_anaphor()` that discharges the `ExiledBySource` leg the seam already satisfied while preserving And/Or structure. Re-evaluating the anaphor here would be actively wrong: on a triggered ability it reads a snapshot captured before this ability's own exile step, which would drop every id and turn the bug into a total no-op. Scoped to filters that reference the exile anaphor, so explicitly targeted grants (Emry, Bring to Light, Urza) are untouched, and the 51 bare-anaphor rows residualize to None and keep the full forwarded set. Removes Scarlet Witch, Chaotic Avenger from parser-misparse-backlog root cause #6. Epic Experiment stays under #1: its "that weren't cast" cleanup clause is still dropped. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> * fix(PR-6996): preserve hand cast gate predicates * test(PR-6996): distinguish hand cast type gate * fix(PR-6996): complete hand gate regression loop * test(PR-6996): select hand binding in rich gate regression --------- Co-authored-by: Claude Opus 5 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
keloide
pushed a commit
that referenced
this pull request
Aug 25, 2026
…phase-rs#7703) * docs: custom format engine design proposal (discussion phase-rs#5312) Research + design for a general, data-driven custom-format layer, validated by expressing four Eternal Central retro formats (Old School 93-94, Old School 95, Middle School, Classic Magic) as data on top of it rather than four hardcoded GameFormat variants. Schema splits CustomFormatRules into two independent axes: StructuralRules (life, players, deck size, range of influence, team-based, singleton — already FormatConfig fields, already partially host-adjustable in the lobby) and LegalityRules (legal sets, banned/restricted, legacy rules like mana burn / damage-on-stack / pre-M10 Wish / legend-rule scope). Delivery recommendation is a "save as custom format" action on the existing lobby first (Axis A), with the four EC formats shipping as audited presets on the same schema (Axis B) in parallel. No engine or frontend code — design/research docs only, opened for maintainer review. * docs(custom-format-engine): narrow MVP to Swedish Old School + Axis A only Full LegacyRuleSet engine wiring (mana burn, damage-on-the-stack, pre-M10 Wish, legend-rule scope) is real risk and makes the MVP harder to test. Swedish Old School 93/94 — a distinct, real ruleset verified this session against oldschool-mtg.blogspot.com/p/banrestriction.html — has its own restricted list (23 names, different from EC's), an empty banned list, and no mention of mana burn or any other legacy rule, so it needs none of that wiring. Premodern already exists as a native GameFormat and needs no new work at all; it's cited only as an existing precedent for the same shape. Re-sequences to two phases: phase 1 ships the general engine + the Axis A lobby-save action + swedish_old_school() as the only new Axis B preset (zero LegacyRuleSet wiring exercised); phase 2 ships the four EC formats plus the legacy-rules engine work they actually need. Nothing is cut — the four EC formats and full legacy-rules axis remain the target, they just move to a phase that ships once the schema is already proven end-to-end by something smaller. Flags two new open items: ante-card handling (a third list-shaped rule, distinct from banned/restricted, with no schema slot yet) and Swedish Old School's reprint policy (unconfirmed against the primary source). * docs(custom-format-engine): address matthewevans's CHANGES_REQUESTED review Fixes all five design-correctness gaps from the round-2 review: 1. legal_sets was a bare Vec<SetCode>, so Axis A's default (no set restriction) evaluated as "restricted to nothing" and rejected every card. Changed to Option<Vec<SetCode>> (None = unrestricted). 2. StructuralRules dropped command_zone, commander_damage_threshold, and archenemy_player, and had no source for sideboard_policy (which turns out to be a GameFormat method, not a FormatConfig field, so Custom has no derivation path for it). Added all four; uses_commander is now derived from commander_damage_threshold rather than stored redundantly; supplies_fixed_deck stays false for Custom; allow_debug_actions is correctly excluded (orthogonal to format per its own doc comment). 3. No identity/persistence/transport contract existed for a lobby-saved format. Resolved by separating two conflated concerns: in-game peer agreement (already solved -- FormatConfig.custom_rules carries the full payload, not a lookup key) from a player's reusable saved-format library (client-side-only, never an engine/WASM type). Flags the real version-skew risk (an old client can't be rescued by serde(default) on an enum variant it doesn't know) as a lobby-join-handshake requirement. 4. Mana burn was modeled as damage at every engine Phase-enum transition. Verified against docs/MagicCompRules.txt:8278: it's life loss, not damage. Verified against the engine's own Phase enum (types/phase.rs): it flattens MTG's steps and phases into one flat list, so gating on every transition fires mid-phase (e.g. DeclareAttackers -> DeclareBlockers), not just at real phase boundaries. Also found an existing generic mechanism this can reuse: player_unspent_mana_loss_causes_life_loss / apply_empty_mana_pool_event, currently used for a Yurlok-class card-granted ability at full CR 500.5 granularity. Redesigned as a phase-group-boundary-gated second contribution to the same event, independent of the Yurlok-class check. 5. Swedish Old School's restricted list was mislabeled 23 when 25 names are enumerated, and the preset sketch had dropped Summer Magic from the legal-sets list. Classic Magic's restricted list was labeled 37 when 44 are enumerated. All four fixed and cross-checked between CONTEXT.md, PLAN.md, and RESEARCH.md. * docs(custom-format-engine): address round-2 CHANGES_REQUESTED (matthewevans) Round 2's fixes were themselves incomplete or wrong on all five points matthewevans re-flagged. Each re-verified directly against the cited engine source before treating the review as correct, not accepted at face value: 1. Round 2 only gated the mana-burn LIFE-LOSS check to phase-group boundaries, leaving the pool-emptying event firing unconditionally on every Phase transition -- so by the time a boundary was reached, the pool was already silently drained with nothing left to burn. Fixed by reusing an existing mechanism instead of gating a side-effect on an event that already ran: the engine's ManaExpiry type already has EndOfCombat ("persists through combat steps, drains at EndCombat -> PostCombatMain", used by Firebending) -- generalized with a third variant, EndOfPhaseGroup, so mana_burn-tagged mana actually persists across intra-phase-group steps and only drops (and burns) at a real phase-group crossing. 2. StructuralRules.sideboard_policy had no accessor -- GameFormat:: sideboard_policy() can't see FormatConfig.custom_rules at all. Added FormatConfig::sideboard_policy() as the single canonical accessor and specified migrating both production call sites (deck_loading.rs, match_flow.rs) to it. 3. archenemy_player is per-game seat identity validated against that game's player count (FormatConfig::archenemy_player/validate_for_player_count), not a reusable structural setting -- removed from StructuralRules entirely; Axis A doesn't support the Archenemy topology. 4. uses_commander was derived from commander_damage_threshold alone; GameFormat::uses_commander()'s own doc comment states the real invariant requires command_zone AND the threshold. Fixed the derivation to use both conditions. 5. ReprintPolicy is declared but never consumed by the evaluator, and LegendRuleScope::PreM14AnyController's historical conflation was never resolved. Added a general preset-readiness rule: a preset can't be registered as selectable until every legality/legacy field it declares is both specified and actually enforced -- this currently blocks swedish_old_school() specifically. Also fixed two more lingering 23-vs-25 restricted-list references round 2 missed. * docs(custom-format-engine): full audit of all 4 review rounds, address round 3 Per direct instruction, re-audited every point raised across all four review rounds against current source (not against prior claims) rather than only fixing the newest round. Found one additional real gap nobody had named yet: deck_validation.rs's DeckCompatibilityRequest.selected_format has the same bare-GameFormat problem as companion.rs, just never flagged by anyone. 1. Round 3's sideboard_policy fix used FormatConfig::sideboard_policy() as a method with .expect("Custom format must carry custom_rules") -- a production panic path -- and migrated only 2 of 7 real consumer call sites (companion.rs x4, deck_loading.rs x2, match_flow.rs x2, deck_validation.rs x5). Fixed: fallible validation of the format/custom_rules invariant at every construction/ingestion point (malformed values rejected at the boundary, never constructed); sideboard_policy becomes a stored FormatConfig field matching the existing uses_commander/supplies_fixed_deck pattern (verified via their own consistency test at format.rs:1512-1513), not a new method; every real consumer migrates to read it, with signatures widened wherever they only carry a bare GameFormat today (companion_offers, DeckCompatibilityRequest). 2. from_lobby_config never specified where sideboard_policy comes from. Fixed: config.format.sideboard_policy(), valid because the conversion's input is always a built-in format at save time. 3. LegacyRuleSet's three bools (mana_burn, damage_uses_stack, pre_m10_wish_reaches_exile) become typed enums (ManaBurnPolicy, CombatDamageTiming, WishOutsideGameScope), matching LegendRuleScope's existing shape. Tightened the preset-readiness gate: no preset may ship in a "playable with a caveat" state -- retracts this doc's own earlier claim that Middle School/Classic Magic could ship before damage-on-stack lands. 4. Designed the version-skew compatibility fix concretely instead of flagging it: reuse the engine's existing PROTOCOL_VERSION/ MIN_SUPPORTED_PROTOCOL handshake gate (server-core/protocol.rs) rather than inventing new negotiation. For ReprintPolicy enforcement, named two sufficient resolution paths (the general printing cross-reference, or a one-preset verification pass) rather than building the full model now. Also fixed two stale pre_m10_wish_templating references in RESEARCH.md that survived since round 1 despite PLAN.md already using the canonical name throughout. * docs(custom-format-engine): inline source citations per format, not just once Every card list/count in RESEARCH.md shared one citation at the top of section 1, and Swedish Old School -- the actual phase-1 target preset -- had no RESEARCH.md presence at all, only scattered mentions in CONTEXT.md. That makes independent verification harder than it should be, especially given every "preset data inconsistency" finding across four review rounds was an internal cross-reference mismatch, not an external-source check -- worth making the external source trivially reachable at the point of data. - Added a "Source:" line to each of the four existing EC format subsections. - Added a full "Swedish Old School 93/94" subsection to RESEARCH.md #1, matching the EC formats' treatment: direct source URL, its own verbatim legal-sets/banned/restricted/ante/legacy-rules data, and an explicit side-by-side comparison against EC's 93-94 restricted list proving these are two different, real rulesets, not one re-presented as two. - Added the same two source URLs inline at the top of PLAN.md #2, so the preset constructors are checkable without cross-referencing another file. * docs(custom-format-engine): address CodeRabbit findings on the round-4 commit Automated review on the round-4 fix commit (3aa20f1) caught 5 things, at least 3 of which are real: 1. A genuine mistake in round 4's own mechanical rename fix: the search_outside_game pseudocode was updated from the round-1 placeholder name to pre_m10_wish_reaches_exile, but the SAME round-4 commit also converted that field to the typed wish_scope: WishOutsideGameScope enum -- the pseudocode never got updated to match, in the same commit that introduced the mismatch. Fixed, with the naming-history note now tracking both renames. 2. sideboard_policy/uses_commander are plain serialized FormatConfig fields (no #[serde(skip)]) that could diverge from what's derivable from `format`/`custom_rules` on a malformed wire payload -- a real gap that already exists for built-in formats today, not just custom ones. Widened validate_custom_rules_consistency to check derived-field agreement for every format, not only Custom. 3. DeckCompatibilityRequest.selected_format specifically needs the full CustomFormatRules (legal_sets/banned/restricted), not the lighter ResolvedFormatFacts struct that's sufficient for companion.rs's two call sites -- tightened from an ambiguous "per-site judgment call" to an explicit distinction. 4. The protocol-version fix only bumped PROTOCOL_VERSION; format selection happens during lobby setup, so LOBBY_PROTOCOL_VERSION / MIN_SUPPORTED_LOBBY_PROTOCOL (a separate, real constant pair, confirmed this session) needs bumping too. 5. CombatDamageTiming::OnStack's doc comment mischaracterized historical combat damage as a triggered ability; RESEARCH.md #6 already correctly describes it as assigned damage placed on the stack as a stack object. Fixed to match. Also strengthened the preset-readiness gate from a documented convention into an actual technical mechanism (custom_format_registry() validates against a static implemented-axes table before returning a preset) -- responds to a stricter CodeRabbit read of the ReprintPolicy gate without fully adopting its more extreme "remove the field" suggestion, which goes beyond what the human reviewer (matthewevans) actually asked for. * docs(custom-format-engine): address round-5 CHANGES_REQUESTED (matthewevans) Round 5 opened with "the proposal now resolves the previously-requested custom-context, typed-policy, compatibility, and no-caveated-preset concerns" -- confirming round 4 in full. Two narrower points remained: 1. StructuralRules.singleton was declared (since round 2's full-fidelity fix) but evaluate_custom_format's step 5 always called copy_limit_violations(db, &counts, 4), never reading it, and there was no test. Fixed: parameterize on rules.structural.singleton (1 vs 4) -- confirmed this round that copy_limit_violations already takes exactly this parameter and every built-in singleton format already calls it with 1, so this is parameterizing an existing call, not new logic. Card-intrinsic overrides (Relentless Rats, DeckCopyLimit::UpTo) already compose correctly under any limit per the helper's own existing tests. 2. The registry gate (IMPLEMENTED_LEGACY_AXES) covered only LegacyRuleSet's four axes, not ReprintPolicy -- a preset could still register while declaring an unenforced ReprintPolicy. Rather than broadening the gate to a field never designed to be independently enforceable, re-read this document's own original research (RESEARCH.md #3, predating any review round): it already concluded ReprintPolicy's behavior is fully absorbed into legal_sets curation, with only the frame/art-level distinction being a real gap (Open item 2's, not this field's). Resolution: reprint_policy becomes documentation metadata, deliberately not consumed by the evaluator and deliberately outside the registration gate's scope -- satisfying the "keep/reduce to non-selectable/deferred metadata" branch round 5 explicitly offered as acceptable. * docs(custom-format-engine): address round-6 CHANGES_REQUESTED (matthewevans) Round 6 confirmed the singleton fix landed correctly and found one remaining issue: round 5 resolved ReprintPolicy to "documentation metadata, never consumed" but left the field sitting inside LegalityRules/ CustomFormatRules -- the resolved, engine-consumed, wire-traveling payload -- while a comment beside it (unchanged since round 1) still said it "gates LEGALITY". Two incompatible claims about the same field's contract: a struct's shape is itself a claim about what travels with and is enforced by the resolved ruleset, and no comment disclaiming that changes what the type says. Resolved structurally, not documentarily: reprint_policy moves out of CustomFormatRules/LegalityRules entirely, onto a newly-sketched CustomFormatDef struct (previously only described in prose as "display metadata + CustomFormatRules" -- exactly the ambiguity that let this happen), alongside label/short_label/description. This is one of the two resolutions matthewevans offered, chosen over building real engine-owned printing enforcement, for the same reason round 5 established: this document's own original research already showed the field's behavior is fully absorbed by legal_sets curation. Updated every preset sketch, the preset-readiness gate's scope description, and the registry-gate reasoning to match -- the gate no longer needs to exempt reprint_policy since it isn't on the resolved-rules struct at all. Added a discriminating test proving two CustomFormatDef values with identical rules but different reprint_policy produce identical evaluation results. * docs(custom-format-engine): address round-7 CHANGES_REQUESTED (matthewevans) Round 7 confirmed the round-6 structural move was correct ("correctly resolves the prior semantic contradiction") and found one construction gap: CustomFormatDef.reprint_policy was a required ReprintPolicy, but from_lobby_config has no legitimate value to put there -- a lobby-saved format has no authored paper-format reprint intent at all, so forcing any of the three real variants onto it would be fabricated metadata. Fixed: reprint_policy: Option<ReprintPolicy>. None for from_lobby_config (and, for now, swedish_old_school() pending Open item 6) -- the same Option<T>-over-forcing-a-value pattern this proposal already uses for legal_sets and range_of_influence. Some(_) with a real, sourced value for every other Axis B preset. Also fixed a stale round-1 cross-reference in CONTEXT.md's original Axis B field list that still named reprint_policy alongside the genuinely resolved-legality fields, never updated when round 6 moved it. Added a test requiring each of the five preset constructors to set its own specific value, not one shared "is Some" check. * docs(custom-format-engine): address round-8 CHANGES_REQUESTED (matthewevans) from_lobby_config's signature (name + &FormatConfig) never specified how a lobby save derives short_label/description, but CustomFormatDef requires both non-optionally since round 6. Fixes by deriving short_label as name's first 3 alphanumeric chars uppercased (the same convention the frontend already falls back to independently for unrecognized formats) and description via a new derive_structural_description(&StructuralRules) helper mirroring built-in formats' existing comma-joined structural phrasing — both derived from name/config alone, no invented metadata. Extends the existing lobby-save round-trip test to assert the derivation. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-9 CHANGES_REQUESTED (matthewevans) The round-8 fix put label/short_label/description on CustomFormatDef, but that struct never travels past the lobby/picker — FormatConfig.custom_rules carries only CustomFormatRules, and SavedCustomFormat.name is explicitly client-local by round 2's own identity design. GameFormat::label() also structurally cannot return CustomFormatDef.label (a String) from a &'static str-returning function regardless of what's threaded in. Fixes both without adding wire surface: label() becomes Cow<'static, str>, resolving Axis-B presets via a custom_format_registry() lookup (a stable id every peer already shares) and falling back to a fixed "Custom Format" string for Axis-A ad-hoc saves, which have no registry entry to resolve at all. Retracts PLAN.md's prior "label/for_format get a Custom arm reading the resolved def" claim, which was never true for Axis A. Also found and fixed a second, independent instance of the same root cause: FormatConfig::for_format(bare GameFormat) is called for .deck_size at two real deck-validation call sites, silently returning a wrong default size for Custom formats. Gives for_format the same unreachable!() guard sideboard_policy/uses_commander already use, migrating both call sites to read custom_rules.structural.deck_size when present. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-10 CHANGES_REQUESTED (matthewevans) Matt flagged that Old School 93-94/95 register once mana burn lands with no gate tied to their source rules' printing-fidelity requirement (non-foil, original frame + art) - legal_sets is set-code-only and can't express this, and CONTEXT.md's own open item 2 already flagged the gap as unresolved, but the rollout plan had drifted out of sync with it. Resolved directly (discussed with the repo owner rather than decided unilaterally, per the doc's own "do not resolve unilaterally" instruction on that open item): legality enforcement stays legal_sets-only permanently -- no foil/frame data exists anywhere in the engine to enforce against. The source rules' spirit is instead honored by a general display fix: ArtChainEntry's existing {type: "oldest"} per-player preference becomes legal_sets-aware, so "oldest printing" respects the active format's legal set list instead of picking a promo/non-tournament printing the format doesn't recognize. Zero engine change, not gated on ReprintPolicy, benefits every format with a legal_sets restriction, not just the three Axis-B presets that declare one. Also backfills CONTEXT.md's round-history log with rounds 8 and 9, which only got PLAN.md updates in the prior two commits. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-11 CHANGES_REQUESTED (matthewevans) Round 10 tried to resolve Old School 93-94/95's printing-fidelity gap by pairing legal_sets-only legality with a cosmetic ArtChainEntry display default. Matt correctly rejected this: an optional rendering preference is not a legality resolution. Resolved via a third path, grounded in existing precedent rather than a new policy call: confirmed GameFormat::Premodern's legality (LegalityFormat::Premodern) is oracle-card-level only, like every format in this engine -- none has ever checked printing, frame, or foil, and PrintedCardRef has no set-code field for any of them. legal_sets membership isn't an old-school-specific approximation needing a gate; it's this engine's one existing legality model, applied the same way to every format. Registration reverts to mana-burn-only, matching the original pre-round-10 gate. The ArtChainEntry display fix and genuine per-card printing selection (briefly surveyed: a moderate plumbing lift reusing existing PrintingPickerModal/sourcePrinting infrastructure, not from-scratch) are both retracted from this proposal as separate, real, future ideas -- general to every format, not bundled into this proposal's legality story. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs(custom-format-engine): address round-12 CHANGES_REQUESTED (matthewevans) — Old School printing fidelity Round 11's resolution ("legal_sets membership isn't an approximation of Old School 93-94/95's legality — it's the same oracle-card-level model Premodern and every other format already uses") was reviewed again and correctly rejected: Premodern never claimed a printing-level requirement in the first place, so it not enforcing one isn't an approximation of anything. Old School 93-94/95's own cited source (RESEARCH.md §1) explicitly requires non-foil original-frame/art reprints, and legal_sets genuinely falls short of that specific stated rule regardless of what every other format checks. Product decision (not resolved away by precedent this time): decline to build engine-owned printing/frame enforcement — the engine's and frontend's printing systems are confirmed disconnected, and wiring them is a real, separate, moderate-lift future feature, not old-school-specific. Accept the set-code-only approximation on its own terms: in a digital-only client, a printing's frame/border/foil status has zero gameplay consequence, since two printings with identical Oracle text are identical for every rules purpose the engine cares about. The paper community's frame/art requirement serves an anti-counterfeiting/provenance function specific to a physical table that has no digital equivalent. Takes the maintainer's own second offered resolution (explicitly scope the presets as an oracle-card/set-code approximation) and makes it structural rather than documentary, per this proposal's own established standard that conventions must be enforced, not just written down: - New `CustomFormatDef.printing_fidelity: PrintingFidelity` field (NotApplicable / SetCodeApproximation), required and paired with `reprint_policy` at construction time. - All four EC presets (old_school_93_94, old_school_95, middle_school, classic_magic) set SetCodeApproximation and must disclose the limitation in their player-facing `description`, not just a doc comment. - New registry gate (§7) and test (§6) enforce the reprint_policy/ printing_fidelity pairing and the description disclosure — separate from the existing IMPLEMENTED_LEGACY_AXES gate, which covers unimplemented engine work; this one covers an authoring omission. - CONTEXT.md's Open item 2 log gets an appended correction + final resolution (round 12), not an overwrite of round 11's now-superseded entry. Also fixes the CodeRabbit-flagged [MEDIUM]: old_school_95()'s builder called `d.legal_sets.extend(...)` directly, but legal_sets is `Option<Vec<SetCode>>` per §1 — `.extend()` doesn't exist on `Option`. Uses `get_or_insert_with(Vec::new).extend(...)` so it stays correct even if a future refactor changes the base preset's Some/None invariant. Model: claude-sonnet-5 Tier: Frontier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ1bpYEt331DvjqPLSFJEH * docs(custom-format-engine): fix old_school_95() field nesting + add inheritance test Maintainer review of the round-12 commit confirmed the printing-fidelity blocker resolved, but caught that old_school_95()'s builder sketch mutates d.legal_sets/restricted/banned directly — those aren't fields on CustomFormatDef at all; they live at d.rules.legality.* per §1's own declared schema (CustomFormatDef.rules: CustomFormatRules, CustomFormatRules.legality: LegalityRules). The prior get_or_insert_with fix was correct for the Option<Vec<SetCode>> detail but applied at the wrong nesting level. Every mutation now composes through the real path (d.rules.legality.legal_sets.get_or_insert_with(...).extend(...), etc.), and adds the requested preset-inheritance test: asserts old_school_95()'s resolved legal_sets/restricted/banned each equal old_school_93_94()'s base plus exactly its own declared delta (an exact-set comparison, not a superset check), which would also have caught this nesting bug mechanically. Model: claude-sonnet-5 Tier: Frontier Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01NQ1bpYEt331DvjqPLSFJEH --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
7 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Fixes the dropped intervening-if / gating condition misparse (backlog root cause #2) for Shadowborn Demon — "At the beginning of your upkeep, if there are fewer than six creature cards in your graveyard, sacrifice a creature." The existential threshold combinator
parse_there_are_conditionshad a suffix-only comparator axis ("N or more" → GE, bare → EQ) and no prefix axis, so "there are fewer than N <countable> <scope>" failed to parse: the CR 603.4 intervening-if was silently swallowed and the demon sacrificed a creature unconditionally every upkeep while the card stayed coverage-supported. The same root cause left The Warring Triad's "as long as there are fewer than eight cards in your graveyard" gate asStaticCondition::Unrecognized(always-true, backlog root cause phase-rs#13) — the God was never a creature.The fix parameterizes the comparator axis for the class: a new shared
parse_strict_comparator_prefixcombinator ("fewer than " →Comparator::LT, "more than " →Comparator::GT; CR 107.1a) composed asopt(...)intoparse_there_are_conditions, with one exhaustive(prefix, or_more)match that refuses the ungrammatical double-marked form and dedupes the previously duplicated GE/EQ computation.parse_put_onto_battlefield_this_way's pre-existing inline copy of the samealtnow delegates to the shared helper (single authority). Zero new enum variants; zero runtime changes (Comparator::evaluateis already generic at both consumer seams —game/triggers.rsintervening-if andgame/layers.rsstatic gates). Backlog entries for both cards removed with counts updated.Files changed
crates/engine/src/parser/oracle_nom/condition.rscrates/engine/src/parser/oracle_static/tests.rscrates/engine/src/parser/oracle_trigger_tests.rscrates/engine/tests/integration/fewer_than_existential_threshold.rs(new)crates/engine/tests/integration/main.rsdocs/parser-misparse-backlog.mdImplementation method (required)
Method: /engine-implementer
Pipeline as published: plan via
engine-planner→/review-engine-plan(clean, round 1) → implement viaengine-implementation-executor→/review-impl(clean, round 1) → final read-only/review-implagainst the committed head (clean) → commit.CR references
CR 603.4— intervening-"if" triggered abilities (authorizing rule for the Shadowborn Demon trigger path; checked at trigger time and again at resolution)CR 611.3a— continuous effects are not locked in (authorizing rule for the live "as long as" static gate)CR 613.1d— Layer 4 type-changing effects (The Warring Triad's Creature-type removal)CR 107.1a— integer comparisons (strict-inequality prefix idioms; pre-existing convention in this function, propagated)All verified by grep against
docs/MagicCompRules.txt(lines 2592, 2922, 2966, 453).Track
Developer
LLM
Model: Claude Fable 5
Thinking: high
Tier: Frontier
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.
cargo fmt --all— cleancargo test -p engine— 21,142 passed, 0 failed (all suites, incl. the 4 new integration tests; one earlier local run tripped a token-catalog count test against aknown-tokens.tomlthat the setup script had regenerated from a newer MTGJSON snapshot — restored to committed state, unrelated to this diff, passes clean)cargo clippy-strict— clean (exit 0, whole workspace)./scripts/gen-card-data.sh— regenerated; Shadowborn Demon's trigger now carriescondition: creature cards in your graveyard < 6; The Warring Triad's static shows typedconditional: cards in graveyard (you) < 8cargo coverage— 31,501 → 31,502 supported (The Warring Triad flippedsupported: false→true; Shadowborn Demon stayssupported: true, gap_count: 0; net +1, no regressions)cargo semantic-audit— exit 0; zero findings for either card./scripts/check-parser-combinators.sh— PASS (output below)Environment notes: Tilt is not available in this environment, so all checks ran as direct cargo commands (the sanctioned fallback). The §3.4 in-flight check could only scan this repository's open PRs (session GitHub scope is limited to this fork); neither open PR touches this mechanic. The setup WASM sidecar step failed (
wasm-bindgenCLI unavailable) — WASM artifacts are not consumed by Developer-track verification and this change touches no frontend code.Gate A
Anchored on
crates/engine/src/parser/oracle_nom/condition.rs:2643— pre-existingpreceded(tag("fewer than "), parse_number)hand-size gate ("fewer than N cards in hand" → HandSize LT): the strict-inequality prefix family and comparator semantics the new code mirrorscrates/engine/src/parser/oracle_nom/condition.rs:679—parse_put_onto_battlefield_this_way's comparator-prefix site (previously the inlinealt((value(LT, tag("fewer than ")), value(GT, tag("more than "))))this helper was factored from; now delegates to it)crates/engine/src/parser/oracle_nom/condition.rs:3696—parse_ge_threshold, the existing threshold-comparator combinator cluster the new helper is sited beside, samealt((value(_, tag(_)), …))combinator familyFinal review-impl
Final review-impl PASS head=9d1145fa1e2b0745f65b95448b88c2ae1274195d
(Initial clean verdict was issued against 9991c166…, then the commit was amended solely to fix committer identity —
--amend --no-edit --reset-author, tree hash identical (9ec69d51…), empty diff between the two — and the same independent reviewer re-verified tree identity and re-issued the verdict bound to the current head.)Claimed parse impact
Validation Failures
None.
CI Failures
None.
🤖 Generated with Claude Code
https://claude.ai/code/session_01M3jSXUUoGHH2Qh289AMKa9
Generated by Claude Code