Fix Circle of Power: preserve typed-group continuous sibling after a quoted token ability#8
Open
keloide wants to merge 72 commits into
Open
Fix Circle of Power: preserve typed-group continuous sibling after a quoted token ability#8keloide wants to merge 72 commits into
keloide wants to merge 72 commits into
Conversation
* fix(desktop): show native engine provisioning status * fix(release): recover main bundle and clarify desktop downloads --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(ui): surface roll outcomes and turn order * fix(ui): address roll overlay review feedback * test(ui): isolate draft match provider state * fix(tauri): synchronize release lockfile version * fix(ci): attach migration bridge before release publish --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(client): respect legal Adventure cast faces * fix(client): preserve waiting factory variant types * fix(client): retain concrete waiting factories --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(ci): scope preview server secrets * fix(desktop): replay native engine progress --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
phase-rs#6558) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…hange hub) (phase-rs#6560) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…mp (phase-rs#6568) `release: v0.35.2` (21a53d5) bumped `client/src-tauri/Cargo.toml` to 0.35.2 via cargo-release's `pre-release-replacements`, but `client/src-tauri/` is a separate cargo workspace with its own lockfile that cargo-release does not manage. The lock still recorded `phase-tauri 0.35.1`, so the `tauri-check` CI job's `cargo check --locked --manifest-path client/src-tauri/Cargo.toml` refused to reconcile the mismatch and exited 101. That reds the required `Rust (fmt, clippy, test, coverage-gate)` aggregator (which `needs: [... tauri-check]`) on every open PR whose merge ref includes the release commit, blocking the merge queue repo-wide. Evidence: PR phase-rs#6561 tauri-check PASSED at 20:43:00Z; the release commit landed at 20:49:54Z; PRs phase-rs#6564 (20:56:21Z) and phase-rs#6563 (21:15:11Z) both FAILED with the identical --locked error. None of the three touched any Cargo manifest. Follow-up (not in this change): cargo-release should keep the nested lockfile in sync so the next release does not re-break it. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ps (phase-rs#6574) client/src-tauri is its own cargo workspace with its own Cargo.lock, so the existing pre-release-replacement that bumps its Cargo.toml left the lock pinning the previous version. The Tauri CI job builds with `cargo check --locked`, which then refuses to update the lock and fails; because tauri-check is a `needs:` of the required "Rust (fmt, clippy, test, coverage-gate)" aggregator, that reds main and every merge-group ref built on the release commit. v0.35.2 shipped exactly that way and deadlocked the merge queue (fix landed in phase-rs#6568). Verified with `cargo release 0.36.0 --workspace` in dry-run: the new replacement rewrites only the phase-tauri package's version line. The newline is kept inside a capture group rather than in the replacement string: the regex crate expands only ${1}-style references in a replacement, so a literal \n there would be inserted verbatim and splice the two lines together. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…7-23 sweep (phase-rs#6576) * docs(pr-review-loop): encode review-sweep defects found in the 2026-07-23 sweep Four failure modes from a live authorized-maintainer sweep, each of which produced a defective posted review: - Dispatching Review Agents (new): a subagent's final assistant text is not delivered to the lead — findings arrive only via SendMessage, and charters must say so verbatim. Idle means resumable, not dead. Do not substitute a lead-authored review for a slow agent; three of the sweep's four bad reviews came from doing exactly that, and each was overturned by the agent's report minutes later. - Verifying the Review's Own Evidence (new): the CR grep-verification bar binds the reviewer as hard as the contributor, since the contributor acts on what the review says. Check repo convention before proposing a citation, and read the rule rather than trusting a number in roughly the right section — adjacent subrules routinely cover unrelated single cards (612.5 is Exchange of Words; 612.6 is Volrath's Shapeshifter), and layer placement lives in a different section from the effect it orders (613.1c, not 612.x). - Judging the seam (new, under Review Bar): "reuses existing machinery" is not "correct seam". Every proliferation heuristic in this file returned clean on a PR that reused the instance-blind checker while the instance-aware authority twenty lines away already cited the rule and dropped half of it. Grep for the rule, not the symbol; two mechanisms for one rule is the defect. - Maintainer-Caused Staleness: E0004 non-exhaustive-match failures from a PR's own new variants look like contributor sloppiness and are frequently ours — main adds a new match site after the contributor's last push. Timestamp the failing site against the head before writing that finding. - Review Freshness: stale approval plus armed auto-merge is a safety defect, not bookkeeping. With dismiss-stale-reviews off, GitHub keeps reporting APPROVED after a new commit lands, so an unreviewed head can sit armed to merge. Worse when the post-approval commit is maintainer-authored. * docs(pr-review-loop): test compile-error attribution by ancestry, not timestamps Addresses @coderabbitai review on phase-rs#6576. The finding is correct: `.commit.committer.date` is the head commit's author/ committer metadata, not push time, so a rebase, an amend, or a delayed push all skew a date comparison — and a squash-merged main commit carries a date unrelated to when its content actually landed. The suggested remedy (read push time from PR timeline/update events) would fix the skew but still answers the wrong question. What the check actually needs to know is not "which is newer" but "did the branch ever contain the failing code?" — which `git merge-base --is-ancestor` answers directly and is immune to the entire class of timestamp problems, including ones PR timeline events also get wrong. Replaces the three date greps with: 1. `git cat-file -e <headOid>:<file>` — did the file exist at the head at all 2. `git log -S <symbol> -1` + `git merge-base --is-ancestor` — is the commit that introduced the match site in the branch's history 3. the unchanged PR file-list check Also adds a non-vacuity probe, because a check that returns "not an ancestor" for every input is broken rather than exonerating: a commit known to be in the branch's history (`git merge-base origin/main <headOid>`) must report ancestor. Verified verbatim against the case that motivated the section (phase-rs#5866): step 1 reports interaction.rs absent at the head, step 2 reports the shared.rs match site not in the branch's history, and the probe reports the real merge-base df2ab2d as an ancestor — so the test discriminates rather than always failing closed. Timestamps stay as human-readable colour in the review comment, relabelled "committed at" rather than "pushed at". --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Bumps the npm_and_yarn group with 1 update in the /client directory: [react-router](https://github.com/remix-run/react-router/tree/HEAD/packages/react-router). Updates `react-router` from 7.15.1 to 7.18.0 - [Release notes](https://github.com/remix-run/react-router/releases) - [Changelog](https://github.com/remix-run/react-router/blob/main/packages/react-router/CHANGELOG.md) - [Commits](https://github.com/remix-run/react-router/commits/react-router@7.18.0/packages/react-router) --- updated-dependencies: - dependency-name: react-router dependency-version: 7.18.0 dependency-type: direct:production dependency-group: npm_and_yarn ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…s#6578) Every asset on a web release is a machine artifact the desktop shell provisions for itself, and those releases fire far more often than shell releases — so /releases/latest showed a page of phase-server-slim binaries with no installer on it, which is not what anyone visiting the releases page is looking for. Web releases now opt out of the badge and the shell release claims it. Also names the shell release "phase.rs Desktop", matching the app's own window title, so the download is identifiable as the desktop app rather than as a bare tag. The live shell-v1.0.1 release was retitled and pinned by hand; this makes the next one do it on its own. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…rs#6554) * Fix Lictor opponent-scoped "entered this turn" intervening-if Lictor's Pheromone Trail — "When this creature enters, if a creature entered the battlefield under an opponent's control this turn, create a 3/3 green Tyranid Warrior creature token with trample" — dropped its intervening-"if" condition (parsed to None), so the ETB trigger fired unconditionally and the token was created every time Lictor entered. The "under your control" surface of this class was already supported; this adds the opponent-scoped, past-tense mirror. A new `parse_entered_this_turn_under_opponent_control` combinator reuses the shared `parse_or_more_entered_count` / `parse_entered_this_turn_subject` helpers and carries the scope via `PlayerScope::Opponent { Max }` — the existential "an opponent" reading already documented on `parse_opponent_had_entered_this_turn` — over the CR 608.2i `BattlefieldEntriesThisTurn` snapshot. No new engine variant. The opponent surface in the QUANTITY path stays honestly Unimplemented (no printed card); only the condition path, which Lictor uses, is added. Adds parser unit tests (singular + count forms) and a discriminating runtime test that casts Lictor and asserts the token is gated on an opponent's entry (fails on revert). Removes Lictor from the parser misparse backlog. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Gw63XPowSxhpJaS9xM8Xyj * test(PR-6554): pin opponent aggregate to Max in Lictor shape assertions The three shape assertions matched `PlayerScope::Opponent { .. }`, which also admits `AggregateFunction::Sum` — precisely the cross-opponent summation the combinator's doc comment identifies as wrong and that is only observable at three or more seats. Every test the PR adds would still have passed if the parser were later changed to emit `Sum`. Tighten all three sites to `{ aggregate: AggregateFunction::Max }` so the existential "an opponent" reading is actually pinned by the test suite. Co-authored-by: keloide <75585494+keloide@users.noreply.github.com> --------- Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ve layer resets (Rotting Rats + Sacrifice) (phase-rs#6538) Unearth installs a rider — "if it would leave the battlefield, exile it instead of putting it anywhere else" (CR 702.84a) — on the returned creature, but it was pushed to the object's LIVE replacement set only. Every layer pass reseeds live characteristics from base (CR 613.1), so the rider was wiped before any real leave event; sacrificing, destroying, or bouncing an unearthed creature sent it to the graveyard instead of exile. Only the end-step delayed-trigger half worked (it is state-hosted, not object-hosted). Adds a typed RestrictionExpiry::UntilHostLeavesPlay stamped on the rider at both build sites (unearth synthesis + the Whip-of-Erebos-class parser builder). The stamp (1) forces base-install so the rider survives CR 613.1 reseeds, (2) marks the def non-copiable (CR 707.2 — token copies never inherit it), and (3) prunes it base+live when the host leaves the battlefield (CR 400.7 — a re-entered same-ObjectId object is a new object and must not be re-exiled). Object-bound, so it survives control theft. Fixes ~58 unearth cards + 6 leave-battlefield-rider cards (Whip of Erebos, Gruesome Encore, Isareth, Kheru Lich Lord, Moira and Teshar, From the Catacombs). Personal Decoy's printed static (no expiry stamp) stays copiable — the detectors key on the stamp, not the shape. Closes phase-rs#5976 Co-authored-by: michiot05 <281539540+michiot05@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…6498) (phase-rs#6545) * fix(engine): keep Portent exile picks out of the graveyard (phase-rs#6498) Model put-the-rest after ForEachCategory as LastRevealed still in the library, and preserve dynamic X on reveal Digs instead of collapsing to RevealTop{1}. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-6545): remove duplicate PR narrative * fix(engine): resolve Portent's two remainder sets and hand tail (phase-rs#6498) Separate revealed-library rest from exiled-card cleanup via LastRevealed and TrackedSetFiltered(Exiled), emit explicit LastRevealed siblings for dynamic reveal Digs, and lower the hand tail as ChangeZoneAll. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): unblock PR 6545 clippy and dynamic-reveal rest path (phase-rs#6498) Fix clippy if-same-then-else, emit LastRevealed siblings for dynamic reveal Digs before assembly demotion, and add parse coverage for the shared put-the-rest grammar class Matthew requested. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): add runtime coverage for dynamic reveal rest path (phase-rs#6498) Add a discriminating cast-pipeline test for the shared dynamic-reveal + put-the-rest grammar class, stamp library-bottom placement on the explicit LastRevealed sibling, and narrow TrackedSetFiltered mass-move origin defaults. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(test): assert exact library-bottom order in dynamic reveal regression The runtime sibling test for reveal-only Dig + put-the-rest was failing CI because it assumed reveal order rather than the engine's deterministic batch placement order. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): prompt library order for ChangeZoneAll bottom rest (phase-rs#6498) CR 401.4: when multiple cards are placed at the same library position with random_order false, route through EffectZoneChoice instead of a silent batch order. Update the dynamic-reveal sibling regression to submit a non-default bottom tail and verify it sticks. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): keep up-to tracked-set puts on ChangeZone (phase-rs#6498) The TrackedSetFiltered mass-move promotion must not absorb bounded "up to N" picks like "put up to one land discarded this way onto the battlefield". Co-authored-by: Cursor <cursoragent@cursor.com> * fix * fix * fix * fix(engine): use APNAP order for per-owner library prompts (phase-rs#6545) CR 101.4: group mass library-order batches via apnap_order instead of seat id, make the multi-owner unit test active on P1's turn, and scope scenario auto-answers to PutAtLibraryPosition only. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ci): unblock PR 6545 clippy and Tauri lock sync Remove unnecessary usize casts in scenario auto-answer path and bump phase-tauri to 0.35.2 in client/src-tauri/Cargo.lock after the main merge. Co-authored-by: Cursor <cursoragent@cursor.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>
…(Solar Array phase-rs#5337) (phase-rs#5802) * fix(engine): honor dynamically granted Sunburst at battlefield entry (Solar Array phase-rs#5337) "That spell gains sunburst" (Solar Array) and "it gains sunburst" (Lux Artillery) placed ZERO as-enters counters. Three stacked defects, fixed per layer: 1. Parser — the grant never landed (Solar Array). In a "when you next cast a <spell> this turn" delayed trigger, the subject-position "that spell" anaphor lowered to `affected: ParentTarget`; a `WhenNextEvent` delayed trigger has no parent target, so `register_transient_effect` bound the grant against the empty chain-tracked set and silently dropped it. A new lift (`lift_generic_effect_parent_target_to_triggering_source_in_ability`, mirroring the oracle_trigger.rs lift family) rebinds GenericEffect grants in a WhenNextEvent body to `TriggeringSource` — the just-cast spell is the event source (CR 608.2k). 2. Engine — a landed grant still placed no counters (the class bug, also breaking Lux Artillery). Printed sunburst is realized as an object-carried ETB `ReplacementDefinition` pre-synthesized from the card face's PRINTED keywords (`synthesize_sunburst`); a granted keyword adds no replacement and nothing consulted live keywords at entry. A granted-instance VIRTUAL replacement candidate now surfaces in `find_applicable_replacements` (alongside the shield/finality-counter virtual candidates), built from the same shared `sunburst_replacement_definition` authority the printed synthesis uses, so it participates in normal CR 616 replacement ordering (Doubling Season doubles it). Granted instances are counted as EFFECTIVE minus BASE keywords via `effective_off_zone_keywords` — the entering spell is still on the STACK when its entry pipeline runs, and a granted keyword exists only as a continuous effect at that moment (the materialized keyword list is empty), so the off-zone authority is the only correct read. The base subtraction keeps printed instances on their carried definitions — printed + granted apply separately, exactly CR 702.44d. 3. Engine — cast-trigger resolution wiped the color provenance. Resolving an intervening cast trigger (Lux Artillery's own grant trigger) cleared `colors_spent_to_cast` on objects still on the STACK, erasing the color count before the granted spell entered. The clear now preserves live cast provenance for stack objects (CR 601.2h), mirroring how cast_from_zone is preserved. Supporting changes: `Keyword` instance-coexistence predicate extended so a granted Sunburst survives next to an identical printed instance (CR 702.44d "each one works separately"; previously only Toxic's summation kept duplicates); `synthesize_sunburst`'s per-instance definition extracted into the shared `sunburst_replacement_definition` builder used by both the printed synthesis and the virtual candidate. Tests (integration, real activate/cast/trigger/replacement pipeline, verbatim Oracle texts): - Solar Array: creature cast for 3 colors -> 3 +1/+1; noncreature for 2 -> 2 charge; zero colored mana -> 0 counters (CR 702.44b). - Lux Artillery: 2 colors -> 2 +1/+1 (gap 2+3 canary). - Printed control: 3 colors -> 3 charge (carried-definition path untouched). - Printed + granted: 2 colors -> 4 charge (CR 702.44d separateness). - Counter doubling: granted sunburst under an AddCounter doubler -> 4 (CR 616 ordering through the virtual candidate). - Parser shape: the delayed grant lowers `affected: TriggeringSource` with the Sunburst AddKeyword (gap 1 canary). Full lib suite: 16549 passed, 0 failed. Integration: 3038 passed, 0 failed. Fixes phase-rs#5337. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style: restore the Magus Lucea Kane doc comment to its function (clippy empty-line-after-doc) The phase-rs#5337 parser-shape test was inserted between an existing doc comment and its function; reattach the comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): classify granted sunburst as an additive counter-payload write (CR 616.1e) Review on phase-rs#5802: the virtual granted-sunburst candidate was classified `Disjoint` in `candidate_materiality`, but its applier appends to the same event counter payload a co-firing Count writer modifies — an append does not commute with a doubler ((0+N)*2 vs 0*2+N), so `Disjoint` silently suppressed the CR 616.1e affected-controller ordering choice. Reclassify as `Writes { field: Count, commute: Additive }`: two appenders still commute (no degenerate prompt), while an appender + any non-additive Count writer on one event now surfaces the ordering prompt. Integration regression (both fail on revert to `Disjoint` with "the CR 616.1e ordering prompt must surface"): granted sunburst + a same-event Moved-keyed `quantity_modification(DOUBLE)` writer co-fire on the entering spell's ZoneChange; the ordering prompt lists both candidates, and BOTH legal orderings are driven end-to-end to a clean entry with the granted payload intact. Empirical note recorded in the test doc: the two orderings currently converge (2 counters each) because a bare `quantity_modification` on a Moved-keyed definition has no ZoneChange counter-payload applier yet — the functioning Doubling Season path scales the downstream AddCounter placement instead (asserted at 4 by `granted_sunburst_participates_in_counter_ doubling`). The reclassification pins the ordering machinery so that if a ZoneChange payload applier lands later, only the expected totals change (4 sunburst-first vs 2 writer-first), not the choice surface. Full lib: 16549 passed. Integration: 3040 passed. Clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(engine): generalize the granted-ETB-keyword path to Bloodthirst (Bloodlord of Vaasgoth, phase-rs#5802 review) matthewevans's [MED] review: the virtual ETB replacement handled only granted Sunburst, leaving the equivalent granted Bloodthirst path (Bloodlord of Vaasgoth: "Whenever you cast a Vampire creature spell, it gains bloodthirst 3") incorrect — a new special case beside an equally-broken keyword. Generalize the machinery into a keyword-agnostic granted-ETB-keyword path covering both Sunburst and Bloodthirst (and any future as-enters keyword): - `GrantedEtbKeyword { Sunburst, Bloodthirst }` with per-keyword reserved virtual-candidate ids (`GRANTED_SUNBURST_INDEX = MAX-7`, `GRANTED_BLOODTHIRST_INDEX = MAX-8`) feeding a shared count/apply core; `rid.index` recovers the keyword. - `granted_keyword_etb_instances` factors out the EFFECTIVE-minus-BASE keyword count (via `effective_off_zone_keywords`, since the entering spell is still on the stack); `granted_sunburst_instances` delegates to it, `granted_bloodthirst_instances` counts per distinct `BloodthirstValue` (mirroring `synthesize_bloodthirst`, CR 702.54c). - `bloodthirst_replacement_definition` extracted in synthesis.rs (mirroring `sunburst_replacement_definition`); both the printed synthesizer and the virtual applier build per-instance definitions from the same authority. - `apply_granted_keyword_etb_replacement` is keyword-agnostic and honors each definition's carried `condition` via `evaluate_replacement_condition` — Bloodthirst fixed-N only places counters when an opponent was dealt damage this turn (CR 702.54a); Sunburst has `condition: None` and always applies. `granted_etb_keyword_candidate_applies` re-checks the condition at registration so a condition-unmet grant raises no spurious CR 616.1e prompt. - `find_applicable_replacements` registers both keywords on `ZoneChange`→Battlefield; materiality stays `Writes { Count, Additive }`. - `keywords.rs`: Bloodthirst added to the instance-coexistence predicate so a granted instance survives beside an identical printed one (CR 702.54c). Also addresses Gemini's nit: the applier no longer rebuilds `ProposedEvent::ZoneChange` field-by-field — it mutates `enter_with_counters` in place via `if let ProposedEvent::ZoneChange { enter_with_counters, .. } = &mut event`, immune to new event fields. Rebased onto current main (resolved the `usize::MAX - 6` index collision with the new commander-return virtual candidate; `GRANTED_SUNBURST_INDEX` moved to MAX-7). Tests (integration, real cast/trigger/replacement pipeline): granted Bloodthirst 3 with an opponent damaged this turn → 3 +1/+1 counters; NO opponent damaged → 0 counters (condition revert-canary); end-to-end via Bloodlord's real "it gains bloodthirst 3" trigger → 3 counters. All 9 existing sunburst tests stay green. Full lib: 16959 passed. Integration: 3349 passed. Clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(engine): fold the granted-Sunburst stack-provenance guard into main's cast-payment-stamp authority Conflict resolution for the bring-current merge, isolated here so the judgement is reviewable on its own. This branch guarded `clear_post_collection_transients` so a spell still on the Stack kept `mana_spent_to_cast` / `colors_spent_to_cast`, because a GRANTED sunburst (Solar Array / Lux Artillery) enters AFTER the intervening cast-triggered grant resolves and that clear runs — wiping the color tally made it place zero counters (phase-rs#5337). While this branch sat, main fixed the same underlying seam independently for issue phase-rs#5943: `clear_post_collection_transients` now routes all five cast- payment stamps through `GameObject::clear_cast_payment_stamps()` and calls it ONLY for objects outside the Battlefield/Stack provenance zones. So `colors_spent_to_cast` — the field granted Sunburst actually reads, via `colors_spent_to_cast.distinct_colors()` — already survives on Stack objects under main's authority. The remaining unconditional line clears only the `mana_spent_to_cast` boolean, which main documents as a deliberate per-collection transient and which this feature does not read. Taking main's side therefore preserves this branch's behavior while keeping a single authority for payment-stamp lifetime, instead of layering a second, partly-redundant guard beside it. Verified, not assumed: with main's side taken, all 12 of this branch's own regressions pass — granted_sunburst_5337 (9) and granted_bloodthirst_5802 (3), including lux_artillery_grants_sunburst_two_colors_enters_with_two_p1p1 and solar_array_grants_sunburst_creature_three_colors_enters_with_three_p1p1, which are exactly the cases that fail if stack color provenance is wiped. Full `cargo test -p engine`: 21,496 passed, 0 failed. Co-authored-by: shin-core <153108882+shin-core@users.noreply.github.com> * fix(engine): branch granted Sunburst on printed types; cut the entry-gate keyword sweep Maintainer review fixups on top of phase-rs#5802. CR 702.44a rules defect: `granted_etb_replacement_definitions` branched the Sunburst counter type on `obj.card_types`, the LIVE layer result. The rule says sunburst applies "if this object is entering as a creature, IGNORING ANY TYPE-CHANGING EFFECTS that would affect it", and Layer-6 type effects do reach stack objects (`remote_type_layer_recipients`) — which is the zone this check runs in. Branch on `base_card_types` instead, the same printed `card_face.card_type` the printed synthesizer (`synthesize_sunburst`) uses, so the granted and printed paths agree. Adds the first fixture whose printed and live core types DIVERGE; all 12 existing fixtures set `base_card_types == card_types`, so this arm was previously unexercised and the defect was invisible. Perf on the `find_applicable_replacements` hot path (states are cloned and replayed constantly under AI search): the `ZoneChange`→Battlefield gate ran a full off-zone keyword sweep per keyword family, each one a whole-game continuous-effect collect plus ordering and per-effect filter evaluation, and the guard was ordered expensive-term-first. Test the cheap `already_applied` set lookup first, and resolve the live keyword list at most once (lazily) and thread it through the family helpers so one sweep serves every family. Also renames `Keyword::sums_across_instances` to `instances_must_coexist` — all three call sites implement "don't dedup identical instances" and none sums a parameter, matching the predicate's own doc — and repoints a dangling doc reference to `granted_sunburst_replacements`, a symbol that does not exist. Co-authored-by: shin-core <153108882+shin-core@users.noreply.github.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Matt Evans <matt.evans.dev@gmail.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix: prevent Fevered Visions controller damage * fix(PR-6563): route opponent arms through players::is_opponent; make relation a real alt() axis Maintainer fixup on top of the contributor's work, addressing the two non-blocking review findings: 1. `matches_player_scope` was internally inconsistent under CR 102.3: five arms tested raw `p.id != controller` while `OpponentOtherThanTriggering` in the same match block already routed through the canonical `players::is_opponent` authority. Under team play a teammate is neither the controller nor an opponent, and this is the effect-recipient enumeration, so the raw inequality admitted teammates as recipients of `Opponent`-scoped effects. All five now use `is_opponent`. 2. `parse_scoped_player_opponent_and_has_condition` hardcoded `PlayerFilter::Opponent` at the return while discarding the matched tag, so the documented "relation axis" did not exist. Extracted `parse_scoped_player_relation` returning `PlayerFilter` via `value()` inside `alt()`, mirroring `parse_condition_connector`, so a further relation is a one-line arm. Doc comment corrected to describe what the code does, and the fused `" and has "` connector's justification recorded inline. CR 102.2 / CR 102.3 grep-verified against docs/MagicCompRules.txt. Co-authored-by: invalidCards <842080+invalidCards@users.noreply.github.com> --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
phase-rs#6543) * feat(phase-ai): add poison-clock deck-feature axis + PoisonClockPolicy CR 104.3d makes ten poison counters a loss condition entirely independent of life total, tracked in a dedicated `Player.poison_counters` field. Nothing in the AI scored progress along it, so an infect/toxic deck's whole plan read as doing nothing: a proliferate taking an opponent from 9 to 10 poison scored the same as one taking them from 0 to 1. Feature (`features/poison.rs`) — three structural axes, no card names: * sources: Keyword::Toxic(N) / Infect / Poisonous(N) on a creature face (CR 702.164 / 702.90 / 702.70 — all three are combat-damage abilities, so a noncreature face carrying the keyword is rejected) * direct: Effect::GivePlayerCounter { counter_kind: Poison, target } whose target can resolve to an opponent (CR 122.1f). A Controller/SelfRef scope is rejected so a self-poison drawback is not read as a payoff * proliferate: Effect::Proliferate | ProliferateTarget (CR 701.34a) Commitment is a weighted sum, not a geometric mean: a dedicated Infect deck runs zero direct-poison spells and often zero proliferate and must still read as fully committed. Calibrated to Modern Infect (12 infect creatures + 2 proliferate over 37 nonland) > 0.85, with a superfriends anti-calibration (2 proliferate, no source) staying below the policy floor. Policy (`policies/poison.rs`) scores CastSpell + ActivateAbility by clock progress, critical band when the action reaches ten. The branch structure is driven by a rules detail: CR 701.34a defines proliferate over permanents and players that ALREADY have a counter, so proliferating with no poisoned opponent advances nothing and scores zero rather than a nudge — the inverse would push the AI to durdle before the clock has started. Every predicate is card-local or a plain u32 field read; no board-wide sweep, no affordability call, nothing on the documented inner-loop landmine list. `poison_clock_pressure` is registered UNTUNED: it is a CR 104.3d win-detector weight whose magnitude is load-bearing for correctness, not taste. Boundary with plus_one_counters: that axis already counts Proliferate as a +1/+1 enabler. The overlap is intentional and the axes stay independent — one proliferate card genuinely serves both decks, and a Hardened Scales deck scores high there while scoring ~0 here. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(phase-ai): address review on poison-clock axis (phase-rs#6543) Resolves the five blocking items from the maintainer review of phase-rs#6543. 1. Preserve pre-field PolicyPenalties artifacts. `poison_clock_pressure` now has `#[serde(default = "default_poison_clock_pressure")]`, sharing one default fn with the `Default` impl (matching every sibling field). `ai_tune` deserializes the `policy_penalties` section straight into the struct, so an artifact written before this field must still load — a compatibility test drives that with a pre-poison artifact. 2. Handle modal abilities at the actual choice seam. `ability_chain` gains a typed `AbilityScope` (Unconditional | Potential) and one mode-aware authority, `collect_scoped_effects`. Deck-time detection walks `Potential` (mode_abilities + else_ability included, CR 700.2 / 608.2c); the live policy walks `Unconditional`. A modal cast is no longer credited with a mode's poison — the selected branch is scored at `SelectModes` (routed via ModeChoice / AbilityModeChoice → ActivateAbility), where the mode is actually chosen (CR 601.2b). The engine's own `modal_spell_mode_ability_refs` is the shared mode-list authority. 3. Route the creature poison clock through combat. The policy now declares `DeclareAttackers` and scores an attack by the poison its sources convert (CR 702.90b infect = power, CR 702.164c toxic = summed N, CR 702.70a poisonous), summed per defending player, keyed on damage to a PLAYER only (a planeswalker/battle attack adds nothing, CR 506.4c). 4. Use only live opponents. `most_poisoned_opponent` and the new `live_opponent_poison` filter `is_eliminated` seats (CR 800.4), so a dead player's counters can't produce phantom lethal/progress pressure. 5. Complete opponent-target classification. `filter_can_hit_opponent` gains a sound `Not` arm via a `filter_matches_every_opponent` complement, so `Not { SelfRef }` / `Not { Typed(controller: You) }` read as opponent poison while `Not { Opponent }` does not. Also fixes a latent scoring-contract bug the new verdict tests surfaced: the sub-lethal delta (`pressure` scalar 6.0 × progress) escaped the preference band, tripping `score_in_band`'s debug assert and silently clamping in release. Scoring now routes through `PolicyVerdict::score` with the non-lethal ceiling held at `STRONG_MAX`, so only reaching ten lands critical. Adds direct `verdict()` coverage (lethal / no-counters-to-proliferate / progress-scaled), the modal seam, combat routing (incl. eliminated-seat and planeswalker cases), negated player scopes, and registry-level routing for the modal and combat decision kinds. cargo test -p phase-ai --lib — 1412 passed cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(phase-ai): cite CR 608.2c for the else_ability leg (maintainer fixup) `AbilityScope::Potential`'s doc cited CR 603.4 for the `else_ability` branch. CR 603.4 is the intervening-"if" clause rule for triggered abilities and describes no "Otherwise, ..." leg. The repo convention for `else_ability` is uniformly CR 608.2c (resolve instructions in the order written; later text may modify earlier text) — and `push_scoped_effects` in this same file already cites CR 608.2c for that exact branch, so the two annotations contradicted each other about one construct. Also drops the CR 506.4c parenthetical from `combat_contribution`: CR 506.4c governs a planeswalker or battle being REMOVED from combat, not an attack aimed at one. The load-bearing justification — CR 702.90b / CR 702.164c / CR 702.70a each keying on combat damage dealt to a *player* — is already cited in the same doc block and stands alone. Comment-only; no behavior change. All CR numbers grep-verified against docs/MagicCompRules.txt. Co-authored-by: minion1227 <romantymkiv1999@gmail.com> * test(phase-ai): registry-route every poison seam + tighten calibration (phase-rs#6543) Addresses matthewevans' second review. The five original blockers were confirmed resolved in production; this round closes the HIGH test-gap and the recommended follow-ups, plus two small correctness fixes he flagged. HIGH — the policy's primary seam was declared but never registry-tested. Every direct-poison test called `verdict()` directly, so `DecisionKind::Cast\ Spell` could be deleted from `decision_kinds()` with the whole suite green. Add `registry_routes_cast_spell_to_the_policy` (poison cast scores through the pipeline, inert cast routes-but-scores-nil), and companion routed coverage: * `registry_routes_activate_ability_to_the_policy` — exercises the `obj.abilities.get(index)` lookup, the per-index discrimination, and the out-of-range → neutral fallback. * `registry_routes_modal_spell_mode_choice_to_the_policy` — drives a real `WaitingFor::ModeChoice` + `PendingCast`, the sole production consumer of the new `modal_spell_mode_ability_refs` engine API. Calibration and predicate coverage: * Or/And fixtures for BOTH filter predicates, each flipping one branch's outcome under a `.any`/`.all` swap, so the opposite-quantifier inversion between `filter_can_hit_opponent` and `filter_matches_every_opponent` can no longer pass silently. * `CoreType::Land` calibration fixture — 20 lands must not move commitment; pins the nonland-denominator guard whose removal would silently rescale. * Two-sided calibration bounds (Modern Infect 0.904 ± 0.01, superfriends 0.061 ± 0.005) replacing one-sided asserts that a saturating source weight could slip past. * `MIN_CLOCK_PROGRESS` pinned by a flat-plateau equality (0 and 1 poison score identically); deleting the floor splits them. Correctness (maintainer-flagged, non-blocking): * Combat lethal is capped at `STRONG_MAX`, strictly below the critical band a guaranteed direct poison reaches — CR 509.1a: a declared attack can be blocked or prevented, so a poison swing is a strong play, not a booked win. `score_clock` gains a per-branch `ceiling`. * `source_names` had no consumer — dropped the field, its population, and its two guarding tests rather than leave dead data. * CR nits: `608.2c` (not 603.4) for the `else_ability` branch at ability_chain.rs; dropped the off-point `506.4c` from combat_contribution. Deliberately deferred (per review): the multiplayer `SelectTarget` refinement (DirectPoison scores vs the most-poisoned seat but doesn't yet own target choice in 3+ player games) and the inherited `CR 109.3` citation, which the reviewer noted belongs on its own repo-wide commit. cargo test -p phase-ai --lib — 1420 passed (poison suite 50 → 58) cargo clippy -p phase-ai -p engine --all-targets --features proptest — clean Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * docs(phase-ai): drop the wrong CR 104.3d cite on the land-exclusion test (phase-rs#6543) CR 104.3d is the ten-poison-counters loss rule; it says nothing about excluding lands from a commitment denominator. That exclusion is a deck-modelling choice in `detect`, not a rules requirement, so it carries no CR citation at all. Per CLAUDE.md a wrong CR number is worse than none — it creates false confidence that the code was verified against that rule. Comment-only; the guard and its assertions are unchanged. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…etection (phase-rs#6579) Restrict AI-CONTRIBUTOR to Frontier tier only. The Standard row (claude-sonnet-*, claude-haiku-*, gpt-5-4 and below, codex-5-4 and below) is removed rather than trimmed, and every passage that depended on a two-tier world moves with it: the "assume Standard" fallback becomes an abort, "Pre-PR gates (all tiers)" drops the qualifier, the Tier: block loses its Standard alternative, and the Appendix B copy-paste prompt now tells a sub-Frontier runtime to stop instead of "proceed even if your runtime is below that". That prompt is the one contributors actually paste, so leaving it stale would have kept inviting the PRs the gate is meant to prevent. Model tier is a hard gate; the agentic harness is not. A Co-authored-by: Cursor trailer raises scrutiny to the full evidence bar and forfeits light-touch, but does not by itself close a PR -- the failure mode is CI-as-correction-loop, not the tool. The behavioural tells are what earn a standing change: reverting a fix to push a diagnostic commit so CI prints a value, or deleting a passing assertion to turn a job green. pr-review-loop gains the matching detection recipe (check both the canonical Model: line and commit trailers, and read what matched -- a bare `rg cursor` hits WordCursor), and its skip route now closes on sight, dequeues first, and follows alias_of across an account's alts. Also fixes a defect in the compile-error attribution recipe shipped in 6576: `git cat-file -e "$sha:<path>"` is corrupted before git sees it here, swallowing the ":" plus the path's first character, so paired with `2>/dev/null || echo "file absent"` it silently fabricates evidence of maintainer-caused staleness for any crates/ path. Uses git ls-tree, which takes rev and path as separate arguments. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…erlay, and surface the WASM fallback (phase-rs#6577) * fix(shell): dial phase-server's /ws route from the native bridge The pinned Tauri bridge dialed `ws://127.0.0.1:{port}` while phase-server serves its socket at `/ws`. The root path answers 404, the upgrade fails, and the client's catch silently falls back to WASM — so native AI has never actually connected in a shipped shell. Verified end to end against a live local server: `GET /` upgrades 404 for every origin, `GET /ws` with a first-party Origin returns 101 + ServerHello, and a hand-driven ClientHello/CreateGameWithSettings raised game_sessions 0 -> 1. Also make the command wrapper the single authority for the terminal progress phase. `ensure_native_engine_sync` returns early on both the healthy-in-process and adopted-record paths, so emitting Ready inside it left those runs ending on a non-terminal phase and any listener waiting for one waiting forever. Regenerates gen/schemas for the native-engine progress permission, which had been stale since the command was added. * fix(client): unpin the provisioning overlay and surface the WASM fallback The overlay's lifetime was driven by receiving a terminal progress phase. The installed shell (shell-v1.0.1) fails without emitting one, so the overlay stayed up over the running game for the rest of the session. Drive visibility from the `ensureNativeEngine` call's own lifetime instead: the shell is a separately-versioned binary whose events are advisory, so anything that must eventually stop belongs on this side. A 400ms reveal delay keeps a reused engine from flashing the overlay. An abandoned native attempt also painted GamePage's terminal "connection lost" banner over a healthy WASM game that played on underneath, because the adapter's error events were forwarded before the session was live. Make the fallback visible rather than silent: a one-shot toast when it happens, and an EngineModeBadge beside the game-menu button as the standing signal. The badge distinguishes a genuine fallback from a game that never asked for the native engine (draft matches, a chosen first player, resuming a WASM save), which would otherwise read as a failure on a machine whose native engine works fine. The toast surface itself was mounted only for online games, so a solo game's toast rendered nowhere; GamePage now renders it in every mode and withholds only the Retry action, which has no meaning without a server. * fix(client): treat every non-null engine fallback reason as a failure The default arm caught null and unrecognized reasons alike, so a reason added to nativeFallbackReason later would have been labelled "the native engine wasn't used for it" — a false all-clear for an attempt that actually failed, which is the same false claim this dispatch exists to prevent, pointed the other way. Only null means the game never asked. Reported by CodeRabbit on phase-rs#6577. * fix(client): type the progress-replay mock to its real signature `vi.fn(async () => null)` infers `Promise<null>`, so every `mockResolvedValueOnce({ phase: ... })` in the file was an error under `tsc -b`. Annotate the mock with the real `Promise<NativeEngineProgress | null>` return type. Missed locally because bare `tsc --noEmit` does not build the test project; `pnpm run type-check` (what CI runs) does. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ard" (phase-rs#6486) (phase-rs#6561) * fix(engine): keep a "return to hand" card as the referent for "that card" (phase-rs#6486) Volcanic Vision ("Return target instant or sorcery card from your graveyard to your hand. Volcanic Vision deals damage equal to that card's mana value to each creature your opponents control. Exile Volcanic Vision.") returned the card but dealt no damage. The spell parses correctly — the damage sub-effect's amount is the Demonstrative "that card's mana value". That resolves against the earlier-instruction referent (`effect_context_object`), which `parent_referent_context_from_events` derives from the parent effect's `ZoneChanged` events via `moved_object_context_from_events`. That helper only captured moves to a PUBLIC zone, so the graveyard->HAND return bound no referent, "that card's mana value" resolved to 0, and every opponent creature took 0 damage. CR 608.2c: a later instruction may refer to an object an earlier instruction returned to hand — its identity was established in the public source zone (graveyard), and the reference reads the move-time last-known mana value, so the hidden destination is immaterial. Capture the moved referent for a move to hand when the SOURCE zone is public. A move from a hidden source (a draw, library -> hand) establishes no such referent and is excluded; library destinations stay excluded entirely (a shuffle loses identity). Verified: cargo test -p engine --lib -> 17591 passed, 0 failed. New card-level cast test drives Volcanic Vision through the real pipeline: with a mana-value-3 instant in the graveyard, each opponent creature takes 3 damage, the card is in hand, and Volcanic Vision is exiled. * fix(PR-6561): correct CR citation and pin the widened move predicate The referent widening is rules-correct, but its annotation cited CR 608.2c (instruction ordering / later-text-modifies-earlier-text), which does not speak to source-zone publicity or last known information. Cite the two rules that actually carry the clause, both grep-verified against docs/MagicCompRules.txt: CR 400.7j findability on a move to a PUBLIC zone (the pre-existing arm) CR 608.2h "... or if the effect has moved it from a public zone to a hidden zone, the effect uses the object's last known information" (the new public -> Hand arm) Add three helper-level tests on parent_referent_context_from_events so the widened predicate's boundaries are pinned: - Library -> Hand (a draw) binds no referent. After the widening, is_public_zone's exclusion of Library is the only thing keeping every draw in the game from binding one, and nothing tested it. - Graveyard -> Hand binds the referent and carries the move-time mana value (the Volcanic Vision case, at helper level). - Two qualifying moves in one span bind nothing — the CR 608.2k singular guard now has a wider surface to trip on. Also name the dominant newly-captured population (ordinary Battlefield -> Hand bounce) in the predicate comment so the next reader sizes the blast radius correctly. Co-authored-by: philluiz2323 <philluiz2323@gmail.com> --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…issolved merge groups (phase-rs#6581) The Frontier-only tier rule landed in phase-rs#6579 with no cutoff clause, so read literally it closes PRs opened weeks earlier that declared Sonnet or Composer accurately under the then-current Standard tier. That inverts the incentive the disclosure rule exists to create: the only PRs the rule can find are the ones whose authors filled the field in honestly. Scope it to PRs opened on or after 2026-07-24 and cross-reference the cutoff from the Tier-line section. Add a janitor for dissolved merge groups. Every queue reformation deletes the gh-readonly-queue refs and mints new ones, and ci.yml's concurrency group is keyed on github.ref -- unique per synthesized ref -- so cancel-in-progress can never match the superseded runs. One evening accumulated 22 such runs (~198 queued jobs) against a 20-job concurrency cap. This cannot be fixed in the concurrency key: GitHub's expression language has no regex or split, so a concurrency.group cannot extract the PR number from the ref, and keying on the base branch would cancel live speculative groups -- which the queue reads as a failed check and evicts the entry for. Cancelling runs whose ref no longer resolves is the safe equivalent, and only queued/in_progress runs are considered so a completed run whose ref was reaped is never touched. Paired with a ruleset change (applied out-of-band): max_entries_to_build 5 -> 2, since 5 speculative groups x 9 jobs requests 45 jobs against a 20-job cap; and check_response_timeout_minutes 60 -> 120, which evicted an approved PR ten seconds past deadline during a hosted-runner stall whose CI then passed. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…se-rs#6013) (phase-rs#6539) * fix(engine): Metamorphic Alteration as-enters choose + host copy (phase-rs#6013) Latch the chosen creature's copiable values on the Aura and install a Layer-1 CopyValues TCE on the enchanted host so the second choice works without BecomeCopying the Aura. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): cover EffectKind::ChoosePermanent in trigger index Exhaustive match in keys_from_effect_kind must classify the new Metamorphic Alteration kind as a no-op EffectResolved key (same as Choose/BecomeCopy). Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): stop using GameScenario::state_mut in Metamorphic tests as_enchantment now strips Instant/Sorcery like other permanent converters, and token-donor setup mutates via GameRunner after build. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): attach Aura before Metamorphic copy choice applies Mid-entry CopyTargetChoice pauses delivery before CR 608.3c attach; stash PendingSpellResolution on that pause and apply it when PersistChosenAttribute answers. Also bump the ability-scan census count for ChoosePermanent. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): route Metamorphic as-enters choose through replacement parse is_as_enters_choose_pattern only accepted named choices, so choose-a-creature never reached ChoosePermanent. Parse the choose suffix with the shared target descriptor, establish the Aura host from spell targets or CR 303.4f attach, and cover non-spell entry. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): gate Metamorphic ChoosePermanent on CopyChosen link Bare as-enters permanent choices stay unsupported until a CopyChosenHost document relation proves the companion static; stage the non-spell fixture via from_oracle_text so live and base definitions compile. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): compile CopyChosenHost apply against Box execute is_some_and needs a closure over &Box, description is Option, and assignment targets the boxed AbilityDefinition. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): narrow Metamorphic choose to CopyChosenHost only Stop claiming Moved for bare as-enters permanent choices so Dauntless Bodyguard and Scheming Fence keep unsupported ability shape; inject ChoosePermanent only when a CopyChosen companion proves the relation. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): drop unused as-enters permanent phrase helper Classifier no longer routes object chooses through replacement parse; only as_enters_choose_permanent_filter remains for CopyChosenHost. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): use Definitions::iter_all in Metamorphic fixture assert Live replacement_definitions is Definitions<T>, not a slice. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): use public iter_unchecked in Metamorphic fixture iter_all is crate-private; integration tests need the public API. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): honor mid-entry choice from spell CallerEpilogue drain CallerEpilogue discarded apply_pending_post_replacement_effect's CopyTargetChoice, so Metamorphic spell casts attached without copying. Surface the prompt, stash PendingSpellResolution, and return like the delivery NeedsChoice arm. Mark ChoosePermanent order-independent. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): apply CallerEpilogue WaitingFor without burying continuations Honor mid-entry prompts from the spell drain (Metamorphic CopyTargetChoice) but continue the Aura-attach epilogue instead of early-returning with PendingSpellResolution on top — that buried Tribute/Siege AbilityContinuations and broke resume. Host comes from attached_to after epilogue, matching dig. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): drain CallerEpilogue post-effects after Aura attach PersistChosenAttribute needs attached_to before CopyTargetChoice is answered. Move the honor-WaitingFor drain to after CR 608.3c attach so spell-path Metamorphic matches dig order without burying Tribute AbilityContinuations under SpellResolution. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): recover Aura host attach for Metamorphic spell path Flatten stack enchant targets, fall back to Enchant-filter attach when cast targets are missing, and pin spell fixtures' Enchant to the host so PersistChosenAttribute always sees attached_to before the copy installs. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): carry Aura spell target through Metamorphic CopyTargetChoice CR 608.3c / CR 303.4a: stash PendingSpellResolution.spell_targets on the CallerEpilogue CopyTargetChoice pause instead of recovering the host via an ambiguous Enchant-filter consult. Fixture installs Enchant via MTGJSON hint; add a multi-host regression without SpecificObject pinning. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): resolve cast-link locals in CopyTargetChoice stash CallerEpilogue CopyTargetChoice sits outside the permanent replace_event scope, so re-read cast_timing_permission and convoked_creatures from the battlefield object when building PendingSpellResolution. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): clear Metamorphic CopyTargetChoice before next cast Attach the Aura before the CallerEpilogue ChoosePermanent drain, keep PendingSpellResolution for spell_targets without early-returning, and retire the paused drain before deferred-entry replay so a second cast is not blocked on a leftover CopyTargetChoice. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): stop echoing answered Metamorphic CopyTargetChoice PersistChosenAttribute left state.waiting_for as the inbound CopyTargetChoice, so the completion tail re-returned it and the cast driver re-answered until its loop cap — sequential casts stayed blocked while single-cast tests still passed. Co-authored-by: Cursor <cursoragent@cursor.com> * refactor(engine): Metamorphic review fixups — drop persist, share spell stash Drop the unread ChoosePermanentPersist discriminator (CopyTargetPurpose + CopyChosenHost already gate behavior), extract pending_spell_resolution_snapshot for the three stack stash sites, document why the PersistChosenAttribute completion tail omits BecomeCopy batch-capture steps, and restore CopyTargetChoice harness break uniformity. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…ff evidence (phase-rs#6583) The `skip` route was changed on 2026-07-24 to mean "close on sight" as part of a contributor ban. That ban was reversed: the trigger was `Co-Authored-By: Claude Haiku 4.5` trailers, but `claude-haiku-4-5` was named in the accepted Standard row of the tier table live at the time, and the Frontier-only rule landed at 2026-07-24T02:03:45Z — after the newest affected PR was opened at 2026-07-23T22:09:07Z. The enforcement was retroactive. The close-on-sight text outlived the ban in this tracked skill, so a future sweep would have re-applied it to every `skip` login — including `dale053`, which was set for an unrelated reason and never carried that action. - `skip` returns to decline-to-review; it does not authorize closing. - Drop the "a prior approval is not evidence" paragraph. It was written to justify dequeuing two PRs this maintainer had approved at unchanged heads, and it generalizes into distrusting our own review record. - Add the low-effort close rule, scoped to any author and to diff-visible evidence only, with the requirement to re-verify that evidence firsthand rather than inheriting an earlier review's summary. - Add the policy-date rule: git log the policy doc and diff the version live at the PR's createdAt before enforcing against it. - Record that a Co-Authored-By trailer names the model for one commit, so trailers are read against the implementing commits; a mid-run fallback leaves sub-floor trailers on a compliant change. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…xact model (phase-rs#6584) The Frontier-only table used family wildcards (`claude-sonnet-*`, `claude-haiku-*`), which excluded `claude-sonnet-5` — a current-generation frontier model — while the accepted row still admitted `claude-opus-4-7`. State the floor per vendor by exact model instead: Anthropic claude-opus-4-8+, claude-sonnet-5+ OpenAI gpt-5-5+ Codex codex-5-5+ and name the exclusions explicitly (opus-4-7 and below, sonnet-4-6 and below, all haiku incl. haiku-4-5, all composer, gpt-5-4/5-3 and below, codex-5-4 and below) so no wildcard reading can misclassify a model. Also document how commit evidence is read. A Co-Authored-By trailer is weighed against the commits carrying the implementation: a session that falls back to a sub-Frontier model part-way through leaves sub-Frontier trailers without the change having been generated below the floor, and that is a declaration to confirm rather than dishonesty. The account-level problem is a Model:/Tier: line that contradicts the trailers in the direction that clears the gate. Extend the pre-cutoff grandfather clause to name Haiku, since claude-haiku-4-5 sat in the Standard row until 2026-07-24T02:03:45Z. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…se-server (phase-rs#6597) The desktop shell is a GUI-subsystem app but phase-server.exe is console-subsystem, so Windows allocated a visible console window for the child on every launch. Apply the CREATE_NO_WINDOW creation flag on all three Windows console spawns: the phase-server launch (the persistent window), plus the tasklist stale-process probe and taskkill cleanup (each of which briefly flashed its own console). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…esktop (phase-rs#6599) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…e-rs#6600) Battlefield card previews never appeared over remote desktop (RDP) and flickered locally with a non-zero hover delay. Both timer-based dismiss checks — the 300ms poll (usePreviewDismiss) and the 50ms delayed-clear (uiStore.inspectObject) — tested "is the cursor still over a card?" by sampling document.elementFromPoint() against a JS-tracked pointer that only updates on pointermove. Over sparse/coalesced RDP pointer streams that coordinate goes stale a few px off the card while the OS cursor is genuinely on it, so the check false-negatived and dismissed (or cancelled the pending show for) a live preview. Battlefield cards were uniquely affected: their hover element carries a Framer-Motion layoutId that emits spurious mouseleave events routed through the delayed-clear; hand cards have no layoutId and never triggered the stale-pointer check. Switch both timer-based checks to the browser's authoritative [data-card-hover]:hover element — the engine's continuous hit-test, correct with zero pointermove events — and remove the now-dead lastPointer tracking. The pointerdown-outside handler keeps elementFromPoint (it reads fresh event coordinates). Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* docs: require complete AI contributor PR template * docs: clarify PR template metadata --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* ci(deploy): cache draft pools and bank caches when data is complete The card-data job recompiled engine + draft-core on every run to produce draft-pools.json — 5m37s of cargo for a ~2s JSON transform, because draft-pool-gen cannot take --features cli and so never reuses the units gen-card-data.sh built. Cache the output instead, keyed on the MTGJSON data version (via the shared mtgjson-cache-key action, so the two roll together and /clear-caches mtgjson sweeps both) plus a hash of the sources that define it. Also convert every cache in the job from the combined actions/cache to restore + an explicit save at the point its data is complete. The combined action registers its save as a post step declared post-if: success(), so a job that dies downstream saves nothing. On 2026-07-24 that turned a routine MTGJSON publish into a self-sustaining outage: the key rolled, the run paid ~28m of cold downloads, hit the 30m job timeout mid-R2-upload, saved nothing, and every following run recomputed the same missing key and paid it again (run 30114030288). Raise that ceiling to 50m for the same reason — the budget has to clear the cold path or the cache can never be seeded at all. The pool save is skipped when any set download failed: fetch-draft-sets.sh treats a failed set as non-fatal, and freezing a short pool under a key that only rolls on the next MTGJSON publish would defeat the existing self-heal (sets/ is file-existence-gated, so the next run fetches only what is still missing). * ci: reclaim Actions caches whose scope can never restore them The repo sat at 9.93GB against the hard 10GB quota across 207 entries, with 141 of those (3.4GB) written and never restored even once. The waste is not old or large entries — it is entries whose ref scope is dead. A cache is restorable only from its own ref or the default branch, so caches on vanished gh-readonly-queue branches (58 of 58 never read), on closed PRs, and on one-shot tag refs are unreachable by construction. A dry run over the live inventory reclaims 2.84GB immediately. Deliberately not generational pruning. That was measured first and does not pay: every rust family on main already carries exactly 2 generations, so keep-newest-2 reclaims 0.13GB, and keep-1 would delete the fallback Swatinem's rust-cache restores through restore-keys. Main's caches are healthy (42 of 46 re-read) and this never touches them. Mirrors merge-queue-janitor.yml: ref-absence is a scope test, not a liveness test. Uses pull_request_target for the PR-close trigger because fork-originated pull_request events get a read-only token regardless of the permissions block; safe here since nothing checks out PR code. * ci(card-data): refresh embedded catalogs daily, not weekly MTGJSON publishes roughly daily (5.3.0+20260723 -> +20260724 on consecutive days) while this ran Mondays, so for six days out of seven the committed known-tokens.toml was stale. That is not cosmetic: build.rs embeds that file, so when gen-card-data.sh regenerates it mid-run the deploy has to compile the engine a SECOND time to embed the new catalog — 5m35s on top of the first 5m39s compile, measured on run 30114030288, on a job that was timing out at 30m. Safe by construction: the vintage write-gate means a run against unchanged MTGJSON rewrites nothing and opens no PR. * ci(cache-janitor): fail closed on missing evidence, scope PR-close runs Addresses review findings on phase-rs#6603. Three of these are guards that could not fire on the input they exist to catch: * The draft-pool key validated its source paths with a single combined emptiness check. `crates/draft-core` alone matches 13 tracked files, so renaming either single-file engine path left the output non-empty, passed the guard, and silently dropped that file from the hash — after which the key would never roll on its content again. Validate each path independently. * The live merge-queue lookup used `|| : > live_queue.txt`, which cannot distinguish "no queue refs exist" (200 with `[]`) from "the API call failed". A blip left the file empty, marked every queue ref orphaned, and swept the caches of in-flight entries — the exact fail-open the PR arm already refuses. Abort instead, and skip the lookup entirely on PR-close runs so they cannot fail on a scope they do not touch. * Every delete failure printed "(already gone)". Only a 404 means that; a 403 from a missing `actions: write` or a secondary rate-limit would have reported a clean sweep having reclaimed nothing. Classify the error and emit a warning annotation for anything else. Also scope PR-close runs to the PR that closed, which the env comment already claimed but only the `refs/pull/*` arm implemented, and fix the summary line's `${DRY_RUN:+...}` — DRY_RUN is the string "false" on real runs, not empty, so every sweep reported "would have deleted". Verified by dry-running the sweep against the live cache inventory: the scheduled path reclaims 115 entries / 2.84 GB unchanged, and a PR-close run for a closed cache-holding PR touches only that PR (0 queue, 0 tag, 0 other PRs, live-ref lookup skipped). --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…pty hand (Lion's Eye Diamond) (phase-rs#6596) * fix(engine): pay a zero-card "discard your hand" cost by doing nothing (Lion's Eye Diamond) Lion's Eye Diamond ("Discard your hand, Sacrifice this artifact: Add three mana of any one color") could not be activated with an empty hand: the cost surfacing paths emitted a dead WaitingFor::PayCost { Discard, count: 0, choices: [] } and stalled, even though legal_actions/cost_payability correctly offered the ability (0 >= 0). Discarding your hand with no cards is a cost paid by doing nothing (CR 601.2h; CR 701.9a moves cards only when cards exist). Class-level fix via a single authority: a resolved-count-0 non-self FromHand discard leg is treated as paid and the next unpaid leg (the sacrifice) is surfaced instead of a dead prompt. New resolve_non_self_discard_requirement in game/casting.rs (tri-state: auto-paid None / unpayable Err / interactive Some) is applied at both activation surfacing sites; discard_cost_choice in game/mana_abilities.rs skips count 0 mirroring its exile-cost sibling. Covers Lion's Eye Diamond, Diamond Lion (mana-ability path) and Bomat Courier (activated-ability path). Fixed Discard{1} on an empty hand stays unpayable (guard fires only on resolved count 0). Pure runtime change; zero parse-diff. Closes phase-rs#6494 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(engine): make find_non_self_discard the sole FromHand discard-cost authority (phase-rs#6494 review) Address matthewevans review on phase-rs#6596: consolidate the duplicate mana-ability discard detector (find_non_self_discard_cost) into casting::find_non_self_discard (now returns the CardSelectionMode) and route discard_cost_choice through the shared resolve_non_self_discard_requirement, so the zero-count + payability rules live in one authority. The mana path keeps only its Chosen-selection policy as an explicit call-site gate. Behavior-preserving; adds a shared-seam test. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: michiot05 <281539540+michiot05@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
…s#5981) (phase-rs#6570) * fix(engine): latch Gift recipient instead of next_player (phase-rs#5981) Promise Gift as AdditionalCostOrigin::Gift with optional ChooseGiftRecipient when multiple opponents exist, stamp gift_recipient through cast finalize, and fix Peerless nested Instead target slots via set_context_recursive. Co-authored-by: Cursor <cursoragent@cursor.com> * test/fix(ui): harden Gift recipient coverage and i18n (phase-rs#5981) Add a non-Peerless 3p Gift delivery test that chooses P2 (not seat-next), and route Gift optional-cost chrome through t() using optionalCost.gift keys. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(i18n): mirror Gift keys across locales and cover Gift modal (phase-rs#5981) Add giftRecipient/optionalCost.gift keys to de/fr/es/it/pl/pt (en fallback) and assert Gift OptionalCostChoice copy via OptionalCostModal test. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): initialize GameObject.gift_recipient in new() (phase-rs#5981) CI failed E0063 on the GameObject struct literal and partition destructure after adding the Gift recipient stamp field. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine/ui): address Gift review polish (phase-rs#5981) Drop the zero Gift sentinel uniformly on both recipient paths, render engine candidate order without client re-sort, and translate Gift UI strings in all locales. Co-authored-by: Cursor <cursoragent@cursor.com> * test(engine): fix Gift Draw fixture keyword hints (phase-rs#5981) Scenario oracle inference cannot FromStr a "Gift a card (reminder…)" line. Pass the MTGJSON-style "Gift" hint so the non-Peerless 3p delivery test actually exercises ChooseGiftRecipient + latched recipient draw. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(phase-ai): fill OptionalCostChoice origin/gift_kind fields (phase-rs#5981) The WaitingFor::OptionalCostChoice literal in the multikicker search fixture still used the pre-Gift shape; add Kicker origin and gift_kind: None so phase-ai compiles against the extended variant. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): preserve announcing_opponent when stamping Gift cast context Full set_context_recursive in begin_deferred_target_selection wiped per-link announcing_opponent stamps, so Volcanic Offering never advanced past choice index 1. Also bump WaitingFor variant census for ChooseGiftRecipient. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ui): regenerate interaction bindings for ChooseGiftRecipient CI Interaction bindings check failed; export chooseGiftRecipient on InteractionActionCode. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): route Gift kind lookup through keywords authority gift_kind_for_object used a raw obj.keywords.iter query, which Gate A rejects for off-zone objects. Add effective_gift_kind and call it from casting_costs. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): clear gift_recipient on battlefield exit/entry (CR 400.7) Gift recipient is cast-link provenance; blinking or reanimating must not keep a prior incarnation's selection. Restore only via CastLinkSnapshot on Stack→Battlefield. Add unit + blink/reanimate regressions. Co-authored-by: Cursor <cursoragent@cursor.com> * test(PR-6570): cover Gift recipient fallback * fix(PR-6570): reuse living-player authority --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…er per keyword (phase-rs#6533) * fix(parser): distribute Mutable Pupa's perpetual keyword-mirror trigger per keyword (phase-rs#6321) "Whenever another creature you control enters, this creature perpetually gains flying if that creature has flying. The same is true for first strike, ..." was entirely Effect::Unimplemented -- the same "the same is true for K1, K2, ..." per-item list-collapse bug family PR phase-rs#6168 fixed for continuous exiled-object keyword grants, but for a triggered, resolve-once perpetual grant gated on the entering creature instead. Extends strip_suffix_conditional's trailing-condition stripper with a trigger-scoped "that creature/permanent has <keyword>" arm (CR 115.1: gated on ctx.in_trigger so it never intercepts the pre-existing, unrelated "instead" override class), reusing the existing AbilityCondition::ZoneChangeObjectMatchesFilter and Effect::ApplyPerpetual building blocks -- no new runtime-facing enum variants. Adds a new ReplicateKind::PerpetualKeywordGrant lowering template (parser-internal IR, alongside the existing StaticGrant/CounterPlacement leaves) so each listed keyword becomes an independent SequentialSibling grant. The independent-branch chain needed a real fix in resolve_chain_body: a SequentialSibling with its own condition was only re-checked past a failed preceding sibling for two narrow existing cases, so the keyword list would silently collapse at the first false gate. Added a construction-scoped SiblingCondition marker (Dependent default / ReplicatedOrBranch, stamped only by the two per-keyword replication helpers) rather than matching on condition type, since an earlier type-based approach would have regressed Thieving Skydiver's genuinely dependent "if that artifact is an Equipment" continuation. The identical live bug already shipping in Kathril, Aspect Warper's counter-placement chain is fixed by the same marker. CR 608.2c (instructions resolve in the order written); no CR entry for "perpetually" (digital-only Alchemy templating). Tests: parser-shape assertions for the antecedent clause and the full 12-node independent chain, an Odric non-regression (stays GenericEffect, not routed through the new perpetual path), a Thieving Skydiver field-level non-regression (never stamped ReplicatedOrBranch), and registered runtime integration tests driving parse -> resolve_chain_body -> perpetual grant for Mutable Pupa (single keyword, multi-keyword accumulation) and Kathril (reaches a matching counter and the unconditional tail past a false earlier gate). * fix(PR-6533): cover perpetual keyword grant role * fix(PR-6533): address parser review findings * fix(PR-6533): dereference parser test effect * fix(PR-6533): select Kathril counter recipient * test(PR-6533): fund Kathril regression correctly * fix(test): use self-ref counter recipient in Kathril regression test The Kathril regression test (PR phase-rs#6533) was failing: `trample is in the graveyard ⇒ exactly one trample counter is placed: left: 0 right: 1`. Two prior fix attempts (funding Kathril's exact {2}{W}{B}{G} cost, declaring an explicit `.target_object(kathril)`) left the failure unchanged, pointing at the "any creature you control" recipient's targeting resolution rather than the SiblingCondition mechanism under test. Reworded the fixture to place every counter on Kathril itself ("on Kathril", self-ref -> TargetFilter::SelfRef) instead of "any creature you control" (TargetFilter::Any, a known parser fallback whose runtime targeting path is a separate, pre-existing concern). This is the exact self-targeting phrasing the fixture's own unconditional tail clause ("put a +1/+1 counter on Kathril") already uses, and both go through `normalize_card_name_refs` the same way "When Kathril enters" does -- so it reuses an already-proven path rather than a new one. The test's actual subject (does K1..Kn still resolve independently past a false K0 gate) is unaffected by who the recipient is. * fix(parser): recognize "any <type>" as a real typed target, not a fallback The Kathril regression test was failing because "put a flying counter on any creature you control" parsed to the degenerate TargetFilter::Any fallback rather than a real typed filter -- the actual bug the maintainer flagged after correctly rejecting a prior attempt that sidestepped this by rewording the fixture to self-targeting instead of fixing it. Root cause: parse_type_phrase_with_ctx (oracle_target.rs) strips leading "a "/"an " before a recognized type word, but had no handling for "any ". "any creature you control" therefore never reached the type-word grammar and fell through every dispatch arm to the TargetFilter::Any fallback + TargetFallback diagnostic in parse_target_with_syntax. Fix: strip "any " the same way "a "/"an " are stripped -- guarded on the same starts_with_type_phrase_lead/starts_with_commander_word check, and consolidated all three into one alt() combinator rather than duplicating the guard a third time. CR 115.10a + CR 115.1d: an object is only a target if the text uses the literal word "target", so "any creature you control" is correctly left untargeted (no target_choice_timing change) -- this only fixes the filter shape, letting the existing Stack-time slot collection and chain-target-propagation machinery (both unmodified, already used by hundreds of other cards) do the rest. Restored the Kathril test to its real Oracle text and target_object() declaration (both were removed in a prior commit that avoided the bug instead of fixing it). Documented, rather than silently left, one known follow-up: per CR 608.2d an untargeted "any creature you control" choice should be re-offered independently at each replicated instruction's own resolution, not chosen once and reused chain-wide -- fixing that requires widening target_choice_timing_for_clause's PutCounter classification beyond this PR's list-collapse scope, so it's called out explicitly in the test rather than silently left unstated. Two pre-existing snapshot tests (oracle_ir/snapshot_tests.rs's kathril_aspect_warper, whose stored .snap files serialize the old "type":"Any" target shape for all 11 nodes) will need `cargo insta test` + accept once run in an environment with a working toolchain -- this sandbox has none (confirmed: even `cargo check` fails at the build-script link stage), so they're flagged rather than hand-edited blind. * fix(engine): give each untargeted PutCounter instruction its own CR 608.2d choice Kathril's per-keyword counter placements ("put a flying counter on any creature you control if...") were sharing ONE upfront recipient choice across every independent instruction in the replicated chain, instead of each offering its own choice at its own resolution. The real card's ruling is explicit: "The counters don't need to all be put on the same creature." No existing mechanism did this correctly. PutCounter's only prior Resolution-timing usage was scoped to Equipped/Enchanted hosts (deterministic, no real choice); MultiplyCounter's is a population-wide "each matching creature" enumeration, not a single-recipient pick. Built one from scratch, reusing existing building blocks: - oracle_effect/lower.rs: target_choice_timing_for_clause's PutCounter guard widened from contains_source_attachment_host() alone to !is_context_ref() (an existing TargetFilter method covering every deterministic, no-choice-needed shape) -- matches the pattern MultiplyCounter's own arm already uses. - game/effects/mod.rs: a new interactive recipient prompt in resolve_chain_body, positioned right before effect execution. Mirrors the existing filter_chosen_player_index 0/1/N pattern (a single legal recipient auto-binds; zero is a no-op) and reuses the ChooseFromZoneChoice + parked-continuation machinery already proven for PutCounter continuations (Bolster's is_partition gate names this exact effect type). Candidates are enumerated via a plain matches_target_filter scan, not the targeting-legality path -- per CR 115.10a this is a choice, not a target, so hexproof/shroud/protection must not restrict it (mirrors Bolster's own "chooses, not targets" precedent). - game/effects/mod.rs: extracted should_propagate_parent_targets as the single authority for every targets-inheritance site in the file (13 total) -- a Resolution-timed sub must not inherit an already-chosen target, or the whole mechanism silently collapses back to one shared pick. Source-attachment-host targets are excluded from the new prompt entirely; they keep resolving deterministically through their existing path in counters.rs. - game/scenario.rs: drive_resolution gained a ChooseFromZoneChoice arm so tests can declare which of several legal recipients a given instruction picks. Two rounds of independent review on this mechanism (given its blast radius -- shared resolution code, and a widened classification touching every untargeted PutCounter card, not just Kathril) each found and fixed real issues: a missed second propagation site that would have silently reproduced the exact bug, a targeting-legality correctness gap, and two CR mis-citations. New test: kathril_offers_each_matching_counter_its_own_independent_recipient, proving two independent instructions (flying, trample) pick two different declared creatures rather than collapsing onto one. Two pre-existing snapshot files (oracle_ir/snapshot_tests.rs's kathril_aspect_warper, whose .snap files still serialize the old "type":"Any" target shape) will need `cargo insta test` + accept from an environment with a working toolchain -- this sandbox has none. * fix(engine): pass ability by reference to should_propagate_parent_targets resolve_optional_effect_decision's ability parameter is an owned ResolvedAbility (mut ability: ResolvedAbility), not a reference like every other call site of the new should_propagate_parent_targets helper -- CI caught the resulting E0308 type mismatch that this sandbox's missing compiler couldn't. * fix(parser): compose the "any" quantifier strip through other/another The "a"/"an"/"any" indefinite-quantifier strip in parse_type_phrase_with_ctx only checked starts_with_type_phrase_lead/starts_with_commander_word on the immediate remainder after the quantifier -- so "any other creature you control" (remainder "other creature...") never stripped "any ", never reached the "other"/"another" handler below it either (which reads from the unchanged pos), and the whole phrase fell through to the degenerate TargetFilter::Any fallback. The "all"/"each"/"every" block right below already composes through this exact "other"/"another" case via its own after_other check; the "a"/"an"/"any" block never did. Added the same after_other composition here, and a parser-level regression test (parse_type_phrase_any_other_creature_you_control) mirroring the existing "each other creature"/"all other creatures" coverage for the universal-quantifier case. * test: regenerate Kathril snapshot fixtures for the any-quantifier parser fix CI now has a working toolchain and confirmed exactly what these two insta snapshots (kathril_aspect_warper_ir.snap, kathril_aspect_warper_lowered.snap) need: the previous fixes to parse_type_phrase_with_ctx's "any" quantifier stripping make "any creature you control" parse to a real TargetFilter::Typed{Creature, controller: You} instead of the degenerate TargetFilter::Any fallback, for all 11 replicated keyword-counter nodes. Reconstructed both files precisely: matched the exact serialization shape (type_filters/controller/properties field order and presence of an empty "properties": []) against an existing snapshot with the identical "creature you control" filter (Conclave Mentor), rather than guessing. Also removed the now-stale TargetFallback diagnostic/ parseWarnings entry each file recorded for "any creature you control" -- that diagnostic no longer fires now that the phrase parses correctly, and confirmed the omitted-when-empty field shape against the same reference snapshot. Validated both files' JSON bodies parse correctly and that no "type": "Any" occurrences remain. * test: add target_choice_timing to Kathril snapshot fixtures CI's real toolchain surfaced the last piece: the widened target_choice_timing_for_clause (lower.rs) now correctly marks all 11 replicated keyword-counter clauses Resolution-timed (matching the CR 608.2d fix), and that field is present in the serialized AbilityDefinition when non-default. Added "target_choice_timing": "Resolution" right after "optional": false in each of the 11 keyword-counter nodes (flying through vigilance) in both snapshot files, matching insta's reported diff exactly -- excluded the P1P1 tail node (SelfRef target, correctly stays Stack-default/omitted) and the unrelated trigger-level "optional" field (a different struct). Validated both files' JSON bodies still parse correctly. * fix(test): default ChooseFromZoneChoice's test driver to the legal set The new drive_resolution handler for WaitingFor::ChooseFromZoneChoice hard-panicked when a test declared no objects for it, but this variant predates the Kathril work -- it's the existing CR 608.2d tracked-set choice mechanism (exiled-cards picks, etc.), and pre-existing tests using it (Portent of Calamity) never needed to declare anything, since drive_resolution previously had no handler at all for this variant and such tests must have relied on a path that didn't reach here, or on this exact prompt not existing pre-fix. Match the convention every other default-driven choice in this function already uses (ScryChoice, SurveilChoice, ArrangePlanarDeckTopChoice all auto-default rather than requiring explicit test declaration): try the declared-object pool first so a test can still pin a specific recipient (as the new Kathril discriminating test does), then fall back to the front of the legal set for anything undeclared, instead of panicking. * fix(test): drive ChooseFromZoneChoice manually, don't auto-handle in drive_resolution Reverts the previous fix's approach entirely: adding an auto-driving arm for WaitingFor::ChooseFromZoneChoice to the shared drive_resolution was the wrong direction. That variant predates this PR (it's the existing CR 608.2d tracked-set choice mechanism) and Portent of Calamity's own test (issue_6498) deliberately relies on .resolve() STOPPING at the first such prompt so it can drive each per-type exile pick manually, one at a time, with its own selection logic -- exactly like the pre-existing Wick test does. Auto-driving it out from under that test silently consumed all 4 of its prompts before the test's own loop ever ran, dropping its exile count to 0. Removed the arm entirely, restoring the original `_ => break` handling for this variant. Updated the new Kathril discriminating test to match the same manual-driving convention Portent's and Wick's tests already use: call .resolve() (which now correctly stops at the first prompt), then loop over WaitingFor::ChooseFromZoneChoice states directly, answering each with the declared recipient in written order. * fix(parser): chain a second consecutive subtype word instead of dropping it Reconciling the coverage-parse-diff blast radius from the "any" quantifier widening surfaced a real, pre-existing gap: the CR 205.3a "[Subtype] [CoreType]" promotion (e.g. "Wizard creatures") only ever consumed a SECOND word when it was a concrete core type. A second consecutive SUBTYPE word (e.g. "Elder Dragon", "Elf Warrior", "Human Wizard" -- two independently-registered subtypes stacked on one permanent, not a compound word) was silently dropped, since the `type_filters` builder discards it (the caller doesn't require an empty remainder). Fate Reforged chapter II ("a copy of any Elder Dragon from the Legends expansion") is the concrete case the parse-diff surfaced: before the "any" fix this collapsed all the way to TargetFilter::Any (matching literally anything); after it, "any " correctly reaches the subtype parser, which then only captured Subtype("Elder") and dropped "Dragon" -- an over-broad filter matching any Elder-subtype creature, not specifically Elder Dragons. Chained the second subtype word into the same slot the first arm already threads through, matching the Legends-era Elder Dragon type line (Nicol Bolas, Palladia-Mors, etc.) correctly. This is a general fix, not a Fate-Reforged special case -- it benefits any card using a two-subtype phrase, most of which never appeared in this PR's diff because they were broken the same way before the "any" fix too, just silently. Card-level reconciliation for the remaining coverage-parse-diff entries (issue phase-rs#6321 / PR phase-rs#6533 review): - Naming Screen: "doesn't share a name with any other creature you control" -- parse_shared_quality_reference explicitly rejects a TargetFilter::Any reference population, so before the "any"+"other" composition fix this whole relative clause failed to parse and the static ability fell through to an unstructured fallback. Proven via parse_shared_quality_clause_naming_screen_reference using the card's verbatim clause. - Duplication Device: "becomes a copy of any creature on the battlefield" -- Effect::BecomeCopy's target is parsed via the same shared parse_target this whole fix touches (oracle_effect/subject.rs); "any creature on the battlefield" now correctly reaches the pre-existing, unmodified zone-suffix machinery instead of collapsing to Any. Proven via parse_type_phrase_any_creature_on_the_battlefield. - Kathril, Mutable Pupa: already covered by this PR's existing integration tests. * fix(parser): exclude the "Urza's" possessive fragment from subtype chaining The two-consecutive-subtype-word chain added for Fate Reforged's "any Elder Dragon" regressed the dedicated Urza-lands condition parser: "Urza's" (LAND_SUBTYPES, card_type.rs) is the one possessive-suffixed entry in the whole subtype vocabulary -- a name fragment meant to attach to a following noun ("Urza's Mine"/"Tower"/"Power-Plant"), not an independently AND-combinable subtype like "Elder"/"Elf"/"Human". Chaining it fully consumed "an urza's mine" into one Typed{Subtype("Urza's"), Subtype("Mine")} filter with an empty remainder, which changed which downstream condition-builder claimed the clause and broke urzas_lands_share_delta_shape (expected ControllerControlsMatching{filter} with get_subtype() == "Mine", got a QuantityCheck over the two-subtype AND filter instead) and legacy_misparses_are_now_honest_gaps (expected None -- an intentionally undocumented gap -- got a confidently wrong QuantityComparison). Guarded the chain on the first subtype word not ending in "'s" (verified "Urza's" is the only such entry across LAND_SUBTYPES, ARTIFACT_SUBTYPES, ENCHANTMENT_SUBTYPES, SPELL_SUBTYPES, BATTLE_SUBTYPES, PLANESWALKER_SUBTYPES, and the MTGJSON-derived creature vocabulary), restoring "urza's mine" to its original Subtype("Urza's") + unconsumed "mine" remainder so the specialized handler still sees it, while keeping the chain for genuine two-word subtype pairs. * fix(parser): scope consecutive-subtype chaining to creature types only Redesigns the previous "exclude Urza's specifically" patch into a principled, CR-grounded rule instead of an ad-hoc exception. CR 205.3b: subtypes of every card type except creature (and plane) are always single words on a printed type line -- each dash-separated word is its own subtype. Creature subtypes are the one category the rules let run one OR two words (CR 205.3m: the sole two-word creature type is "Time Lord"; every other listed type, including "Elder"/"Dragon"/ "Elf"/"Warrior"/"Human"/"Wizard", is one word). So when Oracle text names two consecutive creature-subtype words, that is genuinely ambiguous -- one two-word type, or two one-word types stacked -- in a way that never arises for other categories, where CR 205.3b already guarantees each word is separate. This generic chain exists to resolve exactly that creature-only ambiguity, so it is now scoped to fire ONLY when neither matched word is a registered noncreature subtype (fixed_noncreature_subtypes -- land/artifact/enchantment/spell/battle/planeswalker), replacing the narrower "does the first word end in 's" check. "Urza's" is a real land type (CR 205.3i) -- land subtypes genuinely CAN co-occur on one permanent (Urza's Mine has both the "Urza's" and "Mine" land subtypes) -- but the dedicated Urza-lands ControllerControlsMatching condition parser already owns that Oracle-text pattern and deliberately extracts only the discriminating second word, so this generic rule must stay out of its way, and out of every other noncreature category's way too, not just this one land cycle. Also corrects the CR citation on the chaining rule itself from 205.3a (which only establishes that a card may have subtypes at all) to 205.3b + 205.3m (which establish the actual one-vs-two-word creature distinction the rule implements) -- 205.3a stays correctly cited on the unrelated pre-existing "[Subtype] [CoreType]" promotion arm beside it, which is a different pattern. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…hase-rs#6606) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…hase-rs#6604) * fix(client): size mana cost pips to the printed cost, not a fixed px The card-overlay mana pips were ~2.9x the size of the mana symbols printed on the card frame they sit on, and anchored above the title bar rather than inside it. On a desktop hand card a five-symbol cost spanned 67% of the card's width, reducing "Nissa, Ascended Animist" to "Nis". The badge stands in for the card's PRINTED mana cost (it renders the engine's effective cost, with a green ring when reduced), so calibrate its geometry against that cost. Measured on M15-frame art, a printed symbol is ~5.2% of the card's width, its right edge sits ~7% in and its top edge ~5.4% down. The fluid pip drops 15cqi -> 7cqi, which keeps a legibility margin over printed while fitting inside the title bar: a five-symbol cost now spans 32% instead of 67%. Fold the anchor into ManaCostPips as FLUID_ANCHOR. Diameter and anchor only look right together, and all five fluid consumers were repeating the placement by hand -- StackEntry had already drifted to a different value. CardPreview was the lone fixed-px consumer at 28px, which cannot be right for both a 300px hand hover and a 472px docked preview; give it the same @container basis so both land on the same ratio by construction, and drop the now-orphaned lg size. * fix(client): solve the fluid pip geometry for its stated 32cqi width The prior commit claimed a five-symbol cost spans 32% of the card's width, but 5 * 7cqi + 4 * 0.6cqi is 37.4cqi. The 32% came from measuring a fan-rotated hand card, where getBoundingClientRect returns an axis-aligned bounding box that inflates the frame and understates the row -- an invalid basis. Measured on an unrotated preview the row was 37.4%, exactly what the arithmetic predicts. Solve the three values for the stated target instead: 5 * 6cqi + 4 * 0.5cqi = 32cqi exactly. With 0.4cqi padding the symbol lands on 5.2cqi, which is precisely the printed symbol diameter, so the disk reads as a thin ring around a printed-size symbol rather than a badge sitting over one. Dropping the "legibility margin" costs nothing: the printed symbols on the same card are legible at 5.2%, which is what makes it enough. Verified in-app on an unrotated 300px preview -- symbol 5.2%, pip 6.0%, five-symbol row 32.0%. "Nissa, Ascended Animist" now reads to its last character where it was truncated to "Nissa, Ascended Anim" at 7cqi. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…opponent targets (phase-rs#5994) (phase-rs#6034) The parser fix for phase-rs#5994 landed via phase-rs#6565 (the verb-general `per_opponent_target_fanout_min` scan). This adds the runtime target-selection regression that main was still missing, driving Riptide's real ETB through the cast pipeline into `WaitingFor::TriggerTargetSelection` with two opponents: - `riptide_declining_all_per_opponent_targets_does_not_fizzle` declines every optional per-opponent target and asserts the ETB resolves back to `Priority` (both permanents untouched) rather than stalling mid-resolution. - `riptide_selects_one_opponent_target_and_declines_the_other` chooses one opponent's permanent (moved to its owner's library) while declining the other's optional slot (untouched), and asserts the ability resolves. Both are non-vacuous: they pass on current main and fail if `per_opponent_target_fanout_min` is forced back to a mandatory min of 1 — on that revert Riptide lowers to a non-targeted `EffectZoneChoice` that stalls (never reaching `Priority`) and the chosen permanent never moves, so the `final_waiting_for()` and zone assertions each flip. Refs phase-rs#5994
…unbounded-counter mark in the hover panel (phase-rs#6610) Two display-layer defects observed on a real 4-player board (Kilo, Apogee Mind / Freed from the Real / Relic of Legends / Pentad Prism). BUG 1 - Relic of Legends' click prompt offered two byte-identical "Add one mana of any color" rows. abilityChoiceLabel synthesised the label from the produced mana alone and dropped the engine's per-ability description, so any permanent with two mana abilities of the same produced shape collapsed to one indistinguishable option. The engine already ships the discriminator: the CR 602.1a activation-cost text. Attach it as the option's description, gated on the engine's own CR 605.1a verdict (is_mana_ability) plus the presence of engine-authored text, so loyalty mana abilities keep their badge rendering and synthesized basic-land intrinsics stay byte-identical to today. BUG 2 - after an accepted counter-growth shortcut, the hover status box still showed the finite charge count while the battlefield pill and art-crop badge correctly showed the unbounded mark. CardInfoPanel was the one counter render site with no subscription to DerivedViews::unbounded_counters. Extract the twice-duplicated selector into useUnboundedCounterTypes and subscribe all three sites through it. No engine change: the finite count is deliberate (materialize_object_growth_shortcut is display-only) and already covered end-to-end from the same dump. Also substitutes the engine's ~ self-reference token (CR 201.5) on the paths this change makes user-visible: abilityChoiceLabel's non-mana tail, the hover activate list, and both blocked-ability read-outs. Deferred, each with a measured blocker recorded in the hook's doc: the attack-target picker needs a per-counter-type .every() intersection threaded through groupByName/AttackerStack (a representative-only lookup would render a false unbounded mark), and DialogAttachmentCard has no component test file. Assisted-by: ClaudeCode:claude-opus-5
* feat: show temporary unblockable effects * fix: cover temporary unblockable badge * test: advance unblockable effects through cleanup --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ming point (phase-rs#6608) * perf(test): cap the 3p loop-shortcut grind drives at the measured priming point The three longest-running tests in the 21621-test corpus all live in loop_shortcut.rs, and all three spent ~99.9% of their wall clock in a `drive_collect` grind that runs BEFORE their assertions and never exits early. Measured: 500 beats = 19.30s of drive vs 9.4ms for the behaviour actually under test (inject offer -> DeclareShortcut -> accept -> E1 drive-and-measure). The grind was pure waste. A cap sweep over {4, 8, 12, 16, 24, 32, 48, 64, 100} shows every assertion and every reach-guard behaves identically at every cap, and each test's revert-probe still flips it to FAIL at every cap >= 4 -- the loop is primed by beat ~4 and nothing changes through beat 500. Replace the three literals with one shared `PRIMED_LOOP_BEATS = 24` (a directly swept value, 6x margin over the measured priming point) carrying the measurement as its rationale. interactive_3p_subset_lethal_does_not_crown 49.672s -> 2.446s (20.3x) injected_3p_one_faller_no_crown 49.814s -> 2.407s (20.7x) injected_3p_unequal_life_pin_all_no_crown 56.062s -> 4.850s (11.6x) Discrimination is preserved and re-proven on the SHIPPED tests, not on probe copies: each still FAILS under the specific defect it exists to catch -- weakened `nonfallers.len() != 1` (loop_check.rs), bypassed E1-measure `live_mandatory_loop_winner` gate, and removed F2 `fallers_lives_pairwise_equal` re-check. Every panic lands on the test's intended discriminator assertion, not on a reach-guard. No production code, no fixture, and no assertion body changes: the entire non-comment delta is the new const plus three cap substitutions. Two doc corrections the measurements forced, both pre-existing errors: - The M1 REVERT-FAIL note claimed the mutation flips "the two no-crown assertions". Measured: only the `wf` assertion discriminates; the event-scan stays TRUE at every cap. Now stated accurately. - The equal-life half of `setup_3p_both_fall` consumes 0 beats -- its mandatory single-non-faller loop is already crowned by the natural bridge while the kick-off resolves. Cap-independent, so this was equally true at the old cap 200. The const doc carries the measured per-drive beat counts rather than a blanket claim. Assisted-by: ClaudeCode:claude-opus-4.5 * test(loop-shortcut): guard the primed-loop regime the no-crown tests assume Review of phase-rs#6608 (matthewevans, CodeRabbit) found the three no-crown tests had no positive guard proving the loop-classification decision was actually reached: a regression that delayed sampling or classification past the drive cap would leave the runner in `Priority`, satisfy the existing life-loss guards, and read green. Add `drive_collect_primed`, which drives through the existing `drive_collect` beat by beat and reports whether any prior in `GameState::loop_detect_ring` compared equal to the live state modulo resources -- `loop_states_equal_modulo_resources`, the engine's own public predicate, so no recurrence logic is reimplemented test-side. A ring hit forces the bridge's §3 conjuncts to have held at that state, so `find_live_loop_winner` -> `live_mandatory_loop_winner` demonstrably ran on it. Measured, and the measurement shaped the design: - Recurrence is PHASE-dependent, not monotone. Over caps {1,2,3,4,8,24} it holds at 2 and 8 and not at 1/3/4/24 (period 6) -- so the obvious terminal-state check would report false on a perfectly primed loop. Hence the per-beat scan, short-circuited at the first hit: 5.6-17.5 ms per drive, priming at beat 2. Suite still 69/69 in 8.1s. - Discriminating: forcing the cap to 1 (below the beat-2 priming point) flips all three tests to FAIL on this guard. - Placed LAST in `interactive_3p_subset_lethal_does_not_crown`: there the classification and the wrongful-crown failure mode share one drive, and measured, a guard placed first steals the M1 panic (M1 crowns at beat ~1, before recurrence) and reports the wrong cause. Guard last, M1 lands on the intended `wf` no-crown assertion. - Asserted on the unequal half only of the F2 test: the equal half consumes 0 beats (already crowned as the kick-off resolved), so it has no drive in which to recur, and it is the unequal half the F2 revert-probe flips. - M1/M2/M3 re-run on the shipped text: each still fails at its intended discriminator (lines 1212 / 1425 / 1640), not at a guard. Also corrects two claims in the const doc that measurement falsified. The life-loss guards do not "FAIL LOUDLY" when priming is delayed, and this new guard does not close the separate bridge-suppression blind spot either: suppressing the live-detect bridge leaves all three tests green at cap 24 AND at cap 500 (while two offer-dependent tests fail), because the ring sampler is an independent gate. That blind spot is cap-independent -- no cap closes it -- which is why a guard was needed rather than a bigger cap, and it is inherent to a negative test. The doc now says so, states the equality-disjunct-only scope (`loop_states_cover_modulo_growth` is `pub(crate)`), and drops an interpolated "beats 14, 20" claim that was never probed. Still zero production changes; every assertion condition and the cap itself are unchanged. Assisted-by: ClaudeCode:claude-opus-4.5 --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…hase-rs#6091) (phase-rs#6588) * fix(engine): repeat Grindstone mill while milled cards share a color (phase-rs#6091) * fix(engine): compound LastZoneChanged counts and Grindstone regressions (phase-rs#6091) Probe last_zone_changed_ids for nested ledger filters before applying the full compound predicate, reorder Painter fixture so the first mill is the blue/green pair, and assert trailing library cards remain when repeat stops. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): compose LastZoneChanged candidate population correctly (phase-rs#6091) Derive object-count candidates compositionally (ledger seed for positive And, union for Or, complement for Not) instead of gating on the broad filter_contains_last_zone_changed helper. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): recurse candidate universe for nested Or filters (phase-rs#6091) Union branch candidate populations for every And/Or node before applying matches_target_filter, so And(Or(LastZoneChanged, Typed), Typed) includes off-battlefield ledger members. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(engine): union inner universe for compound Not filters (phase-rs#6091) Build every Not candidate set as zone population union the recursive inner universe so Not(And(LastZoneChanged, Typed)) includes off-battlefield ledger members that fail the inner conjunction. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Andriy Polanski <andriy.polanski@gmail.com> Co-authored-by: Cursor <cursoragent@cursor.com>
* fix: clarify replacement modal choices * fix(PR-6612): address review feedback * test(PR-6612): isolate mana symbol assertions * fix(PR-6612): box labeled decline fixture * test(PR-6612): derive replacement choice fixture --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…6616) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(engine): enforce event-bound forced blocks * fix(engine): complete force block type plumbing * fix(ci): resolve force block compile errors * fix(parser): preserve generic forced block forms * fix: address force-block review findings * fix: keep force-block rw match exhaustive * test: use blocker prompt copy in reset coverage * test: initialize force-block trigger fixtures * test: flush force-block effects before blocker validation * fix: preserve event-bound force-block targets --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(p2p): deduplicate host initialization * fix(p2p): reset failed host initialization --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…hoice, not a target (Strategic Betrayal) (phase-rs#6607) * fix(engine): a player-directed "exile a creature they control" is a choice, not a target (Strategic Betrayal) Strategic Betrayal ("Target opponent exiles a creature they control and their graveyard.") triggered Ward on the opponent's own creature: the "exile a creature they control" clause resolved as a TARGETED exile with the wrong actor, instead of a resolution-time choice the target opponent makes. Per CR 115.10a/b a non-"target" object is not targeted (Ward, CR 702.21a, must not fire) and CR 109.4 makes the target player the actor. Two coupled defects, fixed at their seams: - Parser origin leak: a trailing "and their graveyard" conjunct leaked origin:Graveyard onto the creature leg. Origin is now inferred from the primary clause slice only (compound_exile_origin_scan), applied uniformly across the three compound-exile sites. - Target-vs-choice + actor: the "target player exiles ..." arm now pins the anaphoric "they control" scope to ScopedPlayer (unifying the creature and graveyard legs) and stamps Resolution timing on the battlefield pick, so it is a non-targeted player-directed choice. At resolution, a single Player-target ability with a ScopedPlayer-scoped move binds scoped_player to the target player (sibling of the existing trigger-event stamp), so the target opponent is the chooser and their own graveyard is exiled. Reuses existing mechanisms only (no new enum variant); controller_for_relative_filter, sacrifice.rs, and rebind_owned_scope are untouched. The fix generalizes to Leadership Vacuum ("target player returns each commander they control"). Closes phase-rs#6505 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(engine): narrow the target-player exile scope to the moved-object leg; account for parse blast radius (phase-rs#6505 review) The Strategic Betrayal fix pinned `relative_player_scope = ScopedPlayer` around the ENTIRE `lower_imperative_clause` call, so it re-scoped every "they control" predicate — including NON-ChangeZone effects (Sacrifice / PutCounter / PutCounterAll / UntapAll / bounce-repeat) that the runtime `scoped_player` stamp does not cover (`ability_uses_relative_controller_scoped` only matches ChangeZone/ChangeZoneAll). Those parse changes were unintended and latently broken (`ScopedPlayer` with no resolution-time binding). Narrowing (replaces the whole-clause parse-time pin): - Remove the `relative_player_scope = ScopedPlayer` save/restore around the predicate lowering. The predicate now lowers with its natural scope. - Apply the `ScopedPlayer` rebind POST-lowering to the moved-object leg ALONE: when the lowered leg is a battlefield resolution-pick ChangeZone/ChangeZoneAll (`creature_leg_is_resolution_pick`) whose anaphoric "they control" lowered to the default `ControllerRef::You`, rewrite ONLY that leg's `You` controller to `ScopedPlayer` (new `rebind_controller_scope(from, to)` general helper). Gated on the "they control" anaphor so a "you control" caster leg (also `You`) is never mis-rebound. The Resolution-timing stamp stays gated on `creature_leg_is_resolution_pick`, unchanged. `rebind_owned_scope`, `controller_for_relative_filter`, and `sacrifice.rs` are untouched. Effect: the Sacrifice / PutCounter / PutCounterAll / UntapAll / bounce-repeat predicates keep their original scope and DROP OUT of the parse-diff. Verified by a baseline(192897c)-vs-head focused parse harness over the full named blast radius: every non-ChangeZone card is `ScopedPlayer=0` on both sides (Consecrate//Consume, Consumed by Greed, Will of the Abzan, Riveteers Charm, Linessa Zephyr Mage, Hunted Nightmare, Shadrix Silverquill, Early Harvest). Accounted parse-diff (all correct, no regressions): - Part B (target-player "exile/move/return a permanent they control" → the moved-object leg is ScopedPlayer + Resolution): Strategic Betrayal, Debt to the Kami (both modes), Doomfall (creature mode), Early Winter (enchantment mode), Azula Cunning Usurper, Leadership Vacuum (mass "each commander they control"). Blot Out / End of the Hunt do NOT change — their "greatest mana value" superlative is a context-ref filter (not a resolution-pick), so they revert to the pre-existing (unchanged) baseline scope. - Part A (origin de-leak, `from: graveyard/library → ∅`): the trailing "and <graveyard/library conjunct>" no longer leaks its zone onto the primary battlefield/self leg. `∅` is a correct, safer parse: for a specific object the resolver keys on the object's actual zone and treats `origin` as an optional guard (`Some(x)` that mismatches → skip), so `∅` = "no constraint" exiles from the real zone — and actually FIXES Silent Gravestone (baseline `origin=Graveyard` would have skipped exiling its own battlefield artifact). Verified correct for Silent Gravestone, Spellweaver Volute, Gilded Ambusher, and (full-pipeline) Once More with Feeling, Shadows' Verdict, Ultimate Nullification. Dragon's Approach stays Unimplemented (no surfaced change). Also in this commit: - `compound_exile_origin_scan`: replace the manual `base[..base.len()-rem.len()]` slice (panics on a non-suffix / mid-UTF-8 `rem`) with a fail-closed `base.strip_suffix(rem).filter(|_| trailing_and_conjunct)`. - strategic_betrayal_6505.rs hostile fixture: make the creature-leg lookup REQUIRED (`expect`) so it can't pass vacuously. - New discriminating tests: a bare "target player exiles a creature they control" sibling runtime test (opponent is chooser, correct scope, no Ward); a Leadership Vacuum mass-leg ScopedPlayer shape test; and an origin-leak class test (Silent Gravestone's "exile this artifact and all cards from all graveyards" — primary leg stays battlefield origin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(PR-6607): make controller scope rebinding exhaustive * fix(PR-6607): cover current target filter terminal --------- Co-authored-by: michiot05 <281539540+michiot05@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ed Worldsire) (phase-rs#6620) * fix(engine): carry the sacrificed-permanent quality on Devour (Famished Worldsire) "Devour land 3" let the engine offer creatures to sacrifice instead of lands: Keyword::Devour(u32) dropped the "[quality]" axis of CR 702.82c, and synthesize_devour hard-coded the ETB sacrifice pool to creatures. Parameterize Keyword::Devour(u32) -> Keyword::Devour { n: u32, quality: TypeFilter }, defaulting to TypeFilter::Creature (the CR 702.82a plain- Devour behavior). The parser captures the optional quality word ("land", "artifact", "food", ...) via composed nom combinators; the synthesizer threads it into both the Sacrifice target and the sacrificed-count filter; the counter math (N x permanents sacrificed) is unchanged. is_devour_etb_ replacement now discriminates on the quality so same-N different-quality replacements don't false-dedupe. Famished Worldsire now offers lands, Caprichrome artifacts, Feasting Hobbit Foods; plain Devour still offers creatures. Serde keeps a bare-number back-compat for old {"Devour":3} states. The mtgish-import converter maps its already-carried quality qualifier and strict-fails on a non-specific catch-all rather than silently widening the pool. Closes phase-rs#6423 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(PR-6620): format devour quality nouns exhaustively --------- Co-authored-by: michiot05 <281539540+michiot05@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…(CR 603.12) (phase-rs#6621) * fix(parser): bind "that token" inside a reflexive "when you do" body (CR 603.12) `chain_prior_referent_is_created_token` bailed `return false` on the first prior clause carrying a `condition`, before it ever reached the `is_token_creating_effect` check. In a reflexive body the token-creating clause is exactly the clause that carries the gate — `strip_if_you_do_conditional` stamps `WhenYouDo` / `EffectOutcome{OptionalEffectPerformed}` onto it — so a later "that token" anaphor in the same body never saw the token and misbound. Iroh, Tea Master placed its +1/+1 counter on an unbound `ParentTarget` instead of the Ally it had just created; Summoner's Sending bound `TrackedSet{0}`. The referent walk now continues past a prior clause whose condition is an affirmative reflexive gate, and the consuming clause is re-linked `SequentialSibling` -> `ContinuationStep` so the resolver's condition-false descent cannot resolve it against `state.last_created_token_ids` — a game-lifetime ledger that would otherwise hand it a token from an earlier resolution (CR 609.3: no legal antecedent). Building blocks, not special cases: - `AbilityCondition::is_affirmative_reflexive_gate` — exhaustive 53-arm match, no wildcard, so a future variant is a compile error rather than a silent reclassification. Its `true` set is exactly the image of `parse_affirmative_reflexive_connector`. - `AbilityDefinition::is_self_gated_reflexive` — a clone that carries the gate itself is honest wherever it lands. - `oracle_ir::ast::sub_link_after_boundary` and `lower::instruction_spine_is_continuation` extracted so the seeder and the re-link share one authority instead of two drifting copies. - The repeat-process clone guard asks its question directly: it builds the def the assembler would push, runs the re-link over it, and reads the resulting link back, rather than stacking structural proxies. Blast radius, measured over all 34348 AtomicCards entries: 20 cards, 25 chain nodes, every one `SequentialSibling` -> `ContinuationStep`, and exactly 2 `PutCounter.target` moves (the two cards above). No node added or removed on any card. Verification: nextest -p engine 21635 passed / 0 failed; clippy --workspace --all-targets --features engine/proptest -D warnings 0 warnings; check-parser-combinators.sh Gate G + Gate A PASS; combo gate 98 passed / 0 failed; cargo combo-verify rows byte-identical BASE vs POST (13 confirmed / 4 gated / 37 deferred / 0 failed of 54) with the two card-data exports confirmed to differ, so the identity is not vacuous. 21 durable regression tests, each with an executed revert-probe that flips. Assisted-by: ClaudeCode:claude-opus-5 * fix(PR-6621): traverse nested token referents --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Parse changes introduced by this PR · 186 card(s), 101 signature(s) (baseline: main
|
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
16 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.
A spell line that creates a token "with "..."" whose granted-ability quote
ends a sentence was silently swallowing a following typed-group continuous
sibling clause (e.g. Circle of Power's "Wizards you control get +1/+0 and gain
lifelink until end of turn."). The clause-sequence splitter only split after a
sentence-ending close quote when the continuation was a bare imperative verb,
"then"/"if"/"otherwise", or a causative "may have" — a subject-led group clause
(" you control get/gain ...") has a NOUN head, so it fell through and was
folded into the token-creation clause and dropped with no diagnostic (CR 111.3
token text vs CR 611.2a one-shot mass continuous effect).
quote_closes_sentence_before_sequencenow also splits when the continuation isa typed-group continuous clause, recognized via the shared
starts_with_type_wordcombinator (already used by
continues_mass_exile_union) plus a followingcontinuous predicate verb located by
find_predicate_start— the exact pairtry_parse_subject_continuous_clauseuses to split subject from predicate, so amatch guarantees the continuation parses as a group-continuous clause. Anaphors
on the created object ("It gains ...", "That creature gets ...") keep a
pronoun/determiner head and remain attached via
starts_anaphoric_subject.Covers the class of "create token with "...". you control get/gain ...
until end of turn" cards, not just the one card. Coverage unchanged (no
regressions); the Circle of Power semantic-audit finding is cleared with zero
new findings.
Co-Authored-By: Claude Opus 4.8 noreply@anthropic.com
Claude-Session: https://claude.ai/code/session_014cZ4EgEbY8Pvr8RSiEJwZT