Fix Coalition Relic zero-counter mana prompt - #1
Closed
traemyn wants to merge 15 commits into
Closed
Conversation
* fix(coverage): traverse direct effect payloads * fix(coverage): make direct payload traversal exhaustive --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* feat(draft): reveal sealed packs before deckbuilding * chore(changelog): publish entry phase-rs#189 — Sealed packs and Marvel heroes * fix(draft): address sealed pool review feedback * test(draft): complete pool view fixture * test(draft): cover grouped pool transport --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…hase-rs#7186) `split_cross_subject_event_compound` decides whether "Whenever A or B <verb>" is two events or one event shared by two subjects. It gated only the SECOND half on containing an event verb, so a subject disjunction whose second subject happens to be followed by an active-voice verb was split as if it were a cross-subject compound. The leading subject was then stranded in a trigger with no event at all — an `Unknown("Whenever ~")` husk — and its leg silently never fired. Ironsoul Enforcer ("Whenever this creature or a commander you control attacks alone, return target artifact card from your graveyard to the battlefield") is the clearest case: "a commander you control" is followed by "attacks", so the line split into a dead husk plus a working commander leg, and the Enforcer's own attack never triggered. Add the symmetric gate. CR 603.1: a genuine cross-subject compound carries an event on BOTH sides of the "or" (Norin the Wary: "a player casts a spell" / "a creature attacks"); a subject disjunction has a bare noun phrase on the left. `parse_trigger_subject` already folds the latter into a single `TargetFilter::Or` subject, which is the CR-correct shape. The gate uses the WIDE `parse_event_head_start` deliberately, so a state-change or passive leading leg ("becomes tapped or a creature you control attacks") is still recognized as an event and still splits. Measured over the MTGJSON corpus this recovers the dropped leading leg on seven cards across four event families — Ironsoul Enforcer (Attacks), Campsite Cuisine / Shipwreck Sifters / Long Feng (ChangesZone), Calix and Syr Carah (DamageDone), Surrak (BecomesTarget) — and splits zero lines it split before. Donna Noble and The Bus Runner never reach this arm and are unaffected; Norin the Wary still splits into two triggers. Not fixed here: Leovold / Rayne / Parnesse / Unsettled Mariner lose the husk but their "you" (player) leg is still dropped by the subject parser, so only the permanent leg fires. Tests: parser AST shape, production-path no-split for two distinct event families (Attacks + ETB), and a four-case runtime test covering commander attacks alone / source attacks alone / two attackers / unflagged creature. Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
…hase-rs#7101) * fix(ai): stop proposing strategically vacuous loop-shortcut Shortens `smart_shortcut_response` documented "a way to break the loop" but called `has_meaningful_priority_action` — the auto-pass gate, a strictly weaker predicate it was never meant to reuse. On a real 4p board a seat holding only a basic-land fetch answered Shorten against a mandatory trigger cascade it could not touch: rules-legal under CR 732.2b, strategically vacuous, and paid for with a real priority window. Adds a stage-2 efficacy classifier (`WindowReach`) as a fourth orthogonal axis: controller-relative confinement. A seat whose only actions write inside its own resources Accepts; a seat that can reach past them still Shortens. The wildcard arm is deliberately `MayInterfere` — the opposite of `ability_scan`'s default — because a wrong Accept can lose a game while a wrong Shorten costs a beat. Stage 1 is behaviorally unchanged. No CR licence is claimed for stage 2: CR 732.2b is unconditioned and CR 732.2c requires only a different choice. This is AI policy, and the module doc says so. Stage 2 must classify every action stage 1 counted as meaningful, and at first it did not. Stage 1's second disjunct, `has_activatable_sacrifice_for_mana`, reads the state rather than the action list, and sacrifice-for-mana activations never reach that list — so a seat whose only meaningful action was one of them cleared stage 1 and then had stage 2 fold over a list that did not contain it. A confined remainder meant Accept where the shipped predicate returned Shorten. That is not a wrong classification but an absent one, and a fail-closed wildcard cannot cover a shape it never receives. `stage_two_action_set` is that union, built from the same predicate stage 1 uses so the two cannot drift apart, and a row asserts the membership independently of any verdict so a later reclassification cannot silently retire the invariant. `smart_shortcut_response` serves two prompt shapes, so stage 2 applies at the precast-copy window as well as the ordinary one — the same question deserves the same answer — and a row drives that window through production `ai_support::candidate_actions` rather than leaving it inferred. The `predicted_winner` read is structurally absent there: that variant carries no `proposal` field. The two shortcut windows and the tests' reach-guards share one probe recipe (`ai_support::shortcut_probe`) rather than parallel copies. At base the recipe was inlined at two production sites and no test-side guard existed; early revisions of the new guards drifted, evaluating `has_meaningful_priority_action` against the caller's prompt state, whose `has_activatable_sacrifice_for_mana` rung is gated on `waiting_for == Priority`, so a guard could read false where production reads true. Sharing the recipe removes the class. The second copy is NOT unified: `game::engine::no_living_player_has_meaningful_priority_action` still inlines the same seven statements. Only the shared probe documents the mirroring; that second copy carries no back-reference, so a reader standing there has no pointer to it. Routing it through the shared probe would touch the CR 732.5 mandatory-loop firewall — both its production callers are mandatory-loop gates: the `LoopDetectionMode::On` auto-win block, whose own comment calls it the "entire soundness firewall", and `interactive_loop_bridge`'s `mandatory` binding. That is outside this change's scope, so it stays a follow-up rather than being claimed as done. The confinement guarantee is scoped to what it can actually promise — never a false `Accept` from a shape it does not recognize — and the doc now names the residual it cannot cover. A parser-swallowed clause makes an ability look strictly MORE confined, which is the direction that loses games. Measured on `Invoke Justice`, whose "distribute four +1/+1 counters … target player controls" clause is dropped with no `Unimplemented` marker, leaving a lone graveyard-recursion effect that classifies as confined. `SearchLibrary`'s arm gains a real fold-absorption witness (`Haunting Echoes`, whose `SearchLibrary { target_player: None }` sub-ability classifies confined on its own and is absorbed by its sibling graveyard-exile head). The two library rows already present are both caught by `target_player` directly and never exercised the fold they were cited for. Acceptance rides the real dump (844846 B, sha256 9843d5165cbbf7dd7bca4171c7888c190b7eba7e52a2ed095b44ff76fadd7886) driven through the production restore and `apply()`, not a synthetic scenario. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(ai): stop classifying mana production as a confined own-resource window Stage two classified `Effect::Mana` as `WindowReach::OwnResourcesOnly`, so a seat whose only answer was a ritual -- or an actor-owned sacrifice-for-mana source -- was judged unable to interfere, and the AI accepted a loop shortcut that surrendered a live out. `ShortcutResponse::Shorten` hands the responder a real `Priority` window; inside it, mana is fungible reach. The defect was a CR 106.4 quotation truncated before the clause that refutes it. The arm quoted "that mana goes into a player's mana pool" and concluded board-neutrality implies non-interference. The rule's next sentence is "From there, it can be used to pay costs immediately". Board-neutrality and non-interference are different propositions; a correct citation was attached to the wrong one. `Effect::Mana` now falls through to the fail-closed `_ => MayInterfere` wildcard. The allowlist drops from four shapes to three. The *spending*-side `AbilityCost::Mana` arm deliberately stays -- paying a cost consumes reach, it does not create it -- and the arm now says so, so a future reader does not read its survival as an oversight. Regressions on the real 4p dump, both required by the maintainer: v10a a cast mana spell (Dark Ritual) funding an otherwise-unaffordable Bolt v10b an actor-owned sacrifice-for-mana seat (Lotus Petal, Composite[Tap, Sacrifice{SelfRef}]) re-admitted through `stage_two_action_set` Revert probe executed: restoring the deleted arm flips exactly 3 of 20 rows (the unit row, v10a, v10b) and leaves 17 green -- including both ACCEPT arms, the flagship, v9b, and the positive control -- then the files were restored and verified byte-identical. Two fixture facts are load-bearing and documented at their sizing sites: Lotus Petal's capacity is exactly 1 (`AnyOneColor` yields `vec![type; amount]`), leaving a 1-mana margin over P2's cheapest alternative; and any staged P2 source contributing 2 or more unlocks Angel of the Ruins' plainscycling, which would silently destroy v10b's attribution. Both assertions fail loudly instead. `give_bolt_with_cost` seeds `base_mana_cost` as well as `mana_cost`: `seed_live_characteristics_from_base` reseeds `mana_cost` from the base on every layer pass, and every consumer reaches the object through `shortcut_probe`, which flushes layers -- so setting only `mana_cost` would have made the "otherwise-unaffordable" premise measure nothing. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(ai): correct three justifications the mana-reach review measured as false Review-impl returned Semantic-Impact PASS and Maintainer-Simulation PASS on the parent commit; these are its three [LOW] and two [NIT] items. No classifier logic changes -- with comments stripped, the `shortcut_efficacy.rs` delta is a single assertion-message string. - `give_bolt_with_cost`'s rationale for setting `base_mana_cost` was false for the objects it stages. `reset_recipient_to_base` is reached only over `battlefield_phased_in_ids()`, and the incremental arm's recipient set is battlefield entrants plus attachment hosts -- neither reaches a hand object. `sync_missing_base_characteristics` back-fills base FROM live, the opposite direction. The code is kept because it is defensively correct if the staging zone ever changes; the comment now says that instead of asserting a mechanism that does not fire here. A false justification is worse than none: it gets trusted. - The CR 701.21a quotation began at the rule's second sentence -- the same half-quote shape this branch exists to repair. It now starts at sentence one, "its controller moves it from the battlefield directly to its OWNER's graveyard", which is what bounds the conclusion: `SelfRef` proves control, never ownership, so a controlled-but-not-owned source puts a card in another player's graveyard while the leg still answers `OwnResourcesOnly`. Not repairable at this seam -- the AST carries no ownership -- so the limit is named rather than papered over. - `give_ironworks` had become a byte-equivalent duplicate of `give_parsed_card`; field-by-field equivalence was verified, then it was collapsed to a one-line delegation. The divergence hazard had already fired in the parent commit, where `base_mana_cost` reached one construction path and not the other. - The `v1` NON-VACUITY note credited the wrong assertion: `stage_two_action_set` only appends, so `stage_two == flat` cannot catch a non-sacrifice hand or graveyard mana activation. The `non_pass.len() == 1` reach-guard above does. - `summoning_sick = false` was a proven no-op (`create_object` documents that it does not set the flag; `GameObject::new` defaults it false). Removed, with a comment recording why it is absent so it is not re-added. Assisted-by: ClaudeCode:claude-opus-4.8 * test(ai): close three evidence gaps in the mana-reach shortcut regressions Review-impl r2 passed all five of the maintainer's acceptance criteria and returned no BLOCK; these are its three test-quality [LOW] items. Test-only -- no production seam is touched, and each new assertion has a mutation that was RUN and observed to redden it, not merely derived. - v10b's ordinary-mana-source control rested on two negatives (the Sol Ring activation is absent from the flat list; stage two equals the flat list) whose outcome was dominated by an upstream conjunct. `stage_two_action_set` chains `meaningful_sacrifice_mana_actions` over `activatable_object_mana_actions`, which for a probe at `Priority` is literally `mana_sources::activatable_mana_actions_for_player` -- so a Sol Ring that was never swept is filtered out for a reason the row could not distinguish from the penalty filter it means to measure. MEASURED: with the new guard removed and the Sol Ring tapped, BOTH pre-existing negatives still pass. They were green for the wrong reason. Two guards close it at two different levels. The sweep function is `pub`, so the row now asserts definitionally that the Sol Ring IS in the list stage two filters; and it asserts the consequence the row's own doc already stated as fact but never checked -- that the Sol Ring's capacity of 2 unlocks Angel of the Ruins' {2} plainscycling in the flat list. Swapping in a capacity-1 source reddens only the second, so they are independently breakable rather than redundant. - v10a bounded only its ACCEPT arm. Its SHORTEN arm's discrimination rested on a derived-but-unasserted fact, so a future fixture or capacity change that added one MayInterfere action would over-determine the row silently instead of reddening. It now carries the threshold sentinel its sibling already had. MEASURED at 2 (the Ritual cast plus Terramorphic Expanse); staging one extra two-capacity source makes it 3 and reddens the sentinel. - v10a asserted only that the {B}{B}{B} Bolt is NOT castable, leaving the funding half -- the half its own name claims -- as prose. The row now measures it on the production instrument the way v10b's lemma does: a quarantined clone casts the Ritual through `apply`, drives the stack, and re-probes. A zone reach-guard pins that the Ritual actually resolved (CR 608.2n) before the funding assertion runs. Printing the Bolt at {B}{B}{B}{B} reddens the funding assertion alone; collapsing the drive bound reddens the zone guard alone. The row's doc previously said "The row does NOT assert the post-resolution board". That became false with this change, so it is corrected rather than left to mislead. CR 117.1d, CR 601.2g and CR 608.2n were each grepped from the rules text before being written. The first draft of the last one said 608.2m from memory, which is a different rule ("if it leaves the stack once it starts to resolve, it will continue to resolve fully"); the verification step caught it. Assisted-by: ClaudeCode:claude-opus-4.8 * test(ai): pin membership, not just cardinality, in the mana-reach rows Two review findings, both cases of an assertion claiming more than it measured. The SHORTEN-arm sentinel bounded the action set's COUNT at 2 while its own prose claimed its MEMBERSHIP ("the ACCEPT arm's single fetchland PLUS the Ritual cast, and nothing else"). A fixture or capacity change that dropped the fetchland and added some unrelated MayInterfere action satisfies both `len() == 2` and the ritual reach-guard, so the row would have gone on measuring the wrong pair silently. Partitioning on the Ritual and asserting the remainder equals the ACCEPT arm's set closes that gap and pins the Ritual leg in the same equality: an empty or doubled partition reddens. The Angel guard matched on card name alone, but the fact it exists to establish is specific — that the Sol Ring's two mana unlock the {2} PLAINSCYCLING activation from HAND, which is what makes withholding a verdict assertion below it correct rather than evasive. A bare name match is also satisfied by some other Angel ability or an Angel in another zone. Ability index 0 is measured off the fixture, not assumed: object 210 carries exactly one parsed ability, tagged Cycling with activation_zone Hand. Both tightenings are additive; no existing assertion was weakened. Both were revert-probed, and the SHORTEN probe was chosen to be invisible to the two pre-existing guards so the new assertion is provably the one catching it. Assisted-by: ClaudeCode:claude-opus-5 * fix(ai): require a provably tapped entry before calling a fetch confined The mana-reach fix deleted the `Effect::Mana` allowlist arm because mana produced inside the priority window a Shorten hands the responder can fund an otherwise-unaffordable answer. The same mechanism was still live one arm over: `effect_window_reach` allowlisted ANY library-to-battlefield move as `OwnResourcesOnly`, so an untapped fetch — whose land taps for mana in that same window — read as confined. Measured on the production classifier, Crop Rotation and Nature's Lore both classified `OwnResourcesOnly`. `enter_tapped` was already destructured and ignored in that arm. The gate now requires the AST to PROVE a confined entry rather than merely suggest one (CR 110.5b: permanents enter untapped unless something says otherwise): `EtbTapState::Tapped`, with no conditional `enters_modified_if` rider that could change the arrival (CR 614.12 + CR 614.12a), not `enters_attacking` (CR 508.4), and `enters_under` absent or the actor. `Unspecified` and `Untapped` both fall out to `MayInterfere`. `SearchLibrary`'s `split` is the second door onto the battlefield — it moves its own found cards with no `ChangeZone` node — and is gated the same way. Both edits are conjunctive, so they can only move verdicts toward `MayInterfere`; neither can manufacture a false Accept. The gate is destination-scoped rather than tied to the anaphoric disjunct, because ownership does not stop an untapped land from producing mana. This DOES reclassify a live class, and an earlier revision of this message claimed otherwise. That claim came from a census that counted AST nodes over the whole card document; both halves were wrong. `printed_cards.rs` copies only `card_face.abilities` into `obj.abilities` and the classifier folds over `object.abilities` alone, so nodes in `triggers`/`static_abilities`/ `replacements` never reach it; and a verdict is a fold over a whole ability, so an ability already `MayInterfere` for an unrelated reason cannot flip whatever its `ChangeZone` node says. Re-measured at the enforcement surface — 22,717 top-level abilities in this candidate's own projection — this commit's two conjuncts move 195 of the 522 abilities that were still `OwnResourcesOnly` after the mana arm's deletion (37.4%), all in the `OwnResourcesOnly` -> `MayInterfere` direction, with 0 moving the other way. The `split` conjunct is inert at today's pool (9 of 9 battlefield-primary carriers already print tapped, none routes `rest_destination` to the battlefield); it is in because that door would otherwise stay open for the first card that walks through it. Adds `v10c`, a response-level regression driving the real 4p dump to a real `RespondToShortcut` and asserting on `smart_shortcut_response`, with a tapped control on the same board so the tap axis is what the row measures. Assisted-by: ClaudeCode:claude-opus-5 * fix(ai): never call an object confined when its rules content is unreadable `object_window_reach` folded only `obj.abilities`. `printed_cards.rs` splits one card face into four collections — `abilities`, `replacement_definitions`, `static_definitions` and `base_trigger_definitions` — so a card whose entire function lives in one of the other three was classified from the empty half. Measured witness: Stunning Reversal projects `abilities[0] = ChangeZone{origin: None, destination: Exile, target: SelfRef}`, which is confined on every conjunct this module reads, while its function is `replacements[0] = {event: GameLoss, mode: Mandatory}`. A seat holding it read `OwnResourcesOnly`, so the shortcut window was Accepted for the one card that exists to survive it. Both entry points now return `MayInterfere` on the presence of any of the three collections. The activation path gets the gate for its own reason rather than by symmetry: activating an ability is itself a game event, so a trigger on the same object can fire off the activation (CR 603.2) or off the cost being paid. The gate is presence, not content — this module has no classifier for those definition types, and the named upgrade path is to classify them the way `ability_window_reach` classifies `AbilityDefinition`. Same review round, one door over: `destination: Zone::Hand` leaves the confined set. A card put into hand is a castable card; after the spell that put it there resolves the active player receives priority (CR 117.3b) and priority then passes in turn order (CR 117.3d), so the responding seat gets it back still inside this window and a hand arrival is not provably confined. That hand gate went in on the `ChangeZone` arm only, and a review round found the `SearchLibrary` split still routing cards to hand unchecked. Both doors now go through one `landing_zone_is_confined` authority rather than two call sites answering the same question — two answers is how they drifted apart. The split is destructured `..`-free in the closure pattern for the same reason the `ChangeZone` arm is: a future `rest_enter_tapped` must be a compile error here, not a silently ignored arrival modifier. All changes are strictly narrowing — early returns to the absorbing value and one more conjunct each — so `May -> Own` is 0 by construction, not by census. Corpus, on this candidate's own SHA-bound projection of 22,717 top-level abilities: combined 2600 -> 151 confined (2449 flips, 94.2%, 0 widen); the entry gate alone 522 -> 151 (371, 71.1%). The hand authority accounts for 176 of those, split by door: 166 on `ChangeZone` across 154 cards, 10 on the split across 10 cards. All 12 split carriers route something to hand (9 via `rest_destination`, 3 via `primary_destination`), so Cultivate and its class leave the confined set. At object level, 72 printed cards have a fully-confined ability list and 19 carry an unreadable collection (7 static-only); 53 stay confined. That is a printed-card census and a lower bound on runtime objects — `game/stickers.rs` pushes into all three collections at runtime. The gate's price, stated rather than buried: Diligent Farmhand fetches a basic land TAPPED and flips anyway, on one static scoped `active_zones: ["Graveyard"]` that cannot touch this window. It is the only one of the 19 provably inert by its own zone scope; presence-gating cannot see that, and classifying the collections is what buys it back. Tests: `an_object_whose_rules_content_this_module_cannot_read_is_never_confined` covers all three disjuncts — replacement, trigger (through the real `materialize_base_trigger_definitions()` wiring, with a premise that the field the gate reads is populated) and static — each with the other two cleared first, so every verdict is attributable to one collection. `a_destination_is_confined_only_when_the_seat_cannot_act_on_what_lands_there` walks the destination axis on a real parsed node whose target is `SelfRef`, so destination is the only free variable. The split row asserts Cultivate's real `rest_destination == Hand` premise and mutates that one field as its control: since all 12 carriers touch hand, no real card can serve as the tap-axis positive control, and the test says so instead of implying live coverage. Revert-probed: each gate and each conjunct cut alone from pristine source reds its own row and leaves its neighbours green. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(ai): close the fail-open zone wildcard and widen the unreadable-content gate Review findings against a15f074, all reproduced before being fixed. `landing_zone_is_confined` matched `Battlefield`/`Hand` and closed with `_ => true`. `Zone` is a closed seven-variant enum (CR 400.1) and that wildcard was this module's only fail-OPEN default: it silently absorbed `Zone::Stack`, a live `ChangeZone` destination, so a node landing a card on the stack read as confined. CR 405.1 puts a cast spell's card on the stack and CR 608.1 resolves it once all players pass, which is strictly stronger reach than the `Hand` case this same function closed last round. The match is now exhaustive, so adding a `Zone` variant breaks the build. `Zone::Command` joins hand and stack per CR 903.8 (a commander may be cast from the command zone) and CR 114.1 (emblems carry abilities there). The object gate's own doc claimed `printed_cards` splits a card face across four collections. Measured, it writes eight more rules-bearing fields, and `obj.keywords` carries printed Cascade — which never reaches `trigger_definitions` — so a Cascade spell whose printed abilities all read confined was provably `OwnResourcesOnly` while resolving it casts a free spell of arbitrary reach inside the window. Both entry points now route through one `carries_unreadable_rules_content` authority covering every unreadable rules-bearing field rather than a curated subset, since a subset is an allowlist someone has to remember to extend. That argument applies to the list itself, so the list is now compiler-enforced. A new test destructures `CardFace` `..`-free and sorts every field into folded / gated / not-rules-bearing with a reason on each. Writing it immediately found four more unreadable fields no re-reading had surfaced — `case_state`, `class_level`, `intensity` and `attraction_lights` — all now gated. `CardFace` rather than `GameObject` because `GameObject` has 149 fields, mostly runtime state, and destructuring it would be a churn magnet blanket-`..`'d back within a round; `CardFace` has 33 and is the source `printed_cards` actually reads. Scope limit stated in the test: it does not cover `GameObject` fields written outside `printed_cards` (`game::stickers` writes only the three definition collections, which are gated). Measured on this candidate's own projection: ability-level figures are unchanged (2600 -> 151), confirming the zone fix is latent — 0 `Stack` destinations, 1 `Command` (Hellkite Courser, in `triggers`, never folded). Object level moves 72/19/53 to 72/29/43: the widening adds 10 flips (8 keywords, 1 modal, 1 additional_cost). The four fields found by the guard are 0 among survivors and real document-wide (solve conditions 15, Class 38, Case 15, Attraction 35) — latent holes, closed for the same reason as the zone arm. The protected class is untouched — Terramorphic Expanse, Evolving Wilds and Rampant Growth carry none of these fields, asserted in the census as a reach-guard that fails rather than prints. Tests: the destination table now covers all seven zones with a compile-time exhaustiveness guard that is deliberately not a mirror of the production match; the object-gate test gains keyword and spellbook disjuncts with a cleared-field control between each, so no verdict can be a constant. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(ai): gate Room unlock state and test every disjunct of the unreadable-content gate A review round measured three holes in the object-level gate this PR added, all of the same shape: a set that was curated by hand and therefore incomplete. Seven of the gate's disjuncts had neither a test nor a revert-probe -- `modal`, `additional_cost`, `strive_cost`, `cleave_variant`, `casting_restrictions`, `casting_options`, `back_face`. Each occurred exactly once in the file, in the gate itself, so deleting any of them left the whole suite green. Witness coverage was anti-correlated with liveness: `modal` and `additional_cost` are 2 of the 10 cards the widening flips, while all four disjuncts that did have witnesses are inert at today's pool -- witnesses had been written for the newest additions rather than derived from the gate. `every_gated_card_face_field_reaches_the_gate_through_printed_cards` closes that with 12 cases, each mutating a `CardFace` and running the real `apply_card_face_to_object`, which also turns the staleness guard's comment-only CardFace->GameObject mapping into a runnable assertion. CONTROL and witness are separate freshly-created objects because `printed_cards` seeds `class_level` only while `base_characteristics_initialized` is false (CR 716.2b); re-applying to one object would have skipped the field the Class case tests. `back_face` reuses `game::specialize::empty_back_face`, promoted to `pub(crate)`, rather than duplicating a 22-field literal that would go stale the moment `BackFaceData` grew. The staleness guard bound `metadata: _` -- the one field its own comment called "mixed", and the exact path `spellbook` had already used to get into this gate. It now destructures all 8 `CardMetadata` fields with a reason each. The guard's `card_type` bucket reason was measurably wrong: `printed_cards` derives four object-level rules fields from `subtypes` alone (`Class`, `Case`, `Room`, `Attraction`), and `room_unlocks` was the one with no disjunct -- so the bucket whose job is certifying "no gate needed" was certifying a real hole. CR 709.5 makes a shared type line two static abilities that remove each locked half's rules text, and CR 709.5e lets any player unlock a half at any priority, so which halves are unlocked decides what rules text the permanent has and `obj.abilities` cannot express it. Presence gate, strictly narrowing: 0 flips at today's pool (72/29/43 unchanged; of 60 Room faces only two clear every other gate and neither is confined). Assisted-by: ClaudeCode:claude-opus-5 * fix(ai): prove confinement against the live board, not the AST alone `WindowReach::OwnResourcesOnly` was emitted from an AST-only proof whose premises are live-state facts the AST structurally cannot carry. The module was fail-closed on unrecognized *variants* but fail-open on known-incomplete *inputs*. Three gates close three premises, all at `any_action_may_interfere` — the one seam that already holds `&GameState`. Ownership (CR 110.2, CR 109.5, CR 701.21a). `SelfRef`/`Controller` prove CONTROL, never ownership, and a sacrificed permanent goes to its OWNER's graveyard — so a controlled-but-not-owned permanent changes another player's resources while every AST predicate still reads confined. Proven set-level: `TargetFilter::Controller` is a bare unit variant and `Typed` is a set predicate, so per-object threading is structurally impossible. Board observers (CR 603.2, CR 603.6a, CR 113.6). Trigger matching is a board-wide scan; the fold read one object. A tapped, self-targeted fetch was provably confined while an opponent's Hedron Crab ("Landfall — Whenever a land you control enters, target player mills three cards") reached every player. Relief is a disjunction of independently sufficient reliefs, gated zone-of-function first via the existing CR 113.6 authority. The subject carve-out is `SelfRef`-EXACT: reusing `filter_is_actor_owned` returns true for `Typed{controller: You}` — exactly the crab's filter — and provably fails to fix this, which is pinned as a negative control. Parse completeness. A parse cannot testify about its own omissions, so `parse_warnings` now gates confinement and moves from the staleness guard's NOT-RULES-BEARING bucket to GATED. That one line — "parser diagnostics, never consulted at runtime" — was the root cause stated as a certification. The relief predicate lives in `ai_support`, not `game/triggers.rs`: relief is not intrinsic to a trigger, it holds only relative to the confined-action allowlists this module defines. `LifeLost` relief is sound only because `AbilityCost::PayLife` falls to `MayInterfere`; its doc names both allowlists and that witness so widening either forces a re-audit. Measured, not assumed: `Milled*`, `EntersOrAttacks` and `EntersOrHauntedCreatureDies` are NOT relieved — their matchers read a generic `ZoneChanged` that an allowlisted `ChangeZone` emits (CR 701.17a), so relieving them would have been a fresh false-Accept. The flagship acceptance seat keeps `Accept` (171 defs -> 168 zone-gated -> 3 survivors, all relieved); P1 is safe via the stage-1 short-circuit, not because no board fires. All 14 revert-probes executed PASS->FAIL, none non-discriminating. 30 CR numbers verified against the rules text; one self-caught miscite corrected (701.12a is Exchange, not Fight -> 701.14a). Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): redact parse_warnings and confine only library-origin entries Three defects found by independent review of the previous commit. Hidden-information leak, introduced by that commit (CR 400.2: a hand and a library are hidden zones). `GameObject::parse_warnings` was not cleared by `hide_card`, so an opponent received a card whose `name` read "Hidden Card" while its diagnostics carried the printed rules text verbatim — and `SwallowedClause.description` IS that text. Because the field is skipped when empty, it appeared on exactly the 891 cards that carry one, making it a per-card fingerprint for reading hands and libraries. The sibling redactor `redact_face_down_identity_from_observer` had the same hole — measured, not assumed: a manifested card's diagnostics survived the face-down transform and reached the observer's payload (CR 708.2, CR 708.5). Both are cleared, each with a matched-pair test: empty for the non-viewer, non-empty for the owner, so the negative arm cannot pass vacuously. The root cause is structural and larger than this field: `hide_card` is an allowlist-shaped problem implemented as a denylist, so any new `GameObject` field defaults to LEAKED. Passing the serialization partition guard proves nothing about visibility. An audit run against the wire found `base_name` (every hidden card's real name), `spellbook`, `unimplemented_mechanics`, `token_rules_text` and the `base_*` characteristics all leaking. Those are PRE-EXISTING and are deliberately NOT fixed here — the correct repair is to project a minimal object rather than extend a hand-maintained clear-list, and that carries its own test matrix and client-consumer audit. A battlefield entry is now confined only from `Zone::Library`. The previous commit's doc claimed the arriving card "is still in the library, so neither input contains it" — false for 46 abilities returning from graveyard, hand or exile, whose objects ARE in `state.objects`. Witness: Helping Hand returning a Fleshbag Marauder is classified confined while making every player sacrifice. The precise fix needs an entering `ObjectId` that does not exist at decision time (`Typed{controller: You}` is an unchosen target), so this narrows the allowlist instead — moving verdicts only toward `MayInterfere` and making the paragraph true rather than retreating from it. Measured cost: of 493 document-wide tapped entries, Library's 247 keep classification; 245 lose it. The flagship fetch rows are untouched — that disjunct already required a library origin. `t2_10`'s decoder pin is now order-insensitive, so reordering `TriggerMode` no longer reds an `ai_support` test. All four revert-probes executed, each the sole failure of its suite. Full engine suite 23,638 passed / 0 failed; workspace check and clippy clean. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(ai): a library fetch is confined only while every selectable card is inert The classifier admitted `Library -> Battlefield` arrivals as confined whenever the permanent entered tapped, on the argument that the card is an unchosen member of a hidden zone and therefore unreadable. That argument is measurably false. CR 701.23a: "To search for a card in a zone, look at all cards in that zone (even if it's a hidden zone)". Probed on the enrolled 4p dump, 91 of 91 library objects resolve in `state.objects` with full parsed rules content — and the hazard is already on that board: the seat's own library holds Bojuka Bog, whose ETB exiles TARGET PLAYER's graveyard. An unrestricted "search for a land card" could select it, so the seat was accepting away the only window that reaches a live cross-player choice. `library_arrivals_are_inert` reads the actor's pooled hidden zones and asks whether every card matching the union of the object's search filters is inert, reusing `carries_unreadable_rules_content` as the single inertness authority rather than growing a second one. It is applied at both entry points beside that gate, not threaded into the AST classifier: two players holding the same printed card get opposite verdicts, so this is a board fact and presenting it as an AST fact would mislabel it. A non-empty pool with an empty match set fails closed — 24 cards carry a search filter that reads the ability's own targets, which this context cannot evaluate, and a false `Accept` is the one outcome not worth risking. No corpus assumption is made. "Basic lands are vanilla" would have been the cheap boundary and it is not proven — a future set can print a Basic land with a trigger. The flagship row survives on measurement instead: Terramorphic searches Basic-only and all 22 basics in that deck are vanilla. Elvish Reclaimer, the card the report named, cannot carry the regression: its `Sacrifice(Typed[Land])` cost is unqualified, so `cost_window_reach` already answers `MayInterfere` and the row reads `Shorten` with and without the new gate. Measured, then replaced with its structural siblings Reshape the Earth and Urza's Cave, which exercise the spell and activated paths respectively. `parse_warnings` now rides `BackFaceData` and all four face-copy paths, so a transform swaps diagnostics with the face they describe instead of leaving a front-clean/back-warning card reading clean. The base-face snapshot line is load-bearing: `apply_face_down_creature_characteristics` blanks name, types, abilities and definitions but not diagnostics, so without it turning face up would have cleared what turning face down preserved — a regression this change would otherwise have introduced. Adding a `BackFaceData` field is compile-forced across 34 struct literals; each is the same single mechanical initializer. Seven revert-probes executed, each observed pass->fail. Engine suite 18,827 lib + 4,790 integration passed, 0 failed; workspace check and clippy clean. One self-caught miscite removed (CR 701.23c is undefined qualities in hidden zones, not a search limit). Assisted-by: ClaudeCode:claude-opus-4.8 * test(PR-7101): preserve face diagnostics across transform --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* fix(parser): support Exuberant Wolfbear * test(PR-7204): select trigger target before may choice --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
…rs#7205) * docs: name the phase-engine package in every cargo invocation Eight comments and script strings tell a reader to run `cargo ... -p engine ...`. None of them can work. The workspace package is `phase-engine`; `engine` is only the lib TARGET name, so `-p engine` names a package that does not exist: $ cargo test -p engine --lib pool_structure_census -- --list error: package ID specification `engine` did not match any packages $ cargo pkgid -p engine error: package ID specification `engine` did not match any packages $ cargo pkgid -p phase-engine path+file:///.../crates/engine#phase-engine@0.50.0 The authority is `crates/engine/Cargo.toml`: `[package] name = "phase-engine"` against `[lib] name = "engine"`. Every one of these strings is an instruction someone is meant to follow -- a perf benchmark's `#[ignore]` reason, a census test's reproduce line, a migration script's closing "Verify with:" -- so each one costs its reader a failed command and a detour before they work out that the package is named something else. The corrected forms resolve, and resolve to the intended tests rather than to nothing: $ cargo test -p phase-engine --lib pool_structure_census -- --list parser::oracle_ir::feature::pool_structure_census::census: test 1 test, 0 benchmarks Running the fully corrected `--ignored` form reaches the test body and stops on the environment variable the comment itself tells you to set (`ORACLE_POOL_DIR`), which is the expected outcome and the point: the package selector, target filter and test filter all resolve, and only the documented setup step remains. Cargo agrees -- its own rerun hint prints `-p phase-engine --lib`. Three further instances of this defect exist in `game_state_size.rs`, `bin/interaction_bindings.rs` and the generated `client/src/adapter/generated/ interaction/index.ts`; they are fixed by a separate in-flight PR and are deliberately untouched here so the two changes cannot conflict in either merge order. Seven more live in `.claude/wf/*.md` planning documents and are left for maintainer discretion. A naive `grep -rn -- '-p engine'` also matches `.cargo/config.toml`'s `-p engine-inventory-gen`. That is a substring hit, not a defect -- `engine-inventory-gen` is a real package and `cargo pkgid` resolves it. Assisted-by: ClaudeCode:claude-opus-5 * docs(PR-7205): correct remaining phase-engine selectors Completes the live documentation selector migration for the generated interaction bindings and GameState size measurement instructions. Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com> --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* ci: split Rust test job into four nextest shards The two-shard matrix puts each rust-test shard near its 30-minute ceiling. nextest's count partition distributes execution evenly, so four shards roughly halve per-shard test wall-clock; the per-shard workspace build cost is unchanged (each shard already builds the whole workspace, see the disk-cleanup comment) and stays absorbed by sccache. The rust-check aggregation job reads the matrix-wide result, so no downstream job changes. Assisted-by: ClaudeCode:claude-fable-5 * ci: lower rust-test shard timeout to 20 minutes With four shards each runs well under the old two-shard wall-clock, so the 30-minute ceiling sized for two shards is slack; 20 minutes still leaves headroom over the per-shard build + quarter-partition run. Assisted-by: ClaudeCode:claude-fable-5
…ids (phase-rs#7207) AI-CONTRIBUTOR.md aborted a run whenever the model could not emit an exact identifier, which gates on identifier availability rather than capability. Harnesses like GitHub Copilot never hand the running model its own id, so compliant frontier sessions were told to abort. - §0.1.1: replace the blanket "cannot determine your model, abort" with three-way routing; a harness-reported name declares via a hedged `Model: <name> (via <harness>; canonical id not exposed)` form. - §0.1.1: state the version-ordering rule the `+` suffix implied but never defined, including separator normalization and variant suffixes, so `gpt-5.6-sol` resolves against the `gpt-5-5` floor instead of aborting. - §0.1.1: a trailer naming a harness rather than a model neither corroborates nor contradicts a declaration, so its silence is not grounds for a close. - Appendix B.3: correct the Opus floor (4.7 -> 4.8), add Sonnet 5 and the Codex below-floor case, and carry the harness allowance into the self-contained prompt. Tier: routing is unchanged -- `Tier: Frontier` still gates, and the hedged Model line still matches the `^\s*\**Model:` reader in pr-review-loop. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
* Fix Emperor of Bones * Fix replacement prompt preservation * Fix replacement prompt precedence * test: cover replacement prompt pause and resume
Owner
Author
|
Opened against this fork by mistake; replacing with a pull request against phase-rs/phase. |
Owner
Author
|
Superseded by phase-rs#7218: phase-rs#7218 |
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
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Coalition Relic no longer prompts for a mana color when removing zero charge counters. The shared mana prompt path now checks resolved production before offering a color choice, while positive production and existing count-scoped resolution remain intact.
Closes phase-rs#7164
Files changed
Track
Developer
LLM
Model: GitHub Copilot (via GitHub Copilot; canonical id not exposed)
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 106.5 — existing mana resolver/prompt authority; no new CR annotation was added.
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all -- --check— passed.git diff --check— passed.cargo clippy -p phase-engine --all-targets -- -D warnings— passed.cargo test -p phase-engine --test integration coalition_relic_integration::coalition_relic— 2 passed, 0 failed.cargo test -p phase-engine --quiet— 18,832 + 21 + 9 + 4,798 passed, 0 failed; ignored tests: 6 + 0 + 0 + 2 + 7../scripts/check-parser-combinators.sh— passed with Gate G and Gate A.Gate A
Gate G PASS (router/grant architecture: strict router vs permissive grant boundary intact)
Gate A PASS head=a50d51ba496eb366c6a5a53c6908d50eadf329d0 base=97591656218103d8e8c7315725b24cfe64645dd4
Anchored on
AnyCombinationbranch resolves the production count before creating a combination prompt.AnyCombinationOfObjectColorsbranch resolves the production count before creating a dynamic combination prompt.Final review-impl
Final review-impl PASS head=a50d51ba496eb366c6a5a53c6908d50eadf329d0
Claimed parse impact
None.
Scope Expansion
None. The sibling guards remain within the same shared mana prompt authority and enforce the same zero-production invariant for existing dynamic production variants.
Validation Failures
cargo semantic-auditbuilt successfully but could not run because this checkout lacksclient/public/card-data.json; the command reported:card-data.json not found at client/public/card-data.json. CI or a generated card-data checkout is required for that audit.Workspace-wide
cargo clippy --all-targets -- -D warningswas blocked while compiling unrelatedserver-corebecauseopenssl-sys v0.9.116could not find an OpenSSL installation. The touched package passed the scoped Clippy command above.cargo coverage— built successfully but could not loaddata/card-data.json(No such file or directory); it returned an empty coverage report. CI or generated card data is required for this report.CI Failures
None known.