ship/fix desktop show native engine provisioning status (#6546) - #9
Open
keloide wants to merge 180 commits into
Open
ship/fix desktop show native engine provisioning status (#6546)#9keloide wants to merge 180 commits into
keloide wants to merge 180 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>
…ducers at once (phase-rs#6724) `OracleNodeIr::Spell` carries an `AbilityIr` instead of a bare `EffectChainIr`, and `ability_ir_at` emits it directly rather than lowering eagerly into the pre-lowered variant. Because T8 had already routed every phase-A producer through that one method, flipping its body is what converts all nine — the whole payoff of the A0–A4 staging. A chain alone cannot carry the CR 602.1 activation envelope a recognizer stamps around it (cost, activation restrictions, ability tag, the CR 601.2b announced-X floor), which is why a chain-payloaded node forced eager lowering in the first place. CR 707.9a stamping moves from `OracleDocBuilder::finish` to `lower_oracle_ir`'s bucketing loop. It has to: the stamp rewrites the placeholder printed slot inside a definition, and an IR-native spell has no definition until `lower_ability_ir` builds one, so a finish()-time walk could only stamp the pre-lowered shapes and would silently skip every converted producer. The two walks are order-equivalent — verified, not assumed: * both iterate the same `BTreeMap` keyed by `(first_line, start_byte, ordinal)`, so both visit the identical source-ordered sequence and count each category separately; the k-th spell item is at ability slot k either way * the slot is now `result.<category>.len()` at the moment of the push, so "consumes a slot" and "is pushed" are the same operation — the old walk's separate counter could drift from the vector it was predicting, and this cannot * stamped inside the loop, before the relation passes that insert into, remove from, and move ids between the category tracks — the same pre-relation state finish() saw * nothing between the two seams reads a printed slot: relation discovery and the swallow audit inspect effect trees for shape, never `RetainPrinted{Trigger,Ability}FromSource` indices (grep-verified) The exhaustiveness obligation moves with it. `lower_oracle_ir`'s match is already exhaustive over `OracleNodeIr` with no `_` arm, so a new node variant still cannot be added without deciding its slot behavior. `mutate_last_spell` is retired for a typed `raise_last_spell_min_x`, and `reemit_spell` generalized to `reemit_node`. The general closure mutator's `impl FnOnce(&mut AbilityDefinition)` could only be honored by lowering the popped node, so it could not preserve an IR-native spell — it silently re-emitted pre-lowered. `OracleNodeIr::spell_min_x_mut` handles both shapes at the layer each stores the floor (`AbilityShellIr::min_x_value` vs the lowered root), which agree because `apply_ability_shell_envelope` applies the shell's value with `max`. It has exactly ONE caller (the empty-line guard); T7 already deleted the other. Measured over the real pool (data/mtgjson/AtomicCards.json symlinked into the worktree, generated from the repo root): card-data.json is BYTE-IDENTICAL before and after, sha256 c3f8431c494c1a9710e08fc15161eab75203a2dc98a5aa35e3f7a993ec43c245, 99,201,978 bytes, 35,516 face-keyed entries. Generator rebuilt fresh for both runs. Zero delta is a semantic no-op, not an unapplied probe: * the compiler witnesses the conversion — `lower_effect_chain_ir` became an unused import in `oracle.rs`; no spell path there lowers a bare chain any more * 10 `*_ir.snap` files churn, every one of them exactly `PreLoweredSpell` -> `Spell` (total re-serialization: flat def -> `{source_text, body:{clauses}, shell}`). Zero `*_lowered.snap` files change, re-verified after accepting the IR snapshots so a failing `_ir` assertion could not mask a later `_lowered` one — that pairing is the non-vacuity probe, and it is what proves the re-serialization lowers to the same definitions * both stamping directions watched go red: dropping the relocated stamp makes Thespian's Stage read `RetainPrintedAbilityFromSource(0)` instead of `(1)`; restoring the closure mutator makes the new min_x test's node assertion fail while its floor assertion still passes The CR 707.9a placeholder churn the plan predicted did NOT materialize, and that is a finding rather than a discrepancy: no `*_ir.snap` fixture carries a copy-except clause, because `parse_activated_ability_definition` is the only writer of `ParseContext::current_ability_index` and therefore the only source of `RetainPrintedAbilityFromSource` — and it is deliberately still pre-lowered (U0-28/U0-31, T10b). So no IR-native spell body can carry a printed slot today; the stamp's IR-native ability arm is structurally unreachable, which is why the move is byte-neutral. It becomes load-bearing the moment that router converts. New test `a_standalone_x_floor_annotation_raises_an_ir_native_spells_floor` pins the newly reachable path: before this commit the only IR-native producer could not precede a "X can't be 0." line, and now nine can. Harvest, corrected in place because each is prose that now contradicts the code: * `PrintedAbilityIndex::placeholder`'s doc block read as forbidding this very relocation ("deriving the slot from a category-vector length … the source-ordered document builder makes that equality false"). It does not: that sentence is about the retired PARSE-TIME `from_category_vector_len`, which read the length in emission order while a preprocessor could emit out of order. The lowering walk reads the same expression in SOURCE order, where it genuinely coincides with the printed slot. Same expression, different quantity; disambiguated so the next reader does not conclude this commit is unsound. * `lower_oracle_ir`'s "PreLowered variants are identity-lowered (pushed directly to the result)" was true until this commit and is not any more — both spell and trigger arms now stamp. * The `OracleNodeIr` block comment claiming all four IR-native siblings are "dead-coded pending Plan 05 U2's document-seam hoist" — all four have live producers now. * The deleted priority-13e gravestone still named `mutate_last_spell` as having "two callers"; T7 deleted one and this commit retires the symbol. * `ir_spell_node_readers.rs` claimed the IR-native `item_ability` arm has no witness and that `Spell` producers would "rise from 1 to ~14" — the real number is 9, and the arm now has producers (though still no relation witness). Harvest, reported not fixed: * The `*_ir.snap` corpus contains ZERO fixtures carrying a copy-except clause, so it supplies no evidence about CR 707.9a stamping in either direction. The coverage lives entirely in `thespians_stage_emits_retain_printed_ability_from_source` and `an_ir_native_spell_consumes_the_printed_ability_slot_it_occupies`. A two-layer fixture belongs with T10b, when the router that produces the carrier converts. * `finish()` no longer needs `mut self` — it took it only for the stamping walk. Ratchet: `oracle.rs` 16 -> 15. The token count and the semantic producer count move independently (T9a's observation): `ability_ir_at` never contained the literal token, so converting nine producers moves the ledger by one incidental prose mention, not by nine. `doc.rs` stays at its ceiling because `OracleNodeIr::spell_min_x_mut`'s match is exhaustive over the enum and four of its arms name a pre-lowered variant. That match feeds an `.expect()`, so a `_` arm would convert a future compile error into a runtime panic — worth four tokens. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* feat(parser): support Scholarship Sponsor and Enigma Ridges * test(PR-6725): prove tied ETB reaches resolver --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ed (phase-rs#6726) * refactor(parser): Plan 05b U0-61 — hoist the Class effect-sentence recognizer to IR `oracle_class.rs`'s effect/spell-like line arm ("You may play an additional land...") emitted a pre-lowered `AbilityDefinition`; it now emits `OracleNodeIr::Spell(AbilityIr)` and lets `lower_oracle_ir` do the lowering. Byte-identical BY CONSTRUCTION, not by corpus. `parse_effect_chain(t, k)` **is** `lower_ability_ir(&parse_ability_ir_standalone(t, k))` — that is the function's literal body at `oracle_effect/mod.rs`, not a claim about it: pub fn parse_effect_chain(text: &str, kind: AbilityKind) -> AbilityDefinition { lower_ability_ir(&parse_ability_ir_standalone(text, kind)) } So splitting the call into its two halves moves WHERE the lowering happens, not WHAT it produces. This is the same algebraic identity every T8 R1 row used. The `has_unimplemented` gate still reads a LOWERED def — `has_unimplemented(&lower_ability_ir(&ir))` — the shape A3 used at U0-12 and T9a used at U0-39. Whether to emit at all is control flow, and `has_unimplemented` is defined over an `AbilityDefinition`, so the predicate must see the definition the site actually emits. An IR-level `has_unimplemented` over `EffectChainIr` would be a rival authority free to diverge from the one the rest of the parser uses. Only safe on top of T9b: before the `Spell` payload carried `AbilityIr`, this hoist would have dropped `finalize_effect_chain` and the owner-library reveal anchor. Based on T9b's tip (phase-rs#6724, not yet merged). Scope: line 334 only. `oracle_class.rs:398` (the class-level trigger's `execute`) is a different site and is untouched; the `make_unimplemented` residual beneath this block is U0-62, which is blocked on a user ruling. Burn-down: `oracle_class.rs` drops 5 -> 4 in scripts/prelowered-ratchet.txt. * refactor(parser): Plan 05b U0-40 — the heterogeneous IR node mechanism `lower_as_enters_becomes_choice_modal` (CR 208.2b modal "As ~ enters, it becomes your choice of <P/T profile>, ..." — Aquamorph Entity, Primal Clay, Primal Plasma) stops pushing pre-lowered definitions into a scratch `ParsedAbilities` and returns typed IR instead: N `StaticIr` + 1 `ReplacementIr`. This introduces the heterogeneous `Vec<OracleNodeIr>` producer the recensus says to introduce ONCE, deliberately (no such producer existed anywhere in the parser). It is built as a general mechanism — `DocEmitter::emit_ir_nodes_at` emits any ordered node sequence at one line — not as a special case for this site. Which drain caller this is: `drain_result_vectors` has two callers. oracle.rs 4304 is the Priority-1 modal block feeder (`parse_oracle_block` -> `lower_oracle_block`), which is U0-18 and belongs to T10h; it is untouched. This row is the OTHER one, the Priority-8 replacement-adjacent site, identified by content: it is guarded by `is_as_enters_becomes_choice_pattern` and calls `lower_as_enters_becomes_choice_modal`, and its in-code CR annotations (208.2b / 614.1c / 614.12a) are exactly the census row's citations for U0-40. Byte-safe by construction. Converting these definitions to IR newly subjects them to `lower_static_ir`'s two transforms and to `lower_replacement_ir`; all three are provably inert here: * `lower_replacement_ir` is `ir.definition.clone()` — identity, by definition. * `populate_active_zones_from_condition` collects only `SourceInZone` (through And/Or/Not); `StaticCondition::ChosenLabelIs` hits the `_ => {}` arm, so `zones` stays empty and the `!zones.is_empty()` guard never fires. * `bind_counter_anaphor_to_recipient` rewrites only `QuantityRef::CountersOn { scope: Anaphoric }`. Anaphoric counter scopes are produced by exactly one combinator, `parse_deferred_counter_pronoun` (via `parse_for_each_counters_on_source`), and survive only through the two `_deferred` entry points — every other path runs `settle_deferred_counter_anaphor_ref`, which collapses Anaphoric -> Source at parse time. Both non-test callers of the deferred entry are in `oracle_static/anthem.rs`. These modifications come from `animation_modifications_with_replacement` <- `parse_animation_spec`, whose dynamic-P/T routes are `parse_quantity_ref_complete` and `parse_dynamic_pt_clause` -> `nom_quantity::parse_quantity_ref`; neither reaches a deferred entry point. The condition axis is inert for the same reason as above: `bind_anaphoric_counters_in_condition` matches only `UnlessPay`/And/Or/Not. Emission order is preserved deliberately. `drain_result_vectors` emitted by CATEGORY (abilities, triggers, statics, replacements) regardless of push order, and `emit_at` stamps `ordinal_within_span` in emission order, so the caller emits the CR 614.1e face-up residual first, then the statics, then the replacement — which is NOT the order the recognizer constructs them in. `emit_ir_nodes_at` dispatches through the typed `*_ir_at` helpers rather than straight to `emit_at`, so the `last_static` peek mirror that `static_at` maintains today keeps being maintained; `parsed_result_recently_granted_flashback` reads it mid-loop. The CR 614.1e residual stays a bare `AbilityDefinition` the caller emits via `ability_at`: it is a fixed strict-failure marker with no parsed body, and constructing a pre-lowered node inside `oracle_replacement.rs` would move a producer into a file the burn-down ledger does not track. Giving it a real IR node is U0-62's question, which is out of scope. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…s-with replacements
The Adamant cycle parsed its "enters with a +1/+1 counter" clause as a
CR 614.1c replacement but dropped the sentence-initial "If <condition>, "
gate, leaving ReplacementDefinition.condition = None. The engine therefore
gave every copy of these creatures the counter regardless of what mana was
actually spent — silently-wrong behaviour that nothing flagged at runtime.
Coverage reported it as Swallow:Condition_If on six cards.
Root cause: oracle_replacement.rs peeled a TRAILING " unless <cond>" gate
(extract_enters_with_unless_suffix) and a trailing " if <cond>" gate
(extract_enters_with_only_if_suffix), but nothing peeled a LEADING one, and
the payload scan skips everything before "enters with".
Fixed for the class, not the cards:
* extract_enters_with_leading_if_gate mirrors the existing trailing
extractor one-for-one, including its fail-closed tri-state. Condition
recognition delegates to parse_inner_condition (the single authority), so
the ENTIRE static-condition grammar is now reachable from the
sentence-initial position, where previously zero shapes were. An
unrecognised gate returns Unparsed and the clause falls through to
Effect::unimplemented rather than committing a replacement with the gate
erased.
* CR 207.2c ability-word labels are peeled via parse_known_ability_word_name,
which enumerates exactly the CR 207.2c list. CR 207.2d flavour words are
deliberately NOT peeled: the loose 4-word heuristic would peel "Sensational
Save" and regress Scarlet Spider, Ben Reilly to Unparsed.
* CastManaSpentMetric::OfColor parameterises the existing metric axis
(Total | DistinctColors | FromSource) rather than adding a
ReplacementCondition sibling, so the parsed condition flows through the
untouched replacement_condition_from_static into OnlyIfQuantity. Zero new
ReplacementCondition variants, zero new condition-evaluation arms.
* parse_color_mana_spent_threshold reuses parse_amount_threshold, so the
comparator axis (at least N / N or more / N or fewer) is carried rather
than hardcoded to GE.
Coverage: six cards flip to supported (Ardenvale, Embereth, Garenbrig,
Locthwain and Vantress Paladin, plus Necromantic Summons), zero cards
regress. Dust Animus keeps supported=true and gains its gate, so it is
correctly conditional for the first time. Henge Walker ("mana of the same
color" — a max-over-colors metric) and Red and Black Legacy now fail closed
and stay honestly red instead of silently granting their counters.
Measured against a pre-change baseline of all 35516 cards: 0 true->false,
6 false->true, net +6.
CR 614.1c, CR 614.12, CR 207.2c, CR 207.2d, CR 106.3, CR 601.2h, CR 400.7d.
CR 603.4 is deliberately not cited in code: its intervening-if rule is
scoped to triggered abilities, and this gates a replacement effect.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
…hase-rs#6416) (phase-rs#6707) * fix(engine): resume turn order after specified turn for OOS extras (phase-rs#6416) * fix: the clippy lint: initialize active_player * fix(engine): add CR733 matrix row and extra-turn regression tests (phase-rs#6416) Register extra_turn_sequence_anchor in the authority matrix so shard-2 CI passes, and cover legacy extra_turns serde plus nested outer-anchor latch. Co-authored-by: Cursor <cursoragent@cursor.com> * test(engine): cover nested OOS extras through ExtraTurn resolver (phase-rs#6416) Add a GameScenario C→A→B→D path that casts Nexus via the production resolver during both C and A's extra, so the outer-anchor latch cannot regress without failing CI. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com> Co-authored-by: Cursor <cursoragent@cursor.com>
…hase-rs#6446) (phase-rs#6727) * fix(parser): Relic of Progenitus first ability targets only a player (phase-rs#6446) Widen the Strategic Betrayal resolution-pick gate so off-battlefield non-target ChangeZones (exiles a card from their graveyard) stamp Resolution and keep Owned{ScopedPlayer}, collapsing the buggy 3 stack slots to one player target with EffectZoneChoice by that player. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(parser): keep ChangeZoneAll Stack in clause timing helper (phase-rs#6446) target_choice_timing_for_clause must not reclassify mass ChangeZoneAll moves (Bomat Courier / Jace -12 snapshot regressions). Relic still uses the shared helper from the TargetOnly wrap for both variants. Co-authored-by: Cursor <cursoragent@cursor.com> --------- Co-authored-by: Cursor <cursoragent@cursor.com>
* fix(parser): expire step-deadline durations at the step they name (Orcish Farmer)
`oracle_nom/duration.rs` recognized the step-deadline family only as two
hardcoded strings ("your next end step", "the next end step"). Every other
`until [the beginning of] <possessor> next <step>` phrase fell through, so its
clause lost the duration.
Orcish Farmer ("{T}: Target land becomes a Swamp until its controller's next
untap step.") is the silently-wrong case: no `Effect::Unimplemented`, no parse
warning, the card reads as supported, and the duration defaults to
`Permanent` (CR 611.2a), turning an opponent's land into a Swamp forever.
Factor the production into its two real axes — possessor and step — and map
each pair onto the existing `Duration::UntilNextStepOf` variant. No new engine
variant: `Phase::Upkeep`, `Phase::Untap` and `PlayerScope::{Controller,AnyTurn}`
all already exist. `step_deadline_scope` is the single place that decides
whether a pair has a runtime authority whose scoping matches the phrase, and
declines the rest so they stay honestly `Effect::unimplemented`.
Give the one newly-reachable deadline its expiry authority (CR 500.4 — as a
step begins, effects that last until that step expire): both halves, mirroring
the existing end-step pair —
* `layers::prune_until_next_upkeep_effects` for transient continuous effects
* `layers::prune_upkeep_step_casting_permissions` for durational
`PlayFromExile` grants (Elkin Bottle / Grinning Totem lower to this, not to
a continuous effect)
wired at `Phase::Upkeep` after the skip check (CR 614.10a) and before the
upkeep triggers (CR 500.6). The existing untap prune gains the turn-agnostic
arm so no emitted pair is left unenforced.
Measured over all 35,316 MTGJSON card faces: 10 faces change, none regress.
Four lose an `Effect::Unimplemented` (Elkin Bottle, Erhnam Djinn, Grinning
Totem, Xenic Poltergeist); six record a duration that was previously dropped
while keeping an independent gap in the same clause.
Backlog: removes Orcish Farmer from root cause 25 (29 -> 28).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(engine): cite CR 611.2a + CR 500.4, not CR 603.7b, for the AnyTurn deadline
Review finding (HIGH, @matthewevans): CR 603.7b governs delayed triggered
abilities — "a delayed triggered ability will trigger only once, the next time
its trigger event occurs". It is not authority for when a continuous effect or
a casting permission expires, which is what the turn-agnostic `PlayerScope::AnyTurn`
deadline in this PR actually is.
Replace every CR 603.7b citation this PR introduced with the correct basis:
CR 611.2a (the effect lasts as long as *stated* — "the next <step>" names a
step without naming whose it is, so the deadline is the first such step) plus
CR 500.4 (as that step begins, effects lasting until it expire). The per-step
rules CR 502.3 / CR 503.1 / CR 513.1 stay only as descriptive context naming
which step each prune owns.
Scope: the 10 sites this PR added. The pre-existing CR 603.7b citations are
left alone — in `turns.rs` and `effects/rebound.rs` they annotate genuine
delayed triggers and are correct, and `layers.rs:229` predates this branch.
Annotations only; no behavior change. Full engine suite green (17864 lib,
4134 integration).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* fix(parser): keep the unbound delayed graveyard sweep honestly unimplemented
Review finding (HIGH, @matthewevans): recognizing Grinning Totem's
"until … next upkeep" duration took the card to zero Unimplemented, so it read
as fully supported while its required delayed cleanup stayed nonfunctional —
the unplayed opponent-owned card is stranded in exile forever. Verified by
driving the real activate → search → exile → next-upkeep pipeline.
Per the review, restore an honest marker here rather than half-implementing the
sweep; the parser anaphor/stamping work and the shared delayed-trigger dispatch
bug are split out to their own engine-implementer-scoped PR.
`demote_unbound_delayed_sweeps` runs as a post-lowering invariant beside the
existing `scrub_granting_placeholder_descriptions` degrade net, replacing a
`CreateDelayedTrigger` whose inner chain is a graveyard move on an UNBOUND
anaphor (target still `ParentTarget`/`Any`) with
`Effect::unimplemented("delayed_unplayed_exile_sweep", …)`. A post-lowering pass
rather than one grammar arm because several builders emit
`CreateDelayedTrigger`, and the honesty requirement is a property of the final
tree.
Not a card-name special case — it is the whole impulse-cleanup class:
Grinning Totem, Bank Job, Glimpse the Impossible, Three Wishes, Elkin Lair.
Deliberately narrow, so it cannot demote anything that works: only inside a
delayed trigger (the 77-card "search your library … put it into your graveyard"
wording never reaches it), and only an UNBOUND target — Valakut Exploration's
`ExiledBySource` sweep and Necropotence's tracked-set hand recall are both
untouched, each pinned by a reach-guarded negative test.
Measured over all 35,316 MTGJSON faces, vs the previous head: 5 faces change.
Grinning Totem, Bank Job and Glimpse the Impossible gain the honest marker
(Grinning Totem is now 1 -> 1 against main, so it never flips to supported);
Three Wishes and Elkin Lair were already unsupported and only gain the key.
Bank Job and Glimpse the Impossible were falsely supported on main for the same
reason and now correctly report the gap.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
* docs(PR-6705): correct delayed-trigger CR citations
---------
Co-authored-by: Jonathan Fortin <jonathan.fortin@oraventis.com>
Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* feat(engine): support Yurlok of Scorch Thrash * chore(ci): refresh continuation census * fix(engine): address Yurlok review feedback * fix(engine): defer outer actions during life substitutions * fix(engine): preserve deferred life payment roots * fix: preserve unless-payment cost across resume
…on the ability shell (phase-rs#6733) * refactor(parser): Plan 05b D13 — add OracleNodeIr::Unsupported (infrastructure only) Decision 1, half one. Adds the shared honest-failure residual node and wires it through every consumer. Converts ZERO producers, so the commit is byte-identical BY CONSTRUCTION and shows zero snapshot churn — the T8-A0 staging, repeated. What lands: * `OracleNodeIr::Unsupported { text, min_x_value }`. Two sites in the parser build the residual today (`oracle_class.rs`'s terminal Class fallback and `oracle.rs`'s CR 611.3a "the same is true for …" unqualified-keyword tail) and both produce the same value: `Effect::Unimplemented` with the fixed sentinel `name: "unknown"` and `description` = the residual text, with that text also on the definition's `description`. That pair is load-bearing for coverage, which is why the node carries raw text instead of seeding a chain parse — a chain-parsed residual names whatever clause the chain failed inside, and would break byte-identity on the coverage key. * `oracle::lower_unsupported_node`, which delegates to `make_unimplemented` rather than rebuilding the definition. One construction authority, so the node and the two hand-built sites provably cannot drift. * `min_x_value` on the variant, and this is a soundness requirement rather than a convenience. The residual is emitted through the ability channel, so it lands on `spells_emitted` and occupies a CR 707.9a printed ability slot — which makes it reachable by `raise_last_spell_min_x`, whose `spell_min_x_mut().expect("both spell shapes carry an X floor")` is sound only while every node on that stack can name where its floor lives. A text-only variant would have to answer `None` there and would convert a compile-time-guaranteed `Some` into a runtime panic on a structurally reachable path. The shape it replaces already carried the field as an `AbilityDefinition` root field (CR 601.2b, MagicCompRules.txt:2459), so this preserves a floor rather than widening one. Consumers wired, no `_` arm added anywhere: * `doc.rs::OracleNodeIr::spell_min_x_mut` (exhaustive) — returns the new field. * `doc.rs::OracleDocBuilder::emit` slot accounting (exhaustive) — joins the `spells_emitted` arm. Omitting it would stamp every ability printed after an unrecognized line one CR 707.9a slot low. * `oracle.rs::lower_oracle_ir` bucketing loop (exhaustive) — lower, stamp, push. * `oracle_ir/feature.rs` unit-attribution fold (exhaustive) — explicit no-op arm; the residual is already attributed through the ability id track. * `oracle.rs::lower_spell_node` and `oracle.rs::item_ability` — BOTH carry a `_ => None` arm, so the compiler could not have found them. Found by grepping wildcard arms instead. `lower_spell_node` is the quiet one: it backs `last_ability_definition()`, read mid-loop by `parsed_result_recently_granted_flashback` and by the cross-line "instead" fold's `previous_spell`, so a silent `None` there is a behavior change on a DIFFERENT card from the converted one — which per-card byte-identity cannot catch. `item_trigger` / `item_static` / `item_replacement` keep `_ => None`: the residual is genuinely not a trigger, static, or replacement. `emit_ir_nodes_at`'s `other =>` arm routes it to `emit_at`, which is correct. CR numbers grep-verified against docs/MagicCompRules.txt, text read: CR 601.2b (:2459, announced value of a variable cost), CR 707.9a (:5646, copy gains an ability — the printed-slot rule this file already cites). Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS with every per-file count UNCHANGED (15/4/16/9/2 — no ledger edit, and the doc prose above deliberately avoids the counted token); check-skill-doc PASS. * refactor(parser): Plan 05b D13 — convert U0-62 and U0-02 to the residual node Decision 1, half two. Both hand-built honest-failure residual producers now emit `OracleNodeIr::Unsupported` instead of an eagerly-lowered definition. The residual text is unchanged at both sites, so the coverage key (`name: "unknown"` / `description` = that text) is unchanged; only WHEN the definition is built moves, from the producer to `lower_oracle_ir`. * **U0-62** — `oracle_class.rs`, the terminal Class fallback. **The discard on its second route is deliberate and is preserved exactly.** Two routes reach the fallback: a line no recognizer claimed, and an effect-sentence candidate whose chain DID parse but whose lowered root contains an `Unimplemented`. On the second route the parsed `ir` is thrown away and the residual is rebuilt from `line`. Keeping that partial chain would be a BEHAVIOR change, not a conversion — it would name the failed clause instead of the fixed `"unknown"` sentinel and move the coverage key. `ir` is scoped inside the `if` block, so the discard is structural rather than a convention a later edit could drop. * **U0-02** — `oracle.rs`, the CR 611.3a "the same is true for …" unqualified-keyword tail, via a new `DocEmitter::unsupported_at` that mirrors `ability_at` (no peek mirror to maintain — the ability peek is pop-aware, read from the builder's `spells_emitted` stack). `make_unimplemented` is now private and has exactly one caller, `lower_unsupported_node`. The residual therefore has a single construction authority reachable only through the node; a new residual producer must go through the node rather than minting a definition of its own. Its body also stops hand-constructing an `Effect::Unimplemented` literal and calls `Effect::unimplemented`, the constructor CLAUDE.md mandates — a value identity (the helper expands to exactly that literal), not a behavior change. **Ratchet: which occurrence moved.** The count alone cannot distinguish a converted producer from a prose mention, so both files are named explicitly. * `oracle_class.rs` 4 -> 3, ledger tightened. The occurrence that moved is the terminal fallback push, formerly `OracleNodeIr::PreLoweredSpell(...)` — the U0-62 producer itself. * `oracle.rs` stays at **15, and this is not an oversight.** U0-02's producer called the shared `DocEmitter::ability_at` helper, and that helper — not the producer — is where the counted token textually lives. Converting the producer therefore reduces the count by zero while genuinely retiring a producer: `ability_at` call sites in `oracle.rs` go **18 -> 17**, verified against the parent commit. The textual ratchet is blind to any producer that routes through a shared emitter helper; `ability_at` call-site count is the discriminating measure for this file, and the ledger stays at 15 because the helper legitimately survives for its 17 remaining callers. **Snapshot: exactly one `*_ir.snap` moves, and it is the fixture that carries a residual.** `barbarian_class_ir` had serialized the eagerly-lowered residual definition; it now serializes `Unsupported { text, min_x_value: 0 }`. Same text, same floor, IR-native representation. **No `*_lowered.snap` changes** — and that is asserted, not assumed: `parse_two_layer` asserts `_ir` BEFORE `_lowered` in the same test, so a failing `_ir` masks the `_lowered`. The `_ir` snapshot was accepted and the test RE-RUN, and `_lowered` passes unchanged. **Byte gate, non-vacuously verified.** Filtered regeneration over the exact population that can reach either converted site (38 Class faces + 33 cards whose Oracle text contains "same is true" -> 76 exported faces) is byte-identical to `origin/main` (sha256 8d3b3f87…). The probe is verifiably applied: that output contains exactly the three residuals the census predicted — Barbarian Class and Gourmand's Talent (U0-62, "2 abilities") and Cairn Wanderer's "the same is true for landwalk, protection" (U0-02) — all present and unchanged. Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS (oracle_class.rs lowered, no ledger entry raised); Gate G + Gate A PASS; check-skill-doc PASS. * refactor(parser): Plan 05b D13 — add `optional` to AbilityShellIr (infrastructure only) Decision 3, half one. An ELEVENTH field on A0's shell. A0 chose ten deliberately, so this is an explicit amendment rather than a fill-in, and the field doc says why the amendment is warranted. Converts ZERO producers → byte-identical by construction, zero snapshot churn. **This is the one shell field that is NOT the CR 602.1 activation envelope, and the doc block says so rather than pretending otherwise.** Every other field partitions along the seam CR 602.1 (MagicCompRules.txt:2514) draws — cost before the colon, activation instructions after it. `optional` does not: CR 608.2d (:2795) places the choice *"while applying the effect"*, which is CR 608.2 resolution, the half this type deliberately leaves to `EffectChainIr`. Its presence here is a named exception, paid knowingly. The exception is correct because the mechanical requirement is that `optional` be stamped **unconditionally, after lowering** — exactly what the game-start recognizers (`AbilityKind::Mulligan`, `BeginGame`) do by hand today. The alternative, expressing it on clause 0 and letting assembly carry it up, is NOT equivalent: `assemble_effect_chain` maps `clause_ir.parsed.optional` to the root through four suppressions — `Effect::GrantCastingPermission`, `is_lingering_cast_from_zone`, `is_join_forces_pay_any_amount_mana_cost`, `is_pay_to_end_effect_termination` — and a following arm sets `def.optional = false` outright for `Effect::SearchOutsideGame`. It additionally assumes clause 0 becomes the emitted root, which `ClauseDisposition` does not guarantee. All five guards read from the source, not from memory. The shell is the only place the stamp can be unconditional AND survive the IR conversion. **Applied as a monotone OR, never an assignment:** `def.optional |= shell.optional`, mirroring `cant_be_copied`. Here the choice is load-bearing rather than merely tidy — `lower_effect_chain_ir` legitimately sets `optional` from the printed "you may", so an assignment would let every one of the many producers that build a `default()` shell CLEAR a flag the chain had already established. The OR is what preserves A0's defer-on-default property, which is precisely what makes this widening byte-identical by construction: `AbilityShellIr::default()` remains a no-op, so no existing producer changes behavior and no existing producer had to be touched. `serde` skips the field when `false`, so an unset shell serializes exactly as it did before the widening and `*_ir.snap` fixtures cannot churn. Also corrects the struct's own field-count claim, which this field would otherwise have made staler. Counted from the source rather than from the previous sentence: the struct has thirteen fields, twelve of which mirror an `AbilityDefinition` root field (`stages` is a transform list, not a root field). A0's "10 of 38" counted the fields that tranche ADDED and omitted the pre-existing `sub_link`, so it already read one low before `optional` arrived. CR numbers grep-verified against docs/MagicCompRules.txt, text read: CR 608.2d (:2795, choices announced while applying the effect), CR 602.1 (:2514, the envelope this field is the exception to), CR 707.10 (the neighbouring field's citation, unchanged). Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS with every per-file count unchanged; check-skill-doc PASS. * refactor(parser): Plan 05b D13 — convert U0-43, the CR 103.5b mulligan recognizer Decision 3, half two. `try_parse_mulligan_time_ability` (Serum Powder, No-Regrets Egret) returns an `AbilityIr` and emits through `ability_ir_at` instead of building a definition and emitting through `ability_at`. **By construction, stated as the identity rather than as a corpus argument.** `parse_effect_chain(t, k)` **is** `lower_ability_ir(&parse_ability_ir_standalone( t, k))` — that is the entire body of `parse_effect_chain` in `oracle_effect/mod.rs`, not a claim about it. Splitting it into its two halves moves WHERE the lowering happens without changing WHAT it produces. Both root stamps ride the shell and are applied after lowering, exactly as the two lines they replace applied them: * `description` — an `Option` override, so `Some(line)` reproduces `.description(line)`. * `optional` — a monotone OR against the shell's `true`, which for a `true` stamp is value-identical to the `def.optional = true` it replaces, on every input, whatever lowering produced. `apply_ability_shell_envelope` runs before the stage loop, and this site lists no stages, so the stamps are the last thing that happens either way. No property of any card's text participates in the argument, so a future printing reaching this recognizer is covered too. `parse_ability_ir_standalone` is the mode-pinned wrapper for a site whose original called `parse_effect_chain`; the argument list is unchanged, so the `ChainLoweringMode` is inherited mechanically rather than by reviewer judgment. No `has_unimplemented` gate exists at this site, so none was added — the recognizer emits whatever the chain produced, as before (No-Regrets Egret's inner effect is an honest `Unimplemented` today and stays one). **`clauses[0].parsed.optional` is NOT set, deliberately.** CR 103.5b (MagicCompRules.txt:300) says the player *"may perform that action"* — a property of the whole printed ability. Routing it through clause 0 would subject it to `assemble_effect_chain`'s conditional clause→root mapping (four suppressions plus a `SearchOutsideGame` arm that forces `optional = false`) and would additionally assume clause 0 becomes the emitted root, which `ClauseDisposition` does not guarantee. `ability_at` call sites in `oracle.rs`: 17 -> 16. The textual `PreLowered` count is unchanged at 15 for the same reason commit 2 explained — this producer routed through the shared helper, not through a token of its own. Byte gate, non-vacuously verified over the recognizer's exact population (`--filter "No-Regrets Egret|Serum Powder"`): both cards still export `kind: Mulligan`, `optional: true`, and the full printed line as `description`. Gates: cargo fmt; clippy -p engine --lib -D warnings clean; Gate P PASS; check-skill-doc PASS. * docs(parser): name the third spell node shape everywhere the prose says two Follows the four D13 commits, which add `OracleNodeIr::Unsupported` as a third spell-shaped node. Five comments encode "there are exactly two spell shapes" and became false the moment that variant landed. Two of them are the argument text of live assertions, which is what the next reader takes as the invariant. Prose only — no behavior, no signature, no control flow. * `oracle.rs` `raise_last_spell_min_x` — `.expect("… both spell shapes carry an X floor")`. `spell_min_x_mut` now has three `Some` arms. * `oracle.rs` cross-line "instead" fold — `unreachable!("… both spell shapes lower")`. `lower_spell_node` now lowers three. * `oracle.rs` `item_ability` doc — "both shapes are named here". Three are. Also records why its surviving `_ => None` is a hazard for the *next* spell-shaped variant rather than for this one: it is an `and_then` target, so a swallowed variant is a behavior change with no compile error. Deleting it is deferred to the pre-lowered variants' removal, which rewrites these arms anyway. * `doc.rs` `spell_min_x_mut` doc — "Both spell shapes store the floor … at different layers". There are three layers now: the residual holds text rather than a definition, so its floor lives on the node itself until `lower_unsupported_node` builds a definition to put it on. The interchange argument still holds because every path applies with `max`. * `doc.rs` `stamp_printed_ability_slot` doc — "both spell node shapes converge on one here". Three converge; the residual arrives via `lower_unsupported_node`. Found by a case-insensitive sweep, not by the exact-phrase grep that produced the original list — `both spell NODE shapes` and the capitalized doc line both escape a naive `both spell shapes` match. Two of the five would have been missed. Gates: cargo fmt; Gate P PASS; check-skill-doc PASS; Gate G/A PASS. Both gates are textual and count comment prose, so this commit's own wording avoids the literal ratchet token — the first draft raised oracle.rs 15 -> 16 on a single word in a comment. * refactor(parser): centralize spell payload classification --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…irroring it (phase-rs#6734) * chore(manabrew-compat): add protocol v3 dependency * refactor(manabrew-compat): use upstream protocol DTOs * chore(manabrew-compat): bump protocol version to 3 * feat(manabrew-compat): expose protocol v3 card state * feat(manabrew-compat): restore Serum Powder mulligan extension --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…n back (phase-rs#6736) Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…phase-rs#6431) (phase-rs#6738) Under a CR 723 turn-control effect (Emrakul, the Promised End; Mindslaver), `waitingForActorMatches` only fell back to `gameState.priority_player` for the "Priority" WaitingFor variant. Every other single-actor variant — notably `PayCost`, the prompt Lava Dart's flashback "Sacrifice a Mountain" cost uses — fell through to a bare `fields.player === actor` check, which fails when `player` names the controlled seat rather than the controller actually submitting the action. The queued sacrifice-cost response was misjudged "stale" and silently dropped, matching the reported "I click the Mountain and nothing happens." Extend the `priority_player` fallback to every WaitingFor variant that carries a `player` field, matching the same pattern already used in p2p-adapter.ts and aiController.ts for this exact CR 723 concern. Also adds an engine-level integration test pinning that the Rust cast/cost pipeline already threads the correct semantic player through flashback's Sacrifice cost under turn control — the defect was frontend-only.
…rs#6440) (phase-rs#6737) Resolve AmountSpentToCastSource ceilings from the entering object's mana_spent_to_cast_amount (objects/liminal dual lookup), including Some(0) when never cast. Stop reading PendingSpellResolution.actual_mana_spent so Chord put-into-play cannot open an unconstrained MV filter. Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…U0-45) (phase-rs#6742) * feat(parser): add die result branch IR * refactor(parser): lower die roll tables through IR * docs(parser): pin die results into lower_ability_ir's documented order The ordering doc declares itself the single authority on a sequence that is "pinned and behavior-load-bearing", but the die-result step was inserted into that sequence without appearing in it. Placing it before finalize is a real decision (CR 706.3b: the table is part of the same ability as the roll, so finalize and the anchor must see the complete effect) and was undocumented. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Parse changes introduced by this PR · 345 card(s), 210 signature(s) (baseline: main
|
…t possessive (phase-rs#6526) * fix(parser): bind "the mana value of that spell" of-form (Ovika, Enigma Goliath) Ovika, Enigma Goliath (issue phase-rs#1718) never created its Goblin tokens: the trigger fired but its "create X 1/1 red Phyrexian Goblin creature tokens, where X is the mana value of that spell" effect lowered to Effect::Unimplemented. parse_object_mana_value_ref (crates/engine/src/parser/oracle_nom/quantity.rs) recognized the possessive front-form "that spell's mana value" (-> ObjectManaValue { EventSource } via parse_object_possessive_scope) but the prepositional of-form "the mana value of that spell" required a "target" keyword and errored otherwise, so the where-X token count could not bind and the whole clause dropped to Unimplemented. Accept the non-target of-form by mirroring the possessive scope mapping via a new parse_object_prepositional_anaphor_scope combinator (that spell -> EventSource, that creature/permanent -> Target, this ... -> Source), tried only after the existing targeted TargetObjectManaValue path so "mana value of target creature" is unaffected. * test: annotate the Ovika token haste assertion for the engine authority gate The raw `keywords.contains(&Keyword::Haste)` in the new Ovika token-creation runtime test tripped the Engine authority gate (raw keyword query). It asserts the literal keyword set stamped on the freshly created token, not an effective-keyword query, so annotate it with allow-raw-authority per the gate's documented escape hatch. * test(engine): pin mana-value-derived token characteristics through the cast pipeline Strengthen the Ovika runtime test to assert each created token is exactly 1/1, red, and a Phyrexian Goblin, so a count/P-T axis confusion in the ObjectManaValue { EventSource } binding can no longer pass. Add a Pure Reflection runtime module driving the production cast/trigger path: the created Reflection token P/T must equal the triggering creature spell mana value, covering the P/T axis of the same of-form anaphor. The second case casts a {1}{R} spell funded with red + colorless mana so mana-value summation over colored pips is exercised (CR 202.3). Also correct the mana-pool funding comment citation: 601.2f is total-cost determination; funding/payment are 601.2g-h. --------- Co-authored-by: rsnetworkinginc <rsnetworkinginc@users.noreply.github.com> Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com>
…cument relation (U0-46) (phase-rs#6748) * feat(parser): add inert self-replacement relation fold * feat(parser): lower cross-line self-replacement relations * refactor(parser): drop the emitter pop wrapper the instead-fold owned `pop_last_spell` existed solely for the cross-line "instead" fold's pop-and-rebuild. T10f replaced that fold with `DocumentRelationIr::SelfReplacementOverride` (CR 614.15), leaving the wrapper with no callers — `cargo clippy -p engine --lib -- -D warnings` fails on it. Delete it rather than `#[expect(dead_code)]` it for a later unit: it is a two-line private wrapper over `take_last_spell`, which stays live for `raise_last_spell_min_x`, and git history restores it if U0-47 wants it back. `reemit_node`'s doc argued its `OracleNodeIr` parameter from "the two re-emitting callers legitimately differ" — there is one caller now, so the doc is restated on that caller's own terms and records where the other one went. Tighten the burn-down ledger 13 -> 12 to match. The deleted expression carried one pre-lowered spell token, and the ledger's own contract is that a tranche converting a producer lowers its ceiling in the same commit — otherwise the burn-down is invisible in `git log` on that file and a later change may silently drift back up to the stale ceiling. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* refactor(engine): rename package to phase-engine * docs: retarget `cargo -p engine` invocations at `phase-engine` The package rename leaves every documented `cargo <verb> -p engine` invocation pointing at a package that no longer exists. Retarget the ones that are live instructions — skills, agents, workflow scripts, CLAUDE.md, the contributor guide. Historical plan records under `.claude/wf/` are left alone: they describe commands that were run at the time, and rewriting them would falsify the record. Doc comments inside `crates/engine/src/` are also untouched, to keep this commit free of any .rs edit. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…its (phase-rs#6745) * fix(ai): apply the mulligan card-count floor to every deck, not just cEDH Community report: the AI mulliganed down to 2-3 cards. Only cEDH and Momir decks had any floor at all, so an ordinary deck had none -- KeepablesByLandCount was the sole unbounded ForceMulligan source, and its own hand_size <= 4 stop condition was dead code (under the London Mulligan the engine always presents a 7-card hand at the keep decision, because bottoming happens after). Lifts the floor out of CedhKeepablesMulligan into a universal MulliganCardFloor policy: one authority, not an archetype special case. Retires CEDH_MULLIGAN_FLOOR and the vestigial +5.0 short-hand branch, whose production domain is exactly the floor's band in both free-first regimes -- no gap, no overlap -- so the deletion cannot change a verdict, and ForceKeep strictly dominates the Score it replaced. The floor is an AI strategy heuristic, NOT a rules requirement. CR 103.5 permits mulliganing until the opening hand would be zero cards; CR 103.5c is only the free-first discount. Corrects an over-claiming annotation that said otherwise, and an engine doc comment that said the hard cap was a 1-card hand when the file's own tests show it is 0. The reporter's inferred cause (MDFC lands miscounted as spells) was refuted -- 200/200 modal_dfc entries carry two faces sharing one oracle id, and the live oracle-id fallback satisfies that. The observation was right; the cause was not. ai-gate is structurally two-player 60-card and never reaches a 4th mulligan, so it shows no harm but carries no positive evidence; the unit tests are the whole sufficiency argument. baselines/suite-baseline.json deliberately not refreshed -- concurrent units also move AI behaviour and a shared refresh would destroy per-unit attribution. * docs(ai): correct a false architectural claim in the mulligan floor docstring 286b4c64ea shipped a docstring asserting "no registered policy and no part of evaluate_hand reads card names". That is false: five registered policies read obj.name in production -- landfall_keepables.rs:63, aristocrats:75, plus_one_counters:75, spellslinger:88, tokens_wide:69 and :79, all outside cfg(test) and all in MulliganRegistry::default()'s vec. The conclusion the sentence supported still holds, on a narrower ground that nobody had established: DeckFeatures derives Default and features/ carries no manual impl Default, so every *_names Vec<String> is empty under DeckFeatures::default() and no name comparison can match. That is a guarantee about this fixture's configuration, not about the policy architecture -- and it breaks the moment someone writes a test on non-default DeckFeatures. The docstring now says which guarantee it is relying on. Comment-only; no behaviour change. * feat(phase-ai): value mana development with a fixed-coefficient serve-time offset Community reports: the AI does not play its lands on curve ("played against modern tron and it kept just not playing its lands"), and discards lands while holding one land on the battlefield. Root cause at the eval layer: a land drop's weighted delta is strictly negative for every shipped archetype. The card leaving hand costs `hand_size` (serve-time value up to 2.5 x 2.5 = 6.25 for Combo in the late phase), plus its `zone_quality` hand-castability term and `synergy` dilution, for a binding total of 6.5545 -- and nothing on the battlefield side pays for the mana source the card becomes. Adds `mana_development_offset`: `f(n) = MANA_DEVELOPMENT_COEFF * min(n, MANA_SOURCE_TARGET)`, with C = 7.5 and S = 12. The coefficient is fixed rather than learned, mirroring the existing `energy_offset` pattern: it is applied AFTER weighting in `evaluate_state_breakdown` and is excluded from `EvalFeatures::weighted_total`, so the Texel-fitted weights and the train/serve invariant are preserved and no retrain is required. `mana_source_count` counts renewable sources only (CR 106.1), delegating to `zone_eval::is_intrinsic_mana_source` so that cracking a Treasure, Gold or Lotus Petal (CR 701.21) does not read as losing a mana source. Two consequences are disclosed in the constants' own docstrings rather than left for a maintainer to rediscover, and both are pinned by assertions: - The offset is archetype-invariant, so every archetype receives exactly +7.5 per source and only the counterweight differs. Control's rock-vs-body margin inverts (-3.23 -> +4.27); Midrange, the `#[default]`, does not invert but lands 0.33 away. Combo and Ramp are UNMEASURED, not safe. - At and above the source cap the marginal is exactly 0.0 while the land-drop cost is unchanged, so the reported bug RETURNS in full. Reachable in Commander from roughly turn 10 with 12 sources out and two or more lands in hand. Closing that is the root fix and is deliberately out of scope here; raising the cap trades the flooding defect back in. `mana_development_floor_holds_for_every_archetype_and_phase` recomputes the floor from the real weights and pins both poles, so a retrain or a multiplier change fails loudly instead of silently invalidating the constant. The `effect_kind` sweep lines that a concurrent agent's `TargetSelectionSlot` work required are deliberately NOT in this commit; that feature lands separately and this commit builds against `main` without it. Verification: `tilt` test-ai 1766/1766 passed, 14 skipped; clippy `--all-targets -D warnings` plus `check-interaction-bindings.sh` clean. * fix(phase-ai): tier cleanup discard and sacrifice selection by mana role The AI pitched lands and mana rocks at cleanup and fed its own accelerants to sacrifice outlets, because both seams ordered candidates by a single scalar `evaluate_card_value` that carried no notion of what a card does for mana development. A land and a junk instant of equal raw value were interchangeable. Introduces `card_value` as the single authority for that ordering: - `ManaRole` / `mana_role` classify a card's contribution to mana development (LandDrop, Accelerant, None), and `KeepTier` / `keep_tier` rank cards for surrender against the controller's own plan schedule. - `keep_key` + `cmp_keep` replace bare `evaluate_card_value` sorts at the discard seams; `sacrifice_key` + `cmp_sacrifice` do the same for sacrifice selection; `cost_card_value` prices `PayCostKind` arms through one authority rather than per-arm ad hoc formulas. - The tier is keyed on the DISCARDING player and their board, not the AI's, and reverses under CR 723.5 when the AI is the authorized submitter for another seat. - `fallback_action` now takes `&AiConfig` so penalties reach the seam; `fallback_action_default` preserves the previous call shape in tests. When no plan snapshot is available every card is Ordinary and the tuple comparator degenerates exactly to the previous scalar ordering, so the change is inert wherever the plan is absent. Three tests pin that fail-safe. Behavioural constants that a paired-seed `ai-gate` run must attribute correctly: - `sacrifice_land_penalty` 4.0 -> 4.5. This is a change to a pre-existing CMA-ES-trained default, NOT a consequence of the band rescale. A run that omits it will misattribute the delta. - `SACRIFICE_VALUE_RAW_CEILING` = 30.0, derived from the p99.9 of measured sacrifice-selection raw scores (p50 6.0 / p90 12.5 / p95 15.0 / p99 20.0). - `GUARDED_SELECTION_CARDINALITY` = 7. CR corrections made in passing: three sites cited CR 305.7 (which governs subtype-SETTING effects) for the proposition that a creature-land is a creature and a land simultaneously. That proposition is CR 300.2; the citations now point there. Two CR 305.7 sites that genuinely quote the subtype-setting rule are kept. * feat(phase-ai): gate sacrifice costs on the mana the payment would destroy The AI would pay a spell's sacrifice cost with whatever fodder was eligible, including the mana sources it still needed — feeding artifact lands to Fanatical Offering, Reckoner's Bargain and Lava Dart while behind on mana. Nothing in the candidate scoring asked whether committing to the cost was worth what the payment would cost the mana base. Adds `SacrificeCostManaGatePolicy`, which rejects a candidate whose required sacrifice would consume a mana source the controller's own plan still needs. It reads the same `KeepTier` / `ManaRole` authority introduced for discard and sacrifice ordering, so the two compose cleanly: the gate decides whether to commit to the cost at all, the tier decides which fodder is surrendered once committed. The eligibility computation is NOT reimplemented on the AI side. The policy calls `casting::find_eligible_sacrifice_targets` + `casting::sacrifice_cost_bounds` — the same pair, in the same order, that `cost_payability.rs` uses on the real payment path. Both are widened to `pub` for this; `casting` already exports three other `pub` fns consumed by phase-ai. An AI-side copy would have to re-derive the `player_cant_sacrifice_as_cost` static check and would over-count fodder under Yasharn or Angel of Jubilation, which is precisely the divergence the engine owning this logic prevents. Three test helpers move verbatim from `search.rs` into a new `test_support` module. Scope of the evidence, stated because a green run here does not mean what it looks like: `SacrificeCostManaGatePolicy` is unreachable in the ai-gate duel suite. Of the 120 cards in `duel_suite/inline_decks.rs`, zero carry a gated cost shape (measured, with Fanatical Offering / Lava Dart / Dread Return as positive controls), so the suite's exit-0 is evidence the gate is inert there, not that it is safe. The 17 unit tests are the whole of its coverage; F6 drives the production aggregation through `PolicyRegistry::shared().priors(..)`. The flashback-arm `at_root()` perf contingency is untriggered and unmeasured: the ai-gate report emits no wall-clock figures and the suite contains no flashback card. Recorded as an open risk rather than a pass. Candidate generation already walks `effective_flashback_cost` once or twice before a graveyard candidate can exist, so the added walk is amortized, but that is an argument and not a measurement. * feat(phase-ai): value mana development as a differential, not an absolute count Unit 1 gave the evaluator a fixed-coefficient mana-development offset keyed on the AI's own source count. That closed the "AI ignores its mana base" half of the reported problem and opened a narrower one: because the term took no opponent input, it saturated on an absolute count. At MANA_SOURCE_TARGET sources — an ordinary turn-10 Commander board, not a corner case — an additional source scored exactly zero marginal, so a land drop cost the AI a card for nothing and the evaluator preferred to keep the card. The offset is now signed and relative: f(d) = C * clamp(d, -S, +S) d = self_sources - opponent_aggregate Marginal is exactly C for |d| < S and exactly 0.0 beyond, so unilateral development still cannot be farmed — but it saturates on the RELATIVE axis. At 12-vs-12 sources a land drop moves d from 0 to 1 and earns the full C, which is the regression Unit 1 disclosed and this closes. An opponent ahead on mana now yields a negative term rather than the previous unsigned reading of zero. With one opponent the aggregate is a plain average. With two or more it is threat-weighted (sum of w_i * sources_i, w_i from threat_level), matching how card_advantage_breakdown already aggregates the other board_stats-derived count. board_stats returns a BoardStats struct rather than a 4-tuple. The four fields are all i32, so the previous positional reads (.1 for power) were a transposition waiting to happen; call sites now name what they want and the compiler checks them. plan/mod.rs::controlled_mana_sources was a deliberate byte-identical copy of the then-private eval::mana_source_count, carrying a comment instructing a future commit to dedup it. That instruction is honored here: the body delegates to board_stats(..).mana_sources and the comment records the discharge. Scope of the evidence, stated because a green gate here does not mean what it looks like: - cargo ai-gate is exit 0 with 0 FAIL / 3 WARN, and the WARNs are NOT readable as this change's effect. suite-baseline.json predates Unit 1, so every delta is confounded across all five units plus concurrent work on main. All three matchups are mirrors, where 50% is the null and n=10 has wide variance; neither sign test is significant (p = 0.34, p = 0.14). The single baseline refresh is deliberately deferred so it happens once, after this lands. - MatchupSpec has exactly two seats, so the gate exercises the averaged <=1-opponent branch ONLY. It structurally cannot reach the threat-weighted branch or the threat-weight channel below, both guarded on opponents.len() >= 2, nor the Commander regime this unit was written for. Their only coverage is unit tests. A green gate is necessary, not sufficient. Two accepted costs, disclosed rather than fixed: - R8, the threat-weight channel. In the threat-weighted branch a candidate that changes an opponent's threat level moves the mana term with no mana source changing hands, and the normalization is sign-inverting: lowering a mana-rich seat's threat raises the evaluator's mana score, lowering a mana-poor seat's threat lowers it. card_advantage_breakdown has the identical channel, so this is the house pattern rather than a new exposure, but the magnitude is larger because C carries no fitted weight. Pinned by threat_reweighting_moves_the_mana_term_without_a_source_changing_hands. The root fix (teach threat_level about mana development) is circular with the aggregate it would feed and is larger than this unit. - R6, mutual saturation. A 13-source lead returns the marginal to 0.0, so the leader's land drop scores negative. Far rarer than the absolute-form cap it replaces, which triggered at 12 sources outright. HarvestMeta.schema goes 2 -> 3. The bump is provenance only: the column keeps its name across the semantic change, so train_eval_weights.py's defaulted_rows warning does not fire on a mixed pre/post corpus and this is not a pooling guard. Recorded so a human reading a shard's meta line can tell which semantics it carries; the real repair is a column rename or a trainer-side minimum-schema check. Discrimination was demonstrated by applying three mutants to the real tree, compiling, and recording which rows went red — not asserted. Mutant A, the half-migration clamp(d, 0.0, S), reds a different row set than Mutant B, the true revert, which is what distinguishes the two failure modes. * test(engine): prove Notion Thief's controller actually draws at runtime Notion Thief's only coverage was a parser assertion that `execute.is_some()`. That is true of any non-empty chain — including one whose no-op head never reaches its `sub_ability` — so it proved the parser produced something and nothing about whether anyone draws. The card's real behavior was unverified in both halves. The card parses into a two-link chain: the head is `Effect::Unimplemented { name: "draw" }` for "that player skips that draw", and `sub_ability` is `Effect::Draw { count: Fixed(1), target: Controller }` for "and you draw a card". The head being non-`Draw` is exactly what makes `draw_is_substituted_away` zero the opponent's draw count, so the `Unimplemented` is load-bearing rather than debt — replacing it with `Effect::Draw` would stop the pre-zeroing and produce a double draw. Nothing recorded that, and nothing tested it, so the annotation was one plausible cleanup away from a regression. The open question was whether `resolve_ability_chain` continues past the no-op head into `sub_ability`. It does. Answered by execution, not by reading: the controller's hand goes +1 off their own library while the opponent's hand and library are untouched. Four tests, in `tests/integration/` so they share the existing binary: - a no-Notion-Thief control, so the redirect test's deltas cannot come from a draw that never happened; - the redirect itself, asserting both halves independently and asserting its own preconditions (phase is not Draw, active player is the controller) so it is provably outside the card's excluded window; - both directions of the `ExceptFirstDrawInDrawStep` gate in one turn — first draw exempt, second draw in the same step redirected; - a mutation probe that severs `sub_ability` on the live object and pins the mutant world (opponent still loses the draw, controller gains nothing), asserting its own removal took effect so it cannot pass vacuously. The probe is a test rather than a transient revert because this is a shared checkout — a deliberate engine mutation would have shown other agents a red `test-engine` on a tree they are also building. Keeping it as a fourth test makes the discrimination permanent instead of momentary. CR 614.1a: "instead" makes this a replacement effect. CR 614.6: the replaced draw never happens. CR 121.1 + CR 504.1: the exception is the active player's turn-based draw, which the card must leave alone. * fix(phase-ai): veto certified-losing self-cost trades, and stop pricing a tapped body as cheaper to give up The AI fed its whole board to Baron Bertram Graywater — `{1}{B}, Sacrifice another creature or artifact: Draw a card` — one permanent at a time until nothing expendable was left, then took its commander. Two defects sat on top of each other, and neither was the one the code appeared to have. SelfCostValuePolicy computed the price of the permanent about to be surrendered and then discarded it whenever the payoff was non-trivial. The gate was binary, never a comparison, and a draw is unconditionally non-trivial — so "sacrifice a 12/12: draw one card" scored identically to "sacrifice a 1/1 token: draw one card". Meanwhile FreeOutletActivationPolicy answered the same question a second time, with a cliff at 4.0 against a cost scan that walked creatures only: blind to the ability's artifact leg, blind to `Another`, and rewarding the drain at +0.5 rather than merely failing to stop it. The comparison now lives in one place. `appraise_benefit` walks the effect chain once and returns a typed appraisal — trivial, priced, or explicitly unpriceable — and the cost side binds through the filter-faithful give-up authority. The crude duplicate is deleted; FreeOutletActivationPolicy is narrowed to what its name says, scoring genuinely free outlets by death-trigger payoff and opting out entirely below the aristocrats floor. Two things measurement changed about the design, both of them load-bearing. A graduated penalty does not restrain a sampler. The decision is a softmax sample at temperature 1.0, and a batch runs ~100 priority windows. Pricing the trade correctly halved the per-window probability of activating — 63.9% to 28.3%, and the root argmax became Pass on both test boards — and the board drained exactly as before, because across 100 windows any finite negative is near-certain eventual selection while fodder remains. Only -inf is categorical. So the underwater arm emits a Reject, and it does so with no threshold: it fires on the sign of a fully-priced net, after eight exemption rungs have each declined to stand the comparison down. There is no constant to tune, which is the difference between this and the 4.0 cliff it replaces. And the give-up price was inheriting a discount that does not apply to it. `evaluate_creature` subtracts a tapped penalty — correctly, for board evaluation, where a tapped creature cannot block or attack. `sacrifice_cost` called it and inherited that discount, which asserts that sacrificing a tapped creature destroys less permanent than sacrificing an untapped one. It does not. Attacking tokens end tapped, so a 1/1 priced at 1.0 against a 1.0 draw, landed exactly on the inclusive covers-cost boundary, and the entire drain flowed through an arm the design had blessed — never reaching the underwater arm at all. A new `evaluate_creature_intrinsic` prices the permanent, not the turn; board evaluation keeps its discount, pinned by a negative control. Measured, four fixture arms, both search states, against the recorded pre-fix baseline of five activations and a lost commander: tokens search on/off -> 0 activations, board intact, commander safe 3/3 search on/off -> 0 activations, board intact, commander safe Clues search on/off -> all five cracked, then the drain stops at the commander rather than at fodder exhaustion The Clue arm is the one that matters most: it proves the veto does not leak into trades that pay for themselves, and because Clues run out it drives the board to the exact state where the commander is the cheapest legal target and shows the economics declining it. Two reach guards keep it honest — every Clue must be cracked, and the batch must end naturally rather than at the action cap, since either shortfall would let the commander assertions pass without the boundary ever being offered. Scope of the evidence. `cargo ai-gate` is exit 0 with 0 FAIL, but its baseline predates this campaign so the two WARNs are confounded across every unit plus concurrent work, and MatchupSpec has exactly two seats — it cannot see the Commander regime the report came from. The single suite-baseline refresh is deliberately deferred to after this lands, since both changes here alter sacrifice pricing and selection. The commander is safe here because the trade is priced, not because anything values a commander. Nothing in the AI does; a cheap commander would still sort as ordinary fodder. That is tracked separately. * chore(phase-ai): refresh the duel-suite baseline after the AI land campaign The previous baseline was taken on 2026-06-17, before the first unit of this campaign landed. Every reading against it since has been confounded across six AI behaviour changes plus unrelated concurrent work, which is why Unit 5's gate run reported three WARNs that nobody could attribute to anything. Refreshing it was deliberately deferred four times so it would happen exactly once, after the last behaviour change landed. Final comparison against the stale baseline before overwriting it — 0 FAIL, 2 WARN, 1 PASS: affinity-mirror 20% -> 40% flips W->L 1, L->W 1 sign p = 0.5000 PASS enchantress-mirror 50% -> 40% flips W->L 3, L->W 2 sign p = 0.3438 WARN red-mirror 70% -> 40% flips W->L 5, L->W 2 sign p = 0.1445 WARN Neither WARN is significant, and neither is attributable. All three matchups are mirrors, where the null is 50% and n = 10 per matchup gives very wide variance. Read red-mirror's 70% -> 40% carefully: 70% was further from the mirror null than 40% is, so the honest statement is that neither value is distinguishable from a coin flip at this sample size, not that a regression occurred. Pooled across all three, 12/30 against 0.5 is p ~ 0.36. What the refresh buys is the thing this campaign never had: a reference point where a delta means one unit's effect. Units B and C are queued and are both AI behaviour changes; measured against the old baseline their results would have been unreadable in exactly the way Unit 5's were. What it does not buy, stated because a green suite reads like more than it is: - MatchupSpec has exactly two seats, so the suite exercises only the averaged <= 1-opponent branch. It cannot reach Unit 5's threat-weighted aggregate, its disclosed threat-weight channel, or the Commander regime that three of the six originating bug reports came from. That gap is tracked separately and is not closable by any baseline. - The refresh enshrines current behaviour as the reference. If a landed unit regressed something the fixtures do not cover, this makes it invisible rather than detectable. The six sacrifice-outlet arms landed with Unit A all measure what they should, which bounds but does not eliminate that risk. - Harvest shards recorded before and after Unit 5 pool without warning, since the mana_development_offset column kept its name across a semantics change. Any training corpus spanning that boundary needs the column renamed or a trainer-side minimum-schema check first. * fix(phase-ai): price a draw at what the pipeline will actually deliver The AI fed permanents to Baron Bertram Graywater — `{1}{B}, Sacrifice another creature or artifact: Draw a card` — while an opponent had Notion Thief out. Every activation paid a permanent and {1}{B}, drew the AI nothing, and handed the opponent a card. Unit A taught the policy to compare cost against benefit; the comparison was still being fed a benefit that did not exist. `effect_benefit_value` priced `Draw{Controller}` at count x one card unconditionally. The engine already knows better: `can_draw_at_least_one` answers whether a draw right now would actually put a card in a player's hand, delegating each leg to the authority that owns it — `allowed_draw_count` for `CantDraw` and exhausted per-turn limits, `select_cards_to_draw` for an empty library, and `proposed_draw_survives_replacement`, which shares its applicability and substitution classifiers with the live pipeline. It was built for exactly this preflight and `DrawPayoffPolicy` already delegates wholly to it. The Draw arm now consults it, and prices a removed draw at 0.0. Priced zero, not `None`. `None` is `BenefitAppraisal::Unpriced`, which stands the whole comparison down and ALLOWS the activation — the opposite of what the engine has just certified. The zero is not a heuristic and cannot under-price a real draw: an optional replacement is never assumed to apply. The end-to-end arm is the part worth reading, because the plan for it was falsified by measurement. Built as specified — Clue board, opposing thief, assert zero activations — it failed post-fix at five. Instrumenting every activation with the state it was decided in showed all five taken with `can_draw_at_least_one == true` and the thief gone from the battlefield: it is a 3/1 on a board that plays real combat, it dies partway through, and after that cracking Clues for cards is correct play. The fixture had stopped measuring its own premise. Granting indestructible was measured insufficient — the opponent decks out before the run ends, plausibly accelerated by the cards the thief steals — so a final-state guard is the wrong shape entirely. The instrument is now per-decision. `suppressed_activations` counts cracks taken in a window where the engine had already certified the payoff would not arrive; that is the quantity this change governs, and it goes 5 of 5 pre-fix in both search regimes to 0 post-fix. `suppressed_windows` — AI priority, fodder still on board, payoff dead — is asserted `>= 1` so the zero cannot pass by the opportunity never arising. The raw activation count is deliberately unasserted, and the fixture says why: it mixes suppressed windows with the correct-play windows after the suppressor leaves. Unlike its thief-less control, this arm escapes the saturation ceiling recorded with Unit A. A vetoed candidate has softmax weight exactly zero, so the categorical zero is a prediction any single suppressed activation falsifies, not a ceiling five fodder can reach by exhaustion. A census of the crate was measured rather than read: the production edit alone was applied and one full `test-ai` cycle read. Seven pre-existing tests went red and four more were green on a premise that had become false — three and one more than the most careful read had found. All eleven now build a seeded library through one helper, so each stated premise is true rather than annotated as false. The deliverability check is a live predicate, and one row now pins that. `draw_price_follows_the_suppressor_leaving_within_one_state` scores the same source twice inside a single `GameState` — suppressor live, then suppressor moved to the graveyard — through a helper that shares one `AiContext` and `AiSession` across both verdicts, so a memo parked at decision, game or process scope has somewhere to go stale. Both directions were watched go red rather than predicted: a process-global memo reddens the row even before the fix, and a session-scoped one is green before the fix and red after, which is the finding. The one memo home the row cannot catch — a field on the policy value itself, which production holds process-global through `PolicyRegistry::shared()` while the row rebuilds it per call — is recorded at the row instead of left implied. The per-window suppressor-liveness conjunct reads `obj.zone` against the same zone set as the replacement candidacy gate; membership in the battlefield vector alone is a no-op for that gate. Reading `obj.zone` is wider than the gate in one direction, because the gate also excludes liminal sources, so the fixture discloses the measured delta — zero here, since nothing on this board occupies the command zone — rather than arguing the widening is harmless. The probe numbers behind the conjunct are labelled by kind at the fixture: which are measured, which follow from the types, and which were taken on a shrunk-library probe build and so are not the shipped configuration's counts. Scope of the evidence. `cargo ai-gate` is exit 0 with 0 FAIL and zero flips on every matchup, which reads as no-regression and nothing more: the gate is 2-player and its decks field no draw suppression, so the new branch is never taken there. The measuring instrument for this change is the fixture above. Disclosed and unchanged: pricing is binary rather than count-aware, so a `draw(3)` under a limit with headroom 1 still prices 3.0 — over-pricing, which can let a marginal crack through but can never forbid one that pays. Cast-seam draw bonuses (`card_advantage`, `etb_value`, `card_hints`, `best_proactive_cast_score`) remain ungated; they are unreachable from the activate path this fixes and are tracked as their own measured unit. * feat(phase-ai): price an owned commander at what it costs to get it back The AI fed its own commander to a sacrifice outlet. It was not a tie-break that went badly — it was an exact tie. Measured before the change: an owned 4/4 commander and an ordinary 4/4 bear both price 10.0 through `evaluate_creature_intrinsic` and produce identical `(Ordinary, 10.0)` sort keys, so the softmax over `SelectCards` candidates gives each a probability of 0.5. The policy had no way to tell them apart, because the quantity it measures does not distinguish them. A commander is the one permanent whose loss is recoverable. CR 903.9a lets its owner move it to the command zone instead of the graveyard, and CR 903.8 charges {2} for each previous time it was cast from there. So its replacement price is what recasting costs — mana value plus the accumulated tax — and that is a different quantity from its board presence, not a correction to it. The premium is therefore added, never substituted: `GameObject::mana_value_on_battlefield_exit` plus `commander_tax`, and the bear stays at 10.0 while the commander goes to 16.0. The mana value has to come from the face the permanent will have once it leaves. CR 712.8a gives a double-faced card only its front face outside the battlefield, CR 708.2a gives a face-down permanent no mana cost at all, and CR 710.1c leaves a flip card's cost unchanged. The engine already reverts those statuses on the way out, so the method reads the same stash `zones::apply_zone_exit_cleanup` restores from, gated per flag rather than on a single disjunction. It lives on `GameObject` because it is a face rule that happens to have a commander as its first caller — filing it under commander would name it after the card instead of the class. It is documented battlefield-exit-only: one transition is proven, and a destination parameter with one proven value is speculative generality. The partition is the part worth reading. A repurchase price is only owed where the permanent is actually surrendered, and five ledger rows were charging it for permanents that are merely used: crew, station, `ReturnToHand`, `RemoveCounter`, and `TapCreatures`. Tapping a commander to crew a Vehicle gives up nothing, and CR 903.9b leaves a bounced commander command-zone-bound and in no case lost. So `sacrifice_cost` is now a wrapper — `permanent_board_value` plus the premium — and those five rows call `permanent_board_value` directly. The extracted body is the previous `sacrifice_cost` byte for byte; no coefficient moved; the premium is added in exactly one place. This is the distinction `payment_cost`'s own docstring already drew in prose, promoted to a function boundary. Two of those five are the ones that mattered least by size and most by effect. The crew and station rows scale by 0.05, so the leak there is +0.30 against the +3.00 of `ReturnToHand` — but they are also the only two whose consumers return an absolute score into `PolicyRegistry`'s sum rather than a comparison among sibling candidates, and +0.30 lands exactly on `NUDGE_MAX`. Magnitude was the wrong axis; what matters is whether the number ends up in a comparison or a total. The seven-card guarantee in `SacrificeValuePolicy`'s docstring was falsified by this change and is now a formula. The land guard holds while a selection's raw magnitude stays under `SACRIFICE_VALUE_RAW_CEILING`, which with one owned commander priced `C` means `ceil((34 - C) / 4) - 1` cards: five for a four-drop cast once, three for a ten-drop at heavy tax. There is deliberately no single integer, and the pin now measures the bound from a live `sacrifice_cost` call rather than restating a constant. Coverage sits at the seam the report came from. The rate assertion drives `SacrificeValuePolicy::score` and `verdict` through a real `PolicyContext` over `WaitingFor::PayCost { Sacrifice }`, and the comparable-body arm asserts the outcome literally: the bear scores -10.0, the commander -16.0, and the selection the AI gives up is the bear. `pick_lowest_value_sacrifices` is asserted in both input orders, because a stable sort over an unbroken tie returns whichever element came first and one order cannot distinguish an ordering from a lucky input. The saturation arm compares a policy-computed -30.0 against the ceiling constant, so raising that ceiling reddens it — which is the point, since raising it is the anticipated follow-up. Disclosed and unchanged. The premium is finite, so a body worth more than `base + premium` still outranks the commander; giving up a recastable commander rather than a large irreplaceable creature is defensible play and is documented rather than asserted against. `blight_value` keeps the full price because a -1/-1 counter surrenders the permanent only at toughness <= 1 (CR 704.5f) — a partial case that wants a branch-level split, tracked separately. Tempo is still unpriced, a known direction-safe underestimate. * chore(ci): re-freeze the Draw-replacement producer census Unit B's Notion Thief repro adds a fixture constructor that builds an opposing Notion Thief, which is a Draw replacement by construction — so it is a new producer and the frozen list has to say so. Plan 03 pins this surface because a Draw replacement's CR 121.2 scope is declared at construction and cannot be recovered afterwards, which is exactly why the gate demands a human look before re-freezing rather than regenerating on the fly. Reviewed: the regenerate is a pure single-line addition. No existing row moved, was reordered, or lost its scope, and the new row's required scope is 1 — the fixture replaces one player's draw, not a global one. * fix: address AI mana decision review feedback * fix: make review regressions testable * fix(ai): prevent stranding demanded mana colors --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* refactor(parser): carry unsupported residuals as IR * refactor(parser): retain nom dispatch results as IR * fix(parser): document dispatch IR size tradeoff * test(parser): assert residual descriptions explicitly * test(parser): update unsupported IR snapshots --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ase-rs#6393) (phase-rs#6741) * fix(ai): halt empty NamedChoice escape and refuse DB-less restore (phase-rs#6393) CardName prompts keep options empty and synthesize from all_card_names; fallback must use legal_actions, and the client must not fabricate PassPriority (or soft-restore without a card DB) after relaunch softlocks. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(ai): engine-owned escape action and shared restore DB guard (phase-rs#6393) P2P host resume now shares restoreState's hard card-DB requirement. Non-Priority AI escape dispatches getAiFallbackAction (WASM fallback_action) instead of inventing from legal-action list order. Co-authored-by: Cursor <cursoragent@cursor.com> * fix(PR-6741): await fallback host resume --------- Co-authored-by: Cursor <cursoragent@cursor.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…ase-rs#6405) (phase-rs#6740) * fix(engine): spill unmatched colored cost reductions into generic (phase-rs#6405) Aang, Master of Elements ("Spells you cast cost {W}{U}{B}{R}{G} less to cast. (This can reduce generic costs.)") wasn't reducing generic mana at all. `apply_shard_reduction` removed a matching colored pip when present but silently dropped any reduction unit with no match, instead of spilling it over to reduce generic mana per CR 118.7b/c. Fixed the shared helper (used by both the general ModifyCost::Reduce path and the Defiler cost-reduction path) and added coverage for the spillover, excess-beyond-match, and mismatched-color-never-touched cases. * fix(engine): address review — CR citations and Defiler consumer coverage - Correct CR annotations that mislabeled the "no matching colored/colorless component" and "excess reduction" spillover cases as CR 118.7a (which only governs generic-mana reductions). Those cases are CR 118.7b (absent type) and CR 118.7c/d (excess), per docs/MagicCompRules.txt. - Add two consumer-level regression tests driving handle_defiler_payment (not just the private apply_defiler_mana_reduction helper) that assert the post-payment pending cost after an unmatched colored reduction shard and after an excess reduction beyond the matching component, so a future regression that decouples the two can't hide behind the existing matching-only Defiler test. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Resolve the generated parser-backlog count conflict using current main values. Co-authored-by: @Lcola98 <75585494+Lcola98@users.noreply.github.com>
Silverflame Ritual's Adamant rider grants a continuous static effect
("creatures you control gain vigilance") rather than an ability-level
effect, so its gate lands on `StaticDefinition.condition` (CR 613 layer
6) instead of the ability's own `condition`. The CI parse-diff bot reads
the ability level only, so that layer move renders as
`conditional: 3+ White spent -> ∅`, which reads like a dropped gate --
the exact bug class this file guards.
It is not a drop: the condition is present and enforced. Verified at
runtime in both polarities, because nothing pinned this subclass. The
existing R8 rider guard (Slaying Fire) cannot catch it -- that payload
is ability-level -- so a future refactor really could drop the static's
condition with every other test here staying green.
Both new tests carry a positive reach-guard asserting the card's UNGATED
first line resolved (the +1/+1 counter), so the "no vigilance" negative
cannot pass vacuously on a spell that never resolved.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
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