[design] Audio/video player chat — research & design (not for merge) - #2
Open
rykerwilliams wants to merge 1 commit into
Open
[design] Audio/video player chat — research & design (not for merge)#2rykerwilliams wants to merge 1 commit into
rykerwilliams wants to merge 1 commit into
Conversation
… and plan Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
You have definitely put some thought into it, I hope he hears you out. |
matthewevans
pushed a commit
that referenced
this pull request
Jul 11, 2026
…ng block already handles it (test-only) (phase-rs#5547) * test(engine): prove Land's Edge already parses and resolves correctly Land's Edge ("Discard a card: If the discarded card was a land card, this enchantment deals 2 damage to target player or planeswalker. Any player may activate this ability.") is listed in the parser-misparse backlog's root cause #1 (dropped relative-clause/filter restriction). Investigation found this is a stale backlog entry, not a live bug: the existing AbilityCondition::CostPaidObjectMatchesFilter building block (added 2026-05-04, previously only exercised in ConditionInstead-wrapped form by Agency Coroner/Surtland Flinger/Stormscale Anarch/Grab the Prize) already correctly handles this card's bare (non-instead) composition -- condition extraction, chunk-boundary protection for the leading "if", runtime evaluation via the cost-paid discard's LKI snapshot, and a separate PlayerFilter::All "any player may activate" mechanism. Zero production code required. Adds 2 parser unit tests locking the full parse shape (CR 602.1 + 602.1a + 602.2 + 118.1 + 608.2c + 608.2k + 400.7j) and 5 GameRunner integration tests proving the runtime behavior: discarding a land deals 2 damage, discarding a nonland deals 0 (ability still resolves), the discard snapshot binds the specifically-chosen object (not any land in hand), a non-controller can activate and pays from their own hand (CR 602.1a), and a sibling without the "any player" clause rejects non-controller activation (CR 117.3d priority-gate correctly enforced). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * docs: remove Land's Edge from parser-misparse-backlog Confirmed fixed (test-only, no production change) by the preceding commit. Root cause #2 (dropped intervening-if): 606 -> 605 cards; totals rebased to 4760 distinct / 4794 total appearances. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM * test(engine): reduce Land's Edge PR to a single discriminating parser test Addresses matthewevans' review on phase-rs#5547: the CostPaidObjectMatchesFilter building block this PR exercises already has runtime coverage via four sibling integration tests (Agency Coroner, Surtland Flinger, Stormscale Anarch, Grab the Prize). Adding a fifth card's standalone 427-line integration test file for the same already-covered condition was duplicative coverage with real suite-runtime/maintenance cost and near-zero marginal risk reduction. The one genuinely new thing Land's Edge's shape exercises is a parser-level distinction: a BARE (non-instead) composition of CostPaidObjectMatchesFilter, versus the ConditionInstead-wrapped form all four existing sibling cards use. At runtime this is not actually a new code path -- evaluate_condition's CostPaidObjectMatchesFilter arm already fires for any ability.condition, ConditionInstead or not -- so the distinction is provable at the parser level alone. Removed: - crates/engine/tests/integration/lands_edge_discard_land_condition.rs (427 lines, 5 tests) and its main.rs mod line - lands_edge_without_any_player_clause_has_no_activator_filter (a second parser test exercising the separately-established PlayerFilter::All mechanism, not the bare-condition distinction) Kept: lands_edge_discard_land_condition_parses_as_bare_intervening_if, a single parser unit test that locks the full bare parse shape (cost, condition explicitly NOT ConditionInstead, effect, activator_filter) in one assertion block -- the "single discriminating assertion" the review asked for. Verification note: could not get a fresh cargo test run to complete locally after this reduction -- 5 consecutive attempts were killed mid-compile (a background-build contention issue this fork's ~10 concurrent worktrees have hit repeatedly; the shared WORKLIST.md cargo-lock was held by another agent throughout). This change is a pure deletion (no logic modified) of an already-green test suite; the one retained test is byte-identical to its previously-verified-passing form. CI will provide the authoritative signal. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01DMa6DrxXyFHBdGxz3uLgvM --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com>
rykerwilliams
pushed a commit
that referenced
this pull request
Aug 22, 2026
…urn costs less" (phase-rs#6196) "The second spell you cast each turn costs {N} less to cast" (Highspire Bell-Ringer, Uthros Psionicist, Raging Battle Mouse, Monk Class, Alisaie Leveilleur) parsed with spell_filter: None and condition: None — the ordinal "second ... each turn" gate was silently dropped, so the reducer cheapened EVERY spell the controller cast instead of only the second. Root cause: `parse_first_qualified_spell_filter` (grammar.rs) hard-coded the literal prefix "the first " and a `SpellsCastThisTurn == 0` gate, so any ordinal past "first" returned NotApplicable and fell through to the generic, filterless, conditionless cost-modifier path. Fix: parameterize the existing seam by ordinal (FirstQualifiedSpell -> NthQualifiedSpell { filter, timing, ordinal }; "first" -> 1, "second" -> 2, ...) so the gate becomes `SpellsCastThisTurn(you) == ordinal - 1` -- exactly ordinal-1 qualifying spells already cast this turn => the spell now being cast is the Nth. "first" is the unchanged ordinal=1 (== 0) case, so merged first-spell behavior is byte-for-byte preserved. A "parameterize, don't proliferate" refactor of one combinator seam, not a new sibling. New `parse_spell_ordinal_prefix` + `parse_ordinal_word` combinators cover first..tenth. Parse-only: the runtime already evaluates the SpellsCastThisTurn static condition at cost determination (collect_battlefield_cost_modifiers -> evaluate_cost_mod_static_condition). CR 601.2f (total cost determination) / CR 107.3 (numeric/ordinal values). Tests: parser unit test asserting "second" -> SpellsCastThisTurn == 1; registered runtime cast-pipeline regression (nth_spell_ordinal_cost_reduction) proving the first spell pays full {2}, the second is discounted to {1}, and the third pays full {2} again (fails on revert -- the pre-fix reducer discounts the first cast). Backlog: removed Uthros Psionicist and Raging Battle Mouse from root cause #2 and Highspire Bell-Ringer from root cause phase-rs#28 (all now parse fully clean); decremented the associated counts. Monk Class and Alisaie Leveilleur retain other misparses and stay listed. Co-authored-by: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
rykerwilliams
pushed a commit
that referenced
this pull request
Aug 22, 2026
…ng-if, restoring the swallowed damage clause (phase-rs#7047) * Fix Valakut Exploration — parse the existential exiled-with intervening-if, restoring the swallowed damage clause Valakut Exploration's end-step trigger was a fully silent double misparse: the existential intervening-if ("if there are cards exiled with this enchantment") was unrecognized — the exiled-with condition family knew only the subject-first surfaces, and the counted "there are N ..." path demands a numeral — so the condition dropped to null, AND the unstripped "if " prefix suppressed the ", then" clause split, silently swallowing the trailing "then this enchantment deals that much damage to each opponent" conjunct. Zero warnings, zero Unimplemented: a Valakut Exploration that never dealt damage and swept nothing (its bare "them" had no antecedent). One seam clears both symptoms plus the class: - parse_card_exiled_with_source_condition gains the existential axis as a comparator-carrying prefix alt: "there are cards exiled with ~" (GE 1) and "there are no cards exiled with ~" (EQ 0), composed onto the existing family (CR 406.6/607.2a/603.4). The counted surfaces keep their number path; the "there are no cards in your graveyard/library" neighbor family backtracks at tag("exiled with ") — pinned by unit negatives. - The hoisted condition introduces the linked-exile pool as the PLURAL anaphor antecedent (new ParseContext.plural_object_pronoun_ref, wired beside the singular object_pronoun_ref; CR 608.2c number agreement) — "put them ..." now lowers through the existing mass branch as ChangeZoneAll{origin: Exile, target: ExiledBySource} (Bomat Courier's proven runtime shape; links consumed per CR 607.2a + CR 406.6), and the restored ", then" split lowers the damage clause to DamageEachPlayer{Ref(EventContextAmount)} (Shadowheart's shape) fed by the sweep's last_effect_count stamp. Number-scoping keeps River Song's Diary's singular "it" chain byte-identical (pinned). Full-pool diff: exactly four movers — valakut exploration (full clear), evercoat ursine (condition + swallow-warning clear), the mysterious sphere (sweep binds to the pool), search the city (EQ-0 gate attaches). Fifteen sibling/lookalike cards pinned byte-identical. Coverage: swallowed-clause warnings -1, Unimplemented count unchanged (honest partials stay red). Backlog hygiene: Valakut Exploration removed from root-cause #2 (590 -> 589; totals updated). Runtime note: T1/T2/T4 damage totals are CHARACTERIZATION assertions with explicit markers — the parse emits the rules-correct shape, but the engine's per-player previous-effect-count table feeds DamageEachPlayer each recipient's own swept-card count rather than the total (a pre-existing amount-channel gap this chain is the first to exercise; engine-side fix out of scope for a parser PR). T1 still revert-discriminates via the sweep and the trigger gate (CR 603.4 — empty pool must not fire, proven). The mandated fixture regen surfaced arashin sovereign in the ordering-parity sweep (fixture staleness, byte-identical parse) — adjudicated as a documented-conservative over-prompt (unprofiled PutOnTopOrBottom in the RwProfile::conservative() catch-all; CR 603.6c first-zone check keeps members' writes disjoint), same adjudication as the sibling phase-rs#7031 branch. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv * Clarify the arashin sovereign ledger row's single-controller premise Review feedback (CodeRabbit): the row's commute argument read as if the sweep's Phase+OnlyDuringYourTurn privacy predicate established the batch's single controller. It never did — the property holds for every batch class by construction: begin_trigger_ordering partitions ordering groups per trigger_order_controller (CR 603.3b — each player orders only the triggers they control; cross-controller placement is APNAP-fixed, not chosen), which the sweep's batch rows model as ControllerUniformity::Uniform (team-pooled groups fail closed to Mixed). Comment-only; ordering_parity_sweep re-run green. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv * Gate the mass-move-fed damage rider honestly; entail nonempty pools for plural antecedents Maintainer review (matthewevans, phase-rs#7047) requested both changes: [HIGH] The mass-move-fed "that much" damage rider parsed to a shape the engine resolves rules-incorrectly (per-recipient own-count instead of the CR 608.2c sweep total — 0 to every opponent in Valakut Exploration's native pattern), while coverage counted the card supported. The rider now lowers to the honest strict-failure marker Effect::unimplemented(MASS_MOVE_TOTAL_DAMAGE_GAP, <verbatim clause>) via a typed gate at the single chain-assembly seam, scoped to the one categorically-broken pairing: ChangeZoneAll immediately followed by DamageEachPlayer reading Ref(EventContextAmount). Corpus census: exactly one member (Valakut). Scalar consumers (Draw/DealDamage-to-object/Mill/ LoseLife/SearchLibrary) stay un-gated — every corpus pool is single-owner by CR 400.3, so max-over-owners equals the true total there (the Whirlpool Drake class regresses nothing; pinned by hostile fixtures). Deletes when phase-rs#7046 lands the completed-sweep scalar-total channel; the zero-delta runtime assertions and the exact-unit marker test are the tripwires forcing that flip to be conscious. Coverage: supported -1 (Valakut honestly red on the rider), Unimplemented +1, warnings ±0. [MED] trigger_plural_object_pronoun_ref_for_intervening_if derived the plural pool antecedent whenever CardsExiledBySource appeared in the condition, ignoring comparator and Not — an EQ-0 "no cards" gate would bind "them" to a provably-empty pool. The derivation is now entailment-gated over the full composite algebra (GE n>=1 / GT n>=0 / EQ n>=1 / NE 0 at positive polarity; And-any / Or-all-nonempty; Not never derives, conservatively). Eleven unit regressions; zero corpus movers (every wiring-reachable condition is positive GE — scanned). Also: QuantityExpr::any_ref relocated verbatim from the game crate's private traversal (one authority, one-line delegation kept); the census test's line pin re-established with a DRIFT LOG entry per its in-file precedent (-16 line shift, content sha-identical); T1's planned unimplemented_oracle_ids assertion was empirically falsified and dropped - resolve_chain_body short-circuits Unimplemented before resolve_effect, so the telemetry insert is dead code on the stack-resolution path (a pre-existing engine gap, root-caused with line refs in the module doc). Full-pool diff: exactly one mover (the Valakut rider -> marker). The lowered snapshot updates; the IR snapshot is byte-identical (pinned). Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01WzL3nFmAqGfwhAUDCiKKcv * fix(PR-7047): address review findings * fix(PR-7047): bind negated empty exile pools * fix(PR-7047): escape assertion braces --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
rykerwilliams
pushed a commit
that referenced
this pull request
Aug 22, 2026
…phase-rs#7125) CR 310.10 is ATTACHMENT (MagicCompRules:1838). The protector state-based action is CR 310.11 (:1840); CR 310.12a (:1844) is what makes a battle's controller ineligible to be its own protector. 24 citations named the wrong rule. Verified verbatim against the rules text, not from memory. Was/Now, 24 sites across 9 files (8/7/4/2/1/1/1/1/1): crates/engine/tests/integration/rules/battle.rs 8 crates/engine/src/types/game_state.rs 7 crates/engine/src/game/sba.rs 4 crates/engine/src/analysis/loop_check.rs 2 crates/engine/src/ai_support/candidates.rs 1 crates/engine/src/analysis/resource.rs 1 crates/engine/src/game/engine_resolution_choices.rs 1 crates/engine/src/types/actions.rs 1 client/src/components/modal/BattleProtectorModal.tsx 1 The mapping is uniform and was REACHED BY CENSUS of all 24 sites, not assumed: 0 attachment sites, 0 Siege sites, 0 needing human adjudication. Symmetry, and the one intended asymmetry: 310.10 24 -> 0 (-24) 310.11 3 -> 27 (+24; baseline was 3, NOT 0) 310.11[a-z] 0 -> 0 (no fabricated sub-letters reintroduced) 310.12 30 -> 31 (+1) <-- Step 3, the sole intended asymmetry 310.12a 14 -> 15 (+1) paired sub-letter gate bare 310.12 0 -> 0 paired sub-letter gate 310.12b 16 -> 16 (untouched) scoped "CR " 4584 -> 4585 (+1), and ONLY game_state.rs moved Step 3 adds "+ CR 310.12a" at the single site whose prose asserts Siege-specific eligibility that 310.11 alone cannot support, following the house precedent at sba.rs:1914 and battle.rs:416 (exactly 2 in tree). This INVERTS Unit F (4889280dc5), where a deliberate deletion made a 6-out/5-in asymmetry correct. The "CR " axis has two populations and they are NOT conflated. The scoped nine-path count is the symmetry gate, measured differentially against its own base (4584 -> 4585). The repo-wide count is reported as MAGNITUDE ONLY and no delta is claimed for it: it moved by roughly +200 across this unit's planning window, part of that from foreign uncommitted work at a FIXED HEAD. The census's own originally-proposed residual gate read 30 on an untouched tree and was therefore vacuous. It was DELETED rather than re-thresholded; the discriminating quantity is bare 310.11, 3 -> 27. Non-comment sites: 3, all test-only, all message-only, zero production strings. Classified by brace-matched module boundaries, not by proximity: crates/engine/src/analysis/resource.rs:10295 mod tests #[cfg(test)] crates/engine/src/types/game_state.rs:21976 mod forced_cascade_window_tests #[cfg(test)] crates/engine/tests/integration/rules/battle.rs:476 integration test file Step 4 replaces a corroborating detail that discriminates nothing. A comment justified its citation by noting the rule "says in so many words 'This is a state-based action'" - true of 26 rules, so it read as verification while distinguishing no candidate. It now quotes the protector-specific clause, which is unique to CR 310.11. The diff is 26 lines for 24 sites: 23 are one-for-one substitutions and Step 4's site is a three-line reflow. VERIFICATION POSTURE, stated honestly: The executor HONEST-STOPPED before the post-edit gate suite. The plan required observing an intermediate state (310.10=1, bare 310.11=26) between edit batches; all edits were applied in one pass, so that state cannot be observed without a prohibited reverse edit. It declined to fabricate it and reported the sequencing failure as its own, finding no plan defect. Gates it skipped: post-edit token/closure/collateral/symmetry/hunk, formatting, frontend, TypeScript, post-edit clippy, post-build disk. Independently re-measured by the orchestrator on the committed candidate: all seven token gates above; scope exactness (9 files, 0 out-of-scope, 0 untracked); the 26-line/24-site accounting; the brace-matched cfg(test) classification; and a DIFFERENTIAL rustfmt check (0 hunks on both the base and candidate versions of all 8 Rust files). Compilation is NOT green at this base, and not because of this change. cargo clippy -p phase-engine --all-targets reports nine pre-existing E0063 errors for a missing `scry_top_count` field, at collect_evidence.rs:256, investigate.rs:47, life.rs:517, proliferate.rs:80, scoped_library_search.rs:541, search_library.rs:540, surveil.rs:48, engine_resolution_choices.rs:1573 and library.rs:114 - all outside every Unit G site, all in committed code at the base commit. The sound test for this change is therefore DIFFERENTIAL: the candidate's error set must equal the base's. That check is pending. Compile evidence came from a direct build with an isolated CARGO_TARGET_DIR in a dedicated worktree, NOT from Tilt - from a worktree every Tilt reading is CANNOT_ANSWER about these bytes. It is authoritative for compilation of its own bytes only. This repository validates no intra-doc links at all (0 rustdoc references across the Tiltfile, workspace manifests, crates/, scripts/ and all 16 CI workflows), so Step 4's link is guarded by an exact byte-identity probe rather than by a lint. Recorded because three review rounds accepted the opposite claim. Unit F follow-up #2, stated accurately: 74 of 76 stale tokens are gone. Two residual 310.11b occurrences remain in card-data.json, both from one card's upstream MTGJSON ruling text that our source provably does not emit (0 in-repo 310.11b against a 16-hit 310.12b control), so it is not ours to fix. Those files are gitignored and served from R2 - present on disk is not deployed. Plan: PLAN-G-r8.md sha256 7cf2f7c189376445b381dfc6c7e5eb26029013f3511f193c1d093f83a6737852 Eight review rounds; the eighth returned 0 findings at every severity. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
rykerwilliams
pushed a commit
that referenced
this pull request
Aug 22, 2026
* fix(ai): cast-commit whiff for dynamic damage spells
Problem:
The AI was committing non-lethal direct damage spells (like Slash of Light) against opponent creatures, effectively wasting the card. The existing cast-commit whiff detection (`AntiSelfHarmPolicy::score_pre_cast`) failed open for dynamic damage amounts (like `ObjectCount`) because the underlying `lethal_to_creature` helper returns `None` for non-fixed amounts.
Solution:
Extend `AntiSelfHarmPolicy` with a cast-commit lethality guard that evaluates dynamic damage amounts live.
* Introduced `removal_lethality::can_kill_any_legal_target`, which composes the existing `pending_damage_to_object` and `outcome_is_lethal` primitives to determine if the spell can actually kill any of its legal targets.
* Applied the established soft penalty (`wasted_cast_penalty`, -8.0) if the spell is harmful, targets only creatures, has opponent targets, but is provably non-lethal against all of them.
* Designed the gate to fail open (no veto) for variable-X spells, non-damage removal, or unknown damage sources at cast-commit to prevent over-blocking.
Validation:
* Flipped pinned reproduction tests for Slash of Light to successfully assert `PassPriority` over `CastSpell`.
* Added building-block and pipeline tests to prove the gate only blocks total whiffs (deferring partial whiffs to target selection), and safely ignores non-damage and variable-X spells.
* prevent cast-commit guard from blocking mixed and variable-X removal
Problem:
The newly introduced cast-commit lethality guard (`can_kill_any_legal_target`) over-blocked certain viable spells by falsely identifying them as total damage whiffs. Because the underlying evaluation only aggregates `DealDamage` effects, mixed removal spells (e.g., "deal 1 damage to target creature; destroy target creature") were evaluated solely on their non-lethal damage component, incorrectly ignoring the independent `Destroy` line (CR 701.8a). Additionally, variable-X damage spells with non-creature-only target filters could be incorrectly penalized.
Solution:
Introduce explicit fail-open guards to `can_kill_any_legal_target` to ensure it only vetoes provable, pure damage whiffs:
* Fail open if the spell contains *any* harmful creature-only effect that is not `DealDamage`. This prevents false damage-whiff vetoes on mixed damage+destroy spells, recognizing the non-damage half as an independent, useful removal line.
* Fail open if *any* `DealDamage` effect references a variable `X` (CR 107.3a), including those with non-creature-only target filters. Since `X` is chosen at announcement, the damage amount cannot be definitively known at cast-commit.
Validation:
* Added building-block tests to verify the fail-open semantics for mixed damage+destroy spells and non-creature `DealDamage` `X` spells.
* Added a production-path differential test (`mixed_damage_and_destroy_is_not_penalized_as_a_damage_whiff`) proving that mixed spells are successfully evaluated and strictly outrank identical pure-damage whiffs.
* fix(ai): extend cast-commit guard fail-open to control-changing lines
The cast-commit lethality gate (can_kill_any_legal_target) only failed open
on Harmful non-DealDamage effects with creature-only filters, so a mixed
'deal damage to target creature; gain control of target permanent' spell
was modelled purely through its non-lethal damage half and wrongly
penalized as a total damage whiff. GainControl is classified Contextual
(effect_classify.rs:198); the fail-open now covers Harmful or Contextual
non-DealDamage effects whose filter can hit an opponent's battlefield
permanent (creature-typed, any-target, or any permanent-type filter,
CR 613.1b Layer 2), so an independent control-changing half-line keeps the
spell castable.
Also harden the Very Hard Slash-of-Light reach-guard: pin the spell's
object id and assert the scorer offers its exact CastSpell candidate before
the AI run, so a pass or unrelated action cannot satisfy the test vacuously.
Adds discriminating unit tests (creature and permanent GainControl
fail-open) and a production-path differential test proving a mixed
damage+GainControl spell outranks the identical pure-damage whiff.
* fix(ai): cover negated/disjunctive control filters in whiff-gate fail-open
The cast-commit guard's fail-open helper (targets_creature_or_permanent)
only recognized creature-typed, any-target, and flat permanent-type
filters. Parser-reachable shapes that also name battlefield permanents
fell through the catch-all and re-introduced the false whiff veto the
GainControl fail-open was built to close: TypeFilter::Non ("nonland
permanent", "noncreature"), nested TypeFilter::AnyOf, Kindred
(CR 308.1), and TargetFilter-level Or/And/Not disjunctions.
Restructure into two recursive predicates that fail open on every filter
shape not provably limited to non-permanents: filter_can_match_permanent
(TargetFilter-level: Any/Typed/Or/And/Not) and
type_filter_can_match_permanent (TypeFilter-level: permanent types,
recursive AnyOf, Non excluding Non(Permanent|Card|Any), Kindred). Drop
the mis-cited CR 608.2b (target-legality rule, unrelated to disjunction
semantics) in favor of referencing the engine's own filter-matching.
Adds a shared mixed damage+GainControl fixture and three discriminating
unit tests (AnyOf, Non(Land), TargetFilter::Or): each fails against the
old catch-all, proving the shapes stay fail-open.
* test(ai): pin nested-AnyOf recursion in whiff-gate fail-open guard
The AnyOf discrimination test used a flat AnyOf([Artifact, Creature])
disjunction, which the pre-commit single-level arm already handled — the
test passed before the recursive helper existed and did not pin the
nested-AnyOf recursion added to close review LOW #2. Swap the fixture to a
nested AnyOf(AnyOf(artifact, creature)) and add a sibling
AnyOf(Non(Land), Non(Creature)) case: both shapes fell through the old
flat arm to the catch-all, so each fails against the pre-commit code and
pins the recursion (including descent into negated inners) going forward.
Correct the doc comment to state the pre-commit limitation accurately.
* comments cleanup
comments cleanup by Gemini 3.1 Pro Extended
* fix(ai): resolve mixed-removal usefulness from real populations, not filter shape
The cast-commit gate's fail-open credited any Harmful/Contextual
non-DealDamage effect whose FILTER SHAPE could match a permanent,
ignoring TypedFilter.controller: an own-controller-constrained GainControl
branch ('gain control of target creature you control', CR 108.3/115.2b)
was credited as opposing removal with no legal opposing population, and a
mixed DestroyAll + damage spell was classified only through its damage half
because extract_target_filter never surfaced DestroyAll.
Replace the shape proxy (targets_creature_or_permanent and its
filter/type-filter helpers, deleted) with a population check:
effect_has_legal_opposing_line resolves the COMPLETE typed filter —
controller included — via the engine's find_legal_targets and credits the
effect only when a legal object under an opponent's control exists. A
wipe's real population is resolved the same way.
Surface Effect::DestroyAll's population filter in extract_target_filter:
unlike the SetTapState/Suspect All scopes (which keep a Single sibling and
stay hidden), a wipe is inherently mass (CR 701.8) and its target IS the
population it destroys, so removal classification now sees the wipe line of
a mixed spell consistently.
Regressions: own-controller GainControl veto (with the AI's own creature
present, so the veto proves the controller axis, not the empty set) and
mixed damage+DestroyAll fail-open, plus an extract_target_filter unit test
contrasting the Suspect/SetTapState carve-outs.
* fix(ai): correct controller CR citations; keep wipes out of targeting-immunity
The population-based gate justified TypedFilter.controller semantics with
'CR 108.3 / CR 115.2b' — but 108.3 is the OWNER rule and 115.2b does not
exist. Use CR 108.4 (an object's controller) and CR 109.5 ('you' refers to
the object's controller) in all five sites.
Surfacing Effect::DestroyAll in extract_target_filter (previous commit)
made harmful_effect_uses_object_targeting classify a wipe as a
single-target effect, so CantBeTargeted/HexproofFrom/Protection grants were
wrongly credited as answering untargeted mass removal (CR 115.10a). Exclude
DestroyAll there — it is the only surfaced inherently-mass effect, exactly
restoring the pre-surfacing guarantee — and document the CR 115.10a /
702.11 / 702.16 / 702.18 rationale.
* cleanup comments
by Gemini 3.1 Pro Extended
* fix(ai): evaluate wipes by resolver population, not target legality
DestroyAll was fed through find_legal_targets (target legality) even though
the resolver (destroy.rs resolve_all) is NON-targeted and matches a
battlefield population: hexproof/protected opposing creatures were wrongly
excluded from the wipe's usefulness (CR 115.10a — an affected object is not
a target), and a declared TargetFilter::None read as an empty set instead
of the resolver's default all-creatures population (CR 701.8).
Dispatch Effect::DestroyAll in effect_has_legal_opposing_line to a new
resolver-mirroring path: mass_effect_has_opposing_population iterates the
battlefield, substitutes the default creature population for None, skips
indestructible (CR 702.12b), and matches the population via the engine's
matches_target_filter with a from_source_with_controller FilterContext —
the same primitives resolve_all uses, with no targeting exemptions.
Regressions: default-population wipe (mixed deal-1 + DestroyAll{None}
fails open; pre-fix the empty find_legal_targets set vetoed it) and a
hexproof-only opposing population (helper-level assert pins the
resolver-semantics seam; pre-fix target legality credited nothing), plus an
ai_quality differential proving a mixed damage + default-population wipe
outranks the identical pure-burn whiff through the full cast pipeline.
* test(ai): correct stale DestroyAll mechanism prose in wipe fail-open test
The sibling test can_kill_fails_open_on_mixed_damage_and_destroy_all still
described the wipe population as resolving 'via find_legal_targets', but
that test's typed-creature DestroyAll now dispatches through the
resolver-mirroring mass path (mass_effect_has_opposing_population —
battlefield population via matches_target_filter, CR 115.10a) introduced in
a30eb9c. Update the test doc comment and assert message to name the mass
path; behavior and assertion are unchanged. Remaining find_legal_targets
mentions in the file are all in genuinely target-legality code or
historical pre-fix descriptions.
* fix(ai): keep extraction target-only; consult mass population at the cast-commit seam
The whiff-gate blocker from review: extract_target_filter surfaced DestroyAll's
POPULATION filter as a selectable target, which flowed into anti_self_harm's
has_targetable_opponent_creature (TARGETING legality). A mixed damage+wipe
spell whose only opposing creature is hexproof (illegal target, but in the
wipe's non-targeted population, CR 115.10a) therefore took the -8 no-target
penalty at anti_self_harm:397 before the resolver-mirroring mass path was
consulted; and the engine tactical gate hard-REJECTED the whole cast as
'redundant creature-only removal'.
- Revert extract_target_filter to strict target selection (drop DestroyAll).
- is_opponent (CR 102.2/102.3, 2HG team-aware) replaces controller != ai in
all removal_lethality opposing checks.
- New cast-commit seam PolicyContext::has_opposing_mass_population, consulted
BEFORE the target-legality gate at anti_self_harm:397 and in the tactical
gate's is_redundant_creature_only_removal, so a mixed spell with a useful
wipe line is neither penalized nor suppressed when its only opposing
creatures are un-targetable.
- Regressions: anti_self_harm hexproof rescue test, 2HG team-aware unit test,
and a production-pipeline differential (mixed DealDamage+DestroyAll{None} vs
a pure wipe, both offered-CastSpell reach-guarded, on a hexproof-only
opposing board) pinning BOTH the tactical-gate and the :397 rescue fixes.
- CodeRabbit: mixed-versus-pure differential margins now use the AiConfig
penalty accessor (half wasted_cast_penalty) instead of hard-coded 1.0.
* fix(ai): cite CR 702.11b for hexproof-targeting; drop redundant wipe guard
The hexproof 'can't be targeted' claim recurs at nine sites citing CR 702.11a,
which is only 'Hexproof is a static ability.'; the cannot-be-targeted rule is
CR 702.11b (verified text). Correct all nine (CR 115.10a non-targeted kept
alongside where cited); bare CR 702.11 group refs left as-is.
With extract_target_filter target-only again, harmful_effect_uses_object_targeting's
leading !DestroyAll conjunct was dead (the .is_some() check already excludes
wipes). Remove it; document that target-only extraction excludes mass effects.
No behavior change.
* docs(ai): cite CR 702.11b for targeting-immunity in self_protection_classify
The any_stack_harmful_answerable_by_grants doc cited CR 702.11a for the
cannot-be-targeted ('targeting immunity') behavior; 702.11a is only 'Hexproof
is a static ability' and 702.11b is the cannot-be-targeted rule. Align with
the same correction applied to the whiff-gate pipeline. The DefensiveGrant
variant comment at line 279 (naming the shroud/he xproof ABILITIES
themselves) keeps 702.11a, which is defensible.
* docs(ai): drop out-of-context :397 line refs and blocker phrasing from comments
* fix(ai): fail open on unbound player-relative wipe populations
mass_effect_has_opposing_population builds a FilterContext with no
ResolvedAbility, but the engine resolves ControllerRef::TargetPlayer /
TargetOpponent by reading the first TargetRef::Player from ability.targets
and FAILS CLOSED without it (filter.rs TargetPlayer arm). A mixed spell
carrying a companion-player wipe ('destroy all creatures target opponent
controls') therefore read NO mass population at cast-commit even with an
opponent creature present, so its non-lethal damage half was vetoed
(wasted_cast_penalty) and is_redundant_creature_only_removal could
hard-reject the whole cast — contradicting the helper's conservative
fail-open contract and destroy::resolve_all (which resolves the population
via FilterContext::from_ability after the companion player is announced).
Make the mass population TRI-STATE: mass_effect_has_opposing_population now
returns Option<bool> — Some(true) opposing population exists, Some(false)
provably empty, None UNKNOWN when the population filter carries an unbound
player-relative controller scope (TargetPlayer/TargetOpponent/Scoped/
ParentTarget/Chosen/Triggering, via the new
filter_has_unbound_player_controller — conservative: any future
ControllerRef variant fails open). effect_has_legal_opposing_line and the
cast-commit seam has_opposing_mass_population treat None as useful
(!= Some(false)), threading the fail-open through BOTH anti_self_harm's
:397 rescue and tactical_gate::is_redundant_creature_only_removal so an
unresolvable-at-commit wipe can never be penalized or hard-rejected
(CR 109.4 / CR 115.1 / CR 601.2c).
Regressions: unit test (unknown seam + no veto) and two production-pipeline
differentials — mixed TargetOpponent wipe vs target-player-wipe baseline on
a live opponent board (anti_self_harm path) and a hexproof-only board
(tactical-gate thread, M-offered reach-guard discriminating).
* docs(ai): correct unbound-player-controller justification; document FilterProp boundary
filter_has_unbound_player_controller's doc claimed only You/Opponent resolve
from the casting source alone, which is inaccurate: ActivePlayer resolves
from state.active_player, EnchantedPlayer from source.attached_to, and
SourceChosenPlayer from source.chosen_attributes (filter.rs): for a pending
spell object those are genuinely absent, but the stated reason was wrong and
could mislead a future maintainer into 'correcting' a conservative
classification. Reword to the honest criterion: non-You/Opponent scopes are
UNBOUND BY CONSERVATIVE DESIGN (an over-approximation so a scoped wipe is
never provably-empty at cast-commit), naming the three source/global-state
readable variants explicitly. Also document the FilterProp-embedded
ControllerRef boundary (Owned/Attacking/ProtectorMatches/HasAttachment/
HasAnyAttachmentOf/MostPrevalentCreatureTypeIn/CanEnchant) as latent with no
current parser-emittable wipe population. No behavior change.
* docs(ai): drop out-of-context reviewer-session attribution from wipe test comments
* fix(ai): respect teams in opponent target checks
---------
Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
rykerwilliams
added a commit
that referenced
this pull request
Aug 23, 2026
…ust once Every card list/count in RESEARCH.md shared one citation at the top of section 1, and Swedish Old School -- the actual phase-1 target preset -- had no RESEARCH.md presence at all, only scattered mentions in CONTEXT.md. That makes independent verification harder than it should be, especially given every "preset data inconsistency" finding across four review rounds was an internal cross-reference mismatch, not an external-source check -- worth making the external source trivially reachable at the point of data. - Added a "Source:" line to each of the four existing EC format subsections. - Added a full "Swedish Old School 93/94" subsection to RESEARCH.md #1, matching the EC formats' treatment: direct source URL, its own verbatim legal-sets/banned/restricted/ante/legacy-rules data, and an explicit side-by-side comparison against EC's 93-94 restricted list proving these are two different, real rulesets, not one re-presented as two. - Added the same two source URLs inline at the top of PLAN.md #2, so the preset constructors are checkable without cross-referencing another file.
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
2 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.
This is a design proposal, not a code PR. No engine or production code changes — this is a discussion artifact capturing research into adding per-player, opt-in audio/video chat during a match. Not intended to merge anywhere.
Summary
Peerobject viapeer.call()/peer.on("call")— no new signaling stack needed. Caveat: hub-and-spoke topology means 3–6p games aren't peer-connected to each other; multi-party A/V would need a mesh and is deferred.lobby-worker/src/turn.ts). The only new cost is media-relay bandwidth for the fraction of sessions behind symmetric NAT/CGNAT.Full docs:
.planning/phases/59-audio-video-chat/{CONTEXT,RESEARCH,PLAN}.md.Test plan
N/A — documentation only, no code changes.