Skip to content

feat(parser): lift "[once] for each ⟨player-set⟩" onto fieldless Investigate - #6110

Merged
matthewevans merged 5 commits into
phase-rs:mainfrom
lgray:feat/std-dynqty-d-investigate-per-opponent
Jul 18, 2026
Merged

feat(parser): lift "[once] for each ⟨player-set⟩" onto fieldless Investigate#6110
matthewevans merged 5 commits into
phase-rs:mainfrom
lgray:feat/std-dynqty-d-investigate-per-opponent

Conversation

@lgray

@lgray lgray commented Jul 17, 2026

Copy link
Copy Markdown
Contributor

🤖 AI text below 🤖

Summary

Generalize the fieldless-Effect::Investigate "for each ⟨set⟩" lift from PlayerCount-only to the parameterized member-count class (PlayerCount | ObjectCount), and add the FilterProp::Goaded building block it needs. Addresses the #6110 review (the lift dropped object-ranged Investigate to a bare 1-Clue, rules-wrong CR 701.16a/608.2c).

Net effect: 7 real parse (AST) changes, all strict improvements, ZERO regressions (confound-free regen-both card-data.json diff at the merge-base 931c2dc2f). The repeat_for coverage projection is scoped to the lift class (see below), so the coverage-parse-diff sticky reports 8 cards / 10 signatures — those 7 plus one pre-existing, AST-identical card (Ethereal Investigator) whose repeat_for merely surfaces in the scoped signature.

  • 3 intended repeat_for lifts: Teysa, Opulent Oligarch (PlayerCount, opponents who lost life) · Wojek Investigator (PlayerCount, comparative hand-size) · Serene Sleuth (ObjectCount, goaded creatures you control).
  • 4 collateral goaded-filter improvements (the general FilterProp::Goaded correctly parsing "goaded creature" where it was previously Unknown/degraded — inherent to a general building block): Bothersome Quasit (CantBlock static) · Puppet Master, String Puller · The Rani · Vengeful Ancestor (goaded-subject triggers).
  • Deliberately NOT lifted: Tamiyo Meets the Story Circle's "investigate twice for each card discarded" (a contextual amount-ref) — lifting it would silently drop the "twice" multiplier. A fail-closed matches!(PlayerCount | ObjectCount) eligibility gate excludes it, locked by a revert-probed negative-boundary test. Byte-identical to base.

How this addresses the #6110 review

  • Blocker 1 (generalize the lift to retain every supported QuantityRef incl. ObjectCount + a runtime discriminator for ≥1 object-set card) → lower.rs gate widened PlayerCount-only → matches!(PlayerCount | ObjectCount); FilterProp::Goaded added so "goaded creature you control" parses to ObjectCount; the :404 enshrining test inverted to require the object-set lift. Runtime discriminator: serene_sleuth_runtime_makes_one_clue_per_goaded_creature (3 goaded → 3 Clues vs Goaded-blind 5 vs bare 1, revert-probed). Serene Sleuth is fixed and proven.
  • Blocker 2 + the coverage blast-radius follow-up → the repeat_for projection in coverage.rs ability_details is scoped to the lift class — it surfaces the repeat_for signature key only for a fieldless Effect::Investigate whose repeat_for is a member-count QuantityRef (PlayerCount/ObjectCount), exactly the eligibility set of for_each_repeatable_repeat_for. It does not touch the rest of the repeat_for surface (CopySpell/Token/Proliferate/SearchLibrary, or pre-existing Fixed/Variable/tracked-set Investigate forms). Measured with the real coverage-parse-diff tool against the merge-base (931c2dc2f, no-projection baseline): 8 cards / 10 signatures (down from the earlier global projection's 250), confound-free (0 oracle-text changes, 0 added/removed). That is 7 real parse changes (the 3 lifts Teysa/Wojek/Serene + the 4 collateral goaded-filter cards Bothersome Quasit, Puppet Master, The Rani, Vengeful Ancestor) plus 1 projection-only, AST-identical card — Ethereal Investigator, a pre-existing Investigate + PlayerCount repeat_for whose value simply becomes visible in the scoped signature (byte-identical card-data.json; structurally indistinguishable from the lifts, so no non-hacky scope excludes it). A revert-probed investigate_signature_exposes_repeat_for regression (2 positive member-count cases project; 4 negatives — None, Fixed, non-member-count Ref, non-Investigate effect — do not) locks the scope. Broadening repeat_for coverage to the whole surface is a deliberate global coverage-schema migration, deferred out of this focused feature.
  • Goaded event-snapshot certificate (matthewevans's 4th review, [HIGH]) — FIXED (option b). classify_prop in types/events.rs had FilterProp::Goaded in the Supported group, asserting the reach gate could answer a goaded event-subject filter from an EventObjectSnapshot. But the snapshot carries no goaded field and the runtime zone-change-record matcher already fail-closes it (game/filter.rs ~:5049, CR 701.15b/c) — an inconsistent, fail-open certificate. Reclassified FilterProp::GoadedUnsupported, aligning the certificate with the snapshot's actual fields and the runtime's existing fail-closed behavior, so a future goaded event-subject filter fails the reach gate loudly rather than silently reading an ungoaded snapshot. Pure contract+test change with zero behavior/coverage delta: the reach gate is test-only latent scaffolding (classify_prop/classify_filter_shape have no production caller today), coverage is supplied independently by coverage.rs, and the live goaded predicate (filter.rs FilterProp::Goaded => !obj.goaded_by.is_empty()) is untouched. Measured: 4-card and full-DB card-data.json byte-identical before/after, plus a revert-probed goaded_subject_filter_is_unsupported test (returning Goaded to Supported measurably flips the assertion). Option-(a) snapshot-plumbing path is deferred (see Deferred follow-ups).
  • Sophina, Spearsage Deserter — disclosed follow-up, NOT fixed here. It remains a bare Investigate. Its "nontoken attacking creature" filter is blocked by a distinct, pre-existing parse_type_phrase leading-adjective order-dependence (the nontoken-leads-attacking ordering returns None before the lift seam is reached), orthogonal to the gate fixed above. Deferred as a strict-failure tag + a flipping tripwire test (the Sophina branch of the inverted :404 regression), because the order-free many0 fix is the hottest shared parser path. The blocker is answered at the seam level (ObjectCount retained, proven by Serene Sleuth); Sophina is a scoped follow-up.

Implementation method (required)

Method: /engine-implementer

CR references

  • CR 608.2c — resolve the effect's instructions; the repeat_for loop reruns the fieldless Investigate once per iteration.
  • CR 701.16a — "Investigate" = create a Clue token (CR 111.10f).
  • CR 701.15b/c — goad designation (FilterProp::Goaded reads GameObject.goaded_by; multi-player set).
  • CR 508.2a + CR 603.2 — an attacks-triggered ability's trigger event is the attacking creature; its valid_card filter is evaluated against the live attacker.
  • CR 109.5 — "you"/"than you" binds the controller (Wojek's comparative operand).
  • CR 402.1 — cards in hand (HandSize).

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.

  • scripts/check-parser-combinators.sh upstream/main — Gate G + Gate A PASS (head 29e9ad9 base 931c2dc)

  • cargo fmt --all -- --check — clean

  • cargo clippy --workspace --all-targets -- -D warnings — clean (0 warnings; post-rebase drift-verify) + cargo clippy -p engine --test integration -D warnings clean for the new test

  • cargo test -p engine --lib — 40 passed (goaded / for_each_repeatable / dq_d_player_set_lift / investigate_signature + upstream goaded-combat)

  • cargo test -p engine --test integration — feature subset passed (teysa/wojek runtime, serene runtime 3-way, object_for_each lift+Sophina-deferred guard, vengeful goaded-attack pair)

  • Regen-both card-data.json diff (base 931c2dc2f = merge-base, feature head; same MAIN corpus symlinked into both worktrees, full per-card face diff): 35,396 cards both sides, no only-in-X ⇒ confound-free. 7 real AST changes, all strict improvements, 0 regressions. Byte-identity confirmed on Tamiyo Meets the Story Circle, Confront the Unknown, Declaration in Stone, Tranquil Frillback.

  • coverage-parse-diff tool (real binary, base 931c2dc2f no-projection snapshot vs the scoped-projection head; base verified: 0 repeat_for keys, 15 Investigate+repeat_for with the 3 lifts absent = no-lift): 8 cards / 10 signatures, confound-free (oracle_changed=0, added=0, removed=0). = the 7 AST cards + Ethereal Investigator (projection-only, AST-identical). Down from the earlier global projection's 250.

Runtime discrimination (apply()-drives, revert-probed)

  • Serene Sleuth (ObjectCount count context): serene_sleuth_runtime_makes_one_clue_per_goaded_creature — 3 goaded + ungoaded Sleuth + 1 plain → 3 Clues (vs Goaded-blind 5, vs bare 1); reverting the Goaded eval flips it.
  • Vengeful Ancestor (trigger-subject context — a distinct wire): vengeful_ancestor_goaded_attack_trigger.rs — goaded attacker → controller loses 1 (trigger fires); ungoaded attacker → no loss (does not fire); legs differ only in goaded state. Production-line revert-probe (filter.rs FilterProp::Goaded => false) flips the goaded leg. Proves the trigger valid_card Goaded filter resolves against the live attacker's goaded_by, not the fieldless EventObjectSnapshot — so the collateral cards are genuinely supported, not false-supported.
  • Teysa / Wojek (PlayerCount): teysa_runtime_*, wojek_runtime_*.

The repeat_for driver drive_repeat_for_outermost (crates/engine/src/game/effects/mod.rs) resolves the quantity generically with zero per-filter branching.

Gate A

Gate A PASS head=29e9ad967 base=931c2dc2f

Anchored on

  • crates/engine/src/parser/oracle_effect/lower.rsfor_each_repeatable_repeat_for (renamed/widened from for_each_player_set_repeat_for) shares the split_for_each_suffix primitive with the pre-existing strip_for_each_repeat_suffix lift at the same seam.
  • crates/engine/src/parser/oracle_effect/mod.rs — the existing repeat_for: Some(QuantityExpr::Ref{..}) chunk-seam lift the widened else if let Some(..) branch follows.
  • crates/engine/src/game/filter.rs / types/ability.rsFilterProp::Goaded mirrors FilterProp::Renowned across all 15 registration sites (candidate-local read of goaded_by).

Final review-impl

Final review-impl PASS head=29e9ad967 (independent /review-impl, zero blocking findings; the collateral live-eval risk was subsequently measured and cleared by the Vengeful Ancestor runtime pair).

Claimed parse impact

coverage-parse-diff reports 8 cards / 10 signatures (confound-free vs merge-base 931c2dc2f):

7 real parse (AST) changes:

  • Teysa, Opulent Oligarch
  • Wojek Investigator
  • Serene Sleuth
  • Bothersome Quasit
  • Puppet Master, String Puller
  • The Rani
  • Vengeful Ancestor

1 projection-only, AST-identical (disclosed, not a regression):

  • Ethereal Investigator — pre-existing Investigate + PlayerCount repeat_for; byte-identical card-data.json, surfaces only because the scoped coverage signature now shows its repeat_for. Structurally indistinguishable from the lifts, so no non-hacky scope excludes it.

Deferred follow-ups

Recorded in-code as a strict-failure tag with a flipping tripwire test (no silent drop):

  • parse_type_phrase leading-adjective order-dependence (Sophina, Spearsage Deserter — "nontoken attacking creature"). The leading-adjective stripping is a fixed positional cascade (combat-status → … → nontoken); a phrase whose adjectives cross that order is only partly stripped, so parse_for_each_clause returns None and the per-attacker Investigate stays honestly bare (no false lift). Fix: collapse the cascade into an order-free many0-style property loop — the hottest shared parser path (high CI-coverage-regression blast radius), hence deferred. Tripwire: the Sophina branch of object_for_each_investigate_is_lifted asserts the current bare-Investigate state and flips to fail when the gap closes.
  • EventObjectSnapshot goaded-projection certificate — resolved in this PR (option b, fail-closed reclassify; see the review-addressing section above). The remaining deferred (option a) is the fuller fix: snapshot goad onto EventObjectSnapshot + ZoneChangeRecord (mirroring is_suspected), wire the capture sites, read it at both runtime matchers, and reclassify FilterProp::Goaded back to Supported. Deferred because no card today prints a goad predicate on an event-subject filter under last-known-information — the reach gate is currently unreachable and goaded_by clears on zone change, so the fail-closed classification is correct and complete for every current card. FilterProp::Renowned is NOT the same shape (correcting an earlier note): EventObjectSnapshot already carries is_renowned, so Renowned=Supported is consistent at this reach gate; Renowned's only gap is on the separate zone-change-record path — a distinct, pre-existing concern, out of scope for this PR.

@lgray
lgray requested a review from matthewevans as a code owner July 17, 2026 17:49
@gemini-code-assist

Copy link
Copy Markdown
Contributor

Caution

The consumer version of Gemini Code Assist on GitHub has been sunset. All code review activity has officially ceased.

@lgray

lgray commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

@matthewevans ready for review + enqueue when you have a moment. This closes the same missing-wire class you flagged on #6108 (Bumi): a parser-only lift of a trailing "[once] for each ⟨player-set⟩" clause onto a fieldless Effect::Investigate, plus the end-to-end regression you would want — two apply()-drives (teysa_runtime_makes_one_clue_per_opponent_who_lost_life, teysa_runtime_no_clue_when_no_opponent_lost_life) with revert-probe evidence: disabling the wire flips the Clue count 0→1 and trips the reach-guard.

Gates green on head 67da88d (rebased on current main 7afcff9): clippy --all-targets -D warnings, integration 7/7, lib 3/3, Gate A, all 6 CRs resolve, coverage-diff = exactly {Teysa, Wojek} (+2, 0 regressions). I am pull-only so I cannot self-enqueue — over to you for the merge queue. Thanks!

@matthewevans

Copy link
Copy Markdown
Member

Maintainer update: the implementation re-review on the current head is clean. Approval remains held until GitHub publishes the current-head coverage-parse-diff artifact and the required Rust lint/test checks finish successfully. No action is needed from you right now; I’ll resume once that evidence lands.

@github-actions

github-actions Bot commented Jul 17, 2026

Copy link
Copy Markdown

Parse changes introduced by this PR · 8 card(s), 10 signature(s) (baseline: main c7225018447d)

🟢 Added (2 signatures)

  • 2 cards · ➕ trigger/DamageDone · added: DamageDone (active in=battlefield, damage kind=combat only, valid source=goaded creature, valid target=opponent)
    • Affected (first 3): Puppet Master, String Puller, The Rani
  • 1 card · ➕ trigger/Attacks · added: Attacks (active in=battlefield, watches=goaded creature)
    • Affected (first 3): Vengeful Ancestor

🔴 Removed (3 signatures)

  • 1 card · ➖ trigger/Whenever a goaded creature attacks · removed: Whenever a goaded creature attacks (active in=battlefield)
    • Affected (first 3): Vengeful Ancestor
  • 1 card · ➖ trigger/Whenever a goaded creature deals combat damage to one of your opponents · removed: Whenever a goaded creature deals combat damage to one of your opponents (active in=battlefield)
    • Affected (first 3): The Rani
  • 1 card · ➖ trigger/Whenever one or more goaded creatures deal combat damage to one of your opponen… · removed: Whenever one or more goaded creatures deal combat damage to one of your opponents (active in=battlefield)
    • Affected (first 3): Puppet Master, String Puller

🟡 Modified fields (5 signatures)

  • 1 card · 🔄 static/CantBlock · changed field affects: selfgoaded opponent controls creature
    • Affected (first 3): Bothersome Quasit
  • 1 card · 🔄 ability/Investigate · changed field repeat_for: # of each opponent
    • Affected (first 3): Ethereal Investigator
  • 1 card · 🔄 ability/Investigate · changed field repeat_for: # of each opponent who lost life this turn
    • Affected (first 3): Teysa, Opulent Oligarch
  • 1 card · 🔄 ability/Investigate · changed field repeat_for: # of each opponent whose HandSize { player: ScopedPlayer } GT Ref { qty: HandSize { player: Controller } }
    • Affected (first 3): Wojek Investigator
  • 1 card · 🔄 ability/Investigate · changed field repeat_for: # of goaded you control creature
    • Affected (first 3): Serene Sleuth

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes — the player-set lift is at the right chain-lowering seam, but it deliberately leaves the same fieldless investigate for each … grammar rules-incorrect for object sets.

🔴 Blocker

  • crates/engine/src/parser/oracle_effect/lower.rs:3581-3586 accepts only QuantityRef::PlayerCount; the new crates/engine/tests/integration/teysa_wojek_investigate_per_opponent.rs:404 regression expressly permits object-set investigate for each to remain without a repeat count. The verified Oracle text for Serene Sleuth is “investigate for each goaded creature you control,” and Sophina, Spearsage Deserter says “investigate once for each nontoken attacking creature.” repeat_for already models an integer count, so neither needs per-object binding: both must create one Clue per matching creature, not silently resolve as one Investigate. CR 701.16a defines investigate as creating a Clue; CR 608.2c requires following the full instruction. Generalize the fieldless-effect suffix lift to retain every supported QuantityRef (including ObjectCount) and add a runtime discriminator for at least one object-set card.

🟡 Non-blocking

  • The current-head <!-- coverage-parse-diff --> artifact says “No card-parse changes detected,” whereas the PR comment claims exactly {Teysa, Wojek} (+2). This prevents the manual Quality Gate from passing or a quality label; it is not evidence that the behavior change is absent. Reconcile the claimed parse-impact statement after the blocker is fixed.

✅ Clean

  • The change is in parse_effect_chain_ir, the appropriate repeat attachment seam, and the Teysa/Wojek tests exercise real parse and scenario-resolution paths.

Recommendation: extend the lift across the complete supported fieldless-Investigate for each class (player and object counts), then re-request review with the regenerated parse-diff evidence.

@matthewevans matthewevans self-assigned this Jul 17, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The current-head parse-diff sticky says “No card-parse changes detected,” but that is a coverage false-green rather than evidence that this change is inert.

This PR attaches repeat_for to otherwise fieldless Effect::Investigate for the Teysa/Wojek player sets (oracle_effect/mod.rs), so the parse tree materially changes. The coverage parse tree cannot report it: build_ability_item merges effect_details and ability_details, but ability_details does not project AbilityDefinition::repeat_for (crates/engine/src/game/coverage.rs:3592), and Effect::Investigate deliberately has no effect detail (:3521). The separate feature extractor does recognize repeat_for (:7050), but that does not feed the parse-diff sticky.

Please project repeat_for (including its quantity) into ability parse details and add a regression proving this Teysa/Wojek class produces a parse-diff signature. Then the CI artifact can serve as the claimed card-level evidence.

@matthewevans matthewevans removed their assignment Jul 17, 2026
lgray added 2 commits July 17, 2026 16:06
…stigate

Lift a trailing "[once] for each <player-set>" clause on a fieldless
Effect::Investigate into repeat_for = PlayerCount{filter}:

- Teysa, Opulent Oligarch: "investigate for each opponent who lost life
  this turn" → PlayerCount{OpponentLostLife}
- Wojek Investigator: "investigate once for each opponent who has more
  cards in hand than you" → PlayerCount{PlayerAttribute{Opponent,
  HandSize{ScopedPlayer}, GT, HandSize{Controller}}}

EDIT 1 extracts a shared split_for_each_suffix primitive (byte-identical
strip refactor) plus a PlayerCount-gated for_each_player_set_repeat_for
lift, consumed at the chunk-loop seam. EDIT 2 adds the comparative
hand-size who-clause combinator and widens the player-attribute operand
to QuantityExpr.

Parser-only: runtime resolution rides the existing filter-agnostic
repeat_for driver (CR 608.2c). Object-ranged "investigate for each"
(Serene Sleuth, Sophina) stays on the unchanged path.

Assisted-by: ClaudeCode:claude-opus-4.8
…r-count class

Responds to the phase-rs#6110 review: the fieldless-Investigate "for each ⟨set⟩"
lift was gated to QuantityRef::PlayerCount only, so object-ranged cards
(Serene Sleuth: "investigate for each goaded creature you control") dropped
to a bare 1-Clue Investigate (rules-wrong, CR 701.16a/608.2c).

- Gate-widen: rename for_each_player_set_repeat_for -> for_each_repeatable_repeat_for
  and widen the single-seam eligibility gate to the member-count class
  matches!(qty, PlayerCount{..} | ObjectCount{..}) with a fail-closed _ => None.
  Preserves Teysa/Wojek (PlayerCount) + Serene Sleuth (ObjectCount); a
  contextual amount-ref (e.g. Tamiyo's "investigate twice for each card
  discarded", FilteredTrackedSetSize) is deliberately NOT lifted. Locked by a
  revert-probed negative-boundary test.
- Gap A: add FilterProp::Goaded (candidate-local read of GameObject.goaded_by,
  CR 701.15b/c) across all 15 registration sites, mirroring FilterProp::Renowned.
- ASK 2: project repeat_for into coverage.rs ability_details (None -> byte-identical).
- Gap B (deferred): Sophina "nontoken attacking creature" strict-failure tag +
  flipping tripwire.

Collateral (measured via regen-both at merge-base 931c2dc, ZERO regressions):
FilterProp::Goaded also correctly parses the "goaded creature" filter on 4
previously-Unknown/degraded cards (Bothersome Quasit CantBlock static; Puppet
Master / The Rani / Vengeful Ancestor triggers). A revert-probed runtime drive
(vengeful_ancestor_goaded_attack_trigger.rs) proves the trigger-subject Goaded
eval resolves against the LIVE attacker's goaded_by (CR 508.2a/603.2), not the
fieldless EventObjectSnapshot -- so these are genuinely supported, not false-supported.

Assisted-by: ClaudeCode:claude-opus-4.8
@lgray
lgray force-pushed the feat/std-dynqty-d-investigate-per-opponent branch from 67da88d to 29e9ad9 Compare July 17, 2026 22:29
@lgray

lgray commented Jul 17, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

Addressed the two requested changes.

Blocker 1 — generalize the fieldless-Investigate "for each" lift beyond PlayerCount. lower.rs now retains the full member-count class matches!(PlayerCount | ObjectCount), and the :404 regression was inverted to require the object-set lift. Serene Sleuth ("investigate for each goaded creature you control") lifts to a runtime-resolved ObjectCount via a new FilterProp::Goaded building block (mirrors FilterProp::Renowned), with the runtime discriminator requested: serene_sleuth_runtime_makes_one_clue_per_goaded_creature — 3 goaded creatures → 3 Clues (vs 5 if the Goaded filter were ignored, vs 1 for a bare Investigate), revert-probed on the production FilterProp::Goaded eval. A fail-closed gate keeps contextual amount-refs out: Tamiyo Meets the Story Circle's "investigate twice for each card discarded" stays byte-identical (lifting it would silently drop the "twice"), locked by a negative-boundary test.

Blocker 2 — project repeat_for into the coverage parse tree. coverage.rs ability_details now pushes ("repeat_for", fmt_quantity(rf)) (None → byte-identical, no spurious diff), with an investigate_signature_exposes_repeat_for regression (Some → present / None → absent, revert-probed). This also resolves the non-blocking parse-diff false-green: a confound-free regen-both (base = feature merge-base) now reports real card-parse changes — 7 cards, all strict improvements, zero regressions (the 3 intended lifts plus 4 collateral goaded-filter cards whose previously-Unknown "goaded creature" filters now parse, with a revert-probed runtime drive proving the trigger-subject Goaded eval resolves against the live object). The claimed parse-impact is reconciled to those 7.

Deferred — Sophina, Spearsage Deserter. It remains a bare Investigate (one Clue). Its "nontoken attacking creature" filter is blocked by a separate, pre-existing parse_type_phrase leading-adjective order-dependence — the nontoken-leads-attacking ordering makes the type parse return None before the lift seam is ever reached, so it is orthogonal to the gate fixed here. It is deferred as a documented strict-failure tag with a flipping tripwire test (the Sophina branch of the inverted :404 regression), rather than silently, because collapsing that fixed adjective cascade into an order-free loop is the hottest shared parser path and warrants its own change.

@matthewevans

Copy link
Copy Markdown
Member

Maintainer update: I have received the new head and its response to the prior requested changes. I am holding the implementation review until GitHub publishes the current-head coverage-parse-diff artifact and the in-progress Rust checks complete; no further contributor action is requested at this point.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Request changes — the object-count runtime fix may be sound, but the current-head parse-diff exposes a 250-card coverage blast radius that the claimed seven-card scope does not account for.

🔴 Blocker

[HIGH] repeat_for coverage projection changes the parse signature of the whole existing repeat-for surface. The current <!-- coverage-parse-diff --> reports 250 changed cards / 158 signatures, including many unrelated existing repeat_for effects (for example CopySpell, Token, Proliferate, SearchLibrary, and dozens of Investigate forms), while the PR describes exactly seven card changes. The new coverage.rs projection therefore makes this focused Investigate/Goaded feature alter card-level parse output across the global repeat-for population.

Why it matters: the advertised parse-impact evidence is no longer a bounded witness for the intended class, and downstream parse-diff consumers now see hundreds of unrelated changes. Suggested fix: restrict the coverage signature change to the intended fieldless-Investigate lift class, or explicitly split and independently validate a deliberate global coverage-schema migration with a complete compatibility/consumer audit and reconciled artifact.

✅ Clean

  • The current artifact does show Serene Sleuth, Teysa, Wojek, and the goaded-filter cards as real changes; this finding concerns the additional 243 cards.

Recommendation: split or narrow the coverage projection, then regenerate the parse-diff and re-request review.

…ate lift class (phase-rs#6110)

The global repeat_for projection in `ability_details` surfaced a new signature
key on every card carrying repeat_for, migrating ~250 parse-identical cards'
coverage signatures at once. Scope it to the lift's own eligibility set — a
fieldless `Effect::Investigate` with a member-count `QuantityRef`
(`PlayerCount`/`ObjectCount`), mirroring `for_each_repeatable_repeat_for` — so
only the intended lift class surfaces. The real `coverage-parse-diff` vs the
merge-base now reports 8 cards / 10 signatures (7 real AST changes + Ethereal
Investigator, a pre-existing Investigate+PlayerCount that is byte-identical),
down from 250. A revert-probed `investigate_signature_exposes_repeat_for`
regression (2 positive + 4 negative cases) locks the scope.

Assisted-by: ClaudeCode:claude-opus-4.8
@lgray

lgray commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

Addressed the coverage parse-diff blast-radius finding.

The repeat_for coverage projection is now scoped to the fieldless-Investigate lift classcoverage.rs ability_details surfaces the repeat_for signature key only for an Effect::Investigate whose repeat_for is a member-count QuantityRef (PlayerCount/ObjectCount), i.e. exactly the eligibility set of the lift (for_each_repeatable_repeat_for). It no longer touches the rest of the repeat_for surface (CopySpell/Token/Proliferate/SearchLibrary, or the pre-existing Fixed/Variable/tracked-set Investigate forms).

Measured with the coverage-parse-diff tool against the merge-base (931c2dc2f, the no-projection baseline), the scoped projection reports 8 cards / 10 signatures (down from the global projection's 250), confound-free (0 oracle-text changes, 0 added/removed cards):

  • 7 real parse changes — the 3 repeat_for lifts (Teysa, Wojek, Serene) + 4 collateral goaded-filter cards (Bothersome Quasit, Puppet Master, The Rani, Vengeful Ancestor).
  • 1 projection-only, AST-identical — Ethereal Investigator, a pre-existing Investigate + PlayerCount repeat_for whose value now surfaces in the scoped signature; its parse is byte-identical to the merge-base (disclosed, not a regression), and it is structurally indistinguishable from the lifts, so no non-hacky scope excludes it.

A revert-probed investigate_signature_exposes_repeat_for regression (2 positive member-count cases project; 4 negatives — None, Fixed, non-member-count Ref, non-Investigate effect — do not) locks the scope. Broadening repeat_for coverage to the whole surface would be a deliberate global coverage-schema migration, deferred out of this focused feature.

@matthewevans matthewevans self-assigned this Jul 18, 2026
@matthewevans matthewevans added the enhancement New feature or request label Jul 18, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

**Request changes — the scoped coverage projection resolves the prior blast-radius finding, but FilterProp::Goaded is now declared snapshot-answerable without snapshotting the designation.

🔴 Blocker

[HIGH] Event/LKI snapshot filtering silently treats a goaded subject as ungoaded. Evidence: crates/engine/src/types/events.rs:301-344 has no goad field, while :562-574 classifies FilterProp::Goaded as Supported; the live predicate reads obj.goaded_by at crates/engine/src/game/filter.rs:4302-4307, and the LKI adapter constructs a ZoneChangeRecord without a goad designation at :1509-1555. Why it matters: any reachable past-event or LKI subject filter using goaded becomes false after the object leaves the battlefield, despite the structural gate representing it as supported. Suggested fix: carry a capture-time goaded fact through the snapshot/record evaluators (and their serialization/construction sites), or classify this predicate unsupported until that full path exists; add a departure/LKI discriminating regression.

Recommendation: request changes for the snapshot contract, then re-run the current-head review.

@matthewevans matthewevans removed their assignment Jul 18, 2026
…ubject reach gate

The event-subject reach-gate classifier (classify_prop in types/events.rs)
marked FilterProp::Goaded as Supported, asserting a goaded event-subject filter
is answerable from an EventObjectSnapshot. It is not: the snapshot carries no
goaded field, and the runtime zone-change-record matcher already fail-closes
Goaded (game/filter.rs). The certificate was fail-open.

Reclassify FilterProp::Goaded as Unsupported, aligning the reach-gate
certificate with the snapshot's actual fields and the runtime's existing
fail-closed behavior, so a future goaded event-subject filter fails the reach
gate loudly rather than silently reading an ungoaded snapshot.

Pure contract + test change with zero behavior/coverage delta: the reach gate is
test-only latent scaffolding (classify_prop/classify_filter_shape have no
production caller), coverage is supplied independently by coverage.rs, and the
live goaded predicate (filter.rs FilterProp::Goaded => !obj.goaded_by.is_empty())
is untouched. card-data.json byte-identical before/after (4-card + full-DB).
Adds a revert-probed test goaded_subject_filter_is_unsupported.

Deferred follow-up (option a): snapshot goad onto EventObjectSnapshot +
ZoneChangeRecord and reclassify back to Supported.

CR 701.15b/c: goad is a designation on the live permanent (its goaded_by set).

Assisted-by: ClaudeCode:claude-opus-4.8
@lgray

lgray commented Jul 18, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

Addressed the [HIGH] EventObjectSnapshot goaded-projection certificate finding (option b — fail-closed reclassify).

classify_prop in types/events.rs classified FilterProp::Goaded as Supported, asserting the event-subject reach gate could answer a goaded filter from an EventObjectSnapshot. It can't: the snapshot carries no goaded field, and the runtime zone-change-record matcher already fail-closes Goaded (game/filter.rs ~:5049, CR 701.15b/c). The certificate was fail-open/inconsistent.

Fix: reclassified FilterProp::GoadedUnsupported, aligning the reach-gate certificate with the snapshot's actual fields and the runtime's existing fail-closed behavior. A future card that prints a goad predicate on an event-subject filter now fails the reach gate loudly (the designed "extend the snapshot + evaluator together" signal) instead of silently reading an ungoaded snapshot.

This is a pure contract + test change with zero behavior/coverage delta:

  • The reach gate is test-only latent scaffolding — classify_prop/classify_filter_shape have no production caller today (referenced only inside events.rs, incl. a #[cfg(test)] helper).
  • Coverage for goaded cards is supplied independently by coverage.rs (FilterProp::Goaded => parts.push("goaded")), not by classify_prop.
  • The live goaded predicate (filter.rs FilterProp::Goaded => !obj.goaded_by.is_empty()) is untouched, so the PR's goaded triggers/statics (Vengeful Ancestor, Puppet Master, The Rani, Bothersome Quasit) resolve exactly as before.

Evidence:

  • Revert-probed test goaded_subject_filter_is_unsupportedTyped{Creature,[Goaded]} classifies Unsupported. Measured revert-probe: returning Goaded to the Supported group flips the test to FAILED (assertion left == right failed, left: Supported, right: Unsupported); restoring → GREEN. Non-vacuous because TypeFilter::Creature classifies Supported, so combine_conjunction does not short-circuit the type axis — the flip is attributable to classify_prop(Goaded) alone.
  • No-regression, byte-identical card-data.json — 4-card spot-check (the 4 goaded cards) and full-DB regen both cmp-identical before/after the change.
  • cargo fmt / cargo clippy -p engine --lib --tests -D warnings clean.

Deferred follow-up (option a): snapshot goad onto EventObjectSnapshot + ZoneChangeRecord (mirroring is_suspected) and reclassify back to Supported — only needed once a card actually prints such a filter. FilterProp::Renowned is a separate, pre-existing concern (its snapshot field is_renowned already exists, so it is consistent at this gate; its only gap is on the distinct zone-change-record path), out of scope here.

@matthewevans matthewevans self-assigned this Jul 18, 2026

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved on the updated current head. The prior blocker is resolved by classifying goaded event-subject filters as snapshot-unsupported, so the existing runtime fail-closed behavior is reflected by the reach gate rather than silently certifying a missing LKI field. The maintainer branch update merged current main without a semantic conflict; merge queue will wait for the restarted checks.

@matthewevans
matthewevans enabled auto-merge July 18, 2026 02:45
@matthewevans matthewevans removed their assignment Jul 18, 2026
@matthewevans
matthewevans added this pull request to the merge queue Jul 18, 2026
Merged via the queue into phase-rs:main with commit 03b7758 Jul 18, 2026
12 checks passed
lgray added a commit to lgray/phase that referenced this pull request Jul 18, 2026
…+ resolution

The DynQty parser fix (e82f871) correctly narrowed Too Evil to Stay Dead's
base branch to "mana value 4 or less", which unmasked a two-layer pre-existing
engine gap that prevented the teamwork "instead" broadening from working:

- Cast-time (CR 601.2b/601.2c/702.194c): additional_cost_paid pre-target
  propagation was kicker-only, so a teamwork/bargain-paid spell built targets
  against the narrow base filter. Generalize the deferral to every
  AdditionalCost-"instead" with a non-empty effective queue (new helper
  build_effective_additional_cost_queue; else-if arms at the live-cast and
  preview gates; begin_target_dependent_additional_cost_declaration made
  exhaustive with the kicker arm byte-identical). The castability precheck is
  renamed additional_cost_instead_spell_has_legal_targets and gated on the same
  requires_additional_cost_declaration_before_targets predicate as the cast-time
  gate, so context-ref "instead" overrides (Bargain "that permanent" =
  ParentTarget, e.g. Torch the Tower) are not misreported castable.

- Resolution (CR 608.2b): apply_instead_swap preserved the parent node's
  targets, which per-node re-validation had emptied under the narrow base filter,
  and discarded the override node's own validated target -> the swapped-in effect
  resolved with zero targets (silent no-op). Take the override's validated
  targets when the parent's were emptied.

Repairs teamwork-cast Too Evil to Stay Dead / Cruel Alliance and kicked
Bloodchief's Thirst (base-illegal/kicked-legal target previously no-op'd).
Bounded: functional surface = 2 teamwork cards + ~6 kicker cards, all no-op ->
correct; other three instead-condition kinds and same-filter/context-ref cases
byte-identical. Engine-only: card-data regen byte-identical (0 parse/coverage
delta). Full cargo test -p engine --lib (17011) + integration (3388) green.

Completes the option-(a) fix for phase-rs#6143 (DynQty series; predecessor phase-rs#6110).

Assisted-by: ClaudeCode:claude-opus-4.8
andriypolanski pushed a commit to andriypolanski/phase that referenced this pull request Jul 18, 2026
…aveyard/zone targets (phase-rs#6143)

* feat(parser): capture mana-value ceiling trailing a zone clause on graveyard/zone targets

Add a post-zone parse_mana_value_suffix second pass in parse_type_phrase_with_ctx,
mirroring the existing zone->counter and zone->without second passes, so a mana-value
clause that TRAILS a zone clause ("target ... card in your graveyard with mana value
less than or equal to ...") is captured on the target filter instead of being silently
swallowed as Swallow:DynamicQty.

Unlocks Squirming Emergence (dynamic Cmc{LE, ZoneCardCount{Graveyard,[Permanent],
Controller}} ceiling) and correctly restricts the whole zone-then-mana-value class:
Lazav the Multifarious + Likeness Looter (Cmc{EQ, Variable(X)} + their now-parsed
', except ...' modifications), Too Evil to Stay Dead's base branch (Cmc{LE, Fixed 4}),
Scrap Trawler (Cmc{LT, ObjectManaValue}), and Devoted Sultai's phrase-level ceiling.

Pre-existing engine gap unmasked (documented at the parser + engine sites, no tracker):
AdditionalCost-'instead' cast-time target-filter broadening is propagated for kicker
only, so Too Evil to Stay Dead's teamwork branch now uses its correctly-narrowed base
filter. Two prior bugs canceled (dropped suffix left the base branch broad); the correct
parser fix unmasks the engine gap. The fix generalizes kicker -> all AdditionalCost-
'instead' (parameterize-don't-proliferate) as a separate engine change.

CR 202.3 (mana value), CR 608.2c (later text modifies earlier), CR 115.2 (targets in a
non-battlefield zone must be specified).

Assisted-by: ClaudeCode:claude-opus-4.8

* fix(engine): resolve additional-cost-"instead" broad targets at cast + resolution

The DynQty parser fix (e82f871) correctly narrowed Too Evil to Stay Dead's
base branch to "mana value 4 or less", which unmasked a two-layer pre-existing
engine gap that prevented the teamwork "instead" broadening from working:

- Cast-time (CR 601.2b/601.2c/702.194c): additional_cost_paid pre-target
  propagation was kicker-only, so a teamwork/bargain-paid spell built targets
  against the narrow base filter. Generalize the deferral to every
  AdditionalCost-"instead" with a non-empty effective queue (new helper
  build_effective_additional_cost_queue; else-if arms at the live-cast and
  preview gates; begin_target_dependent_additional_cost_declaration made
  exhaustive with the kicker arm byte-identical). The castability precheck is
  renamed additional_cost_instead_spell_has_legal_targets and gated on the same
  requires_additional_cost_declaration_before_targets predicate as the cast-time
  gate, so context-ref "instead" overrides (Bargain "that permanent" =
  ParentTarget, e.g. Torch the Tower) are not misreported castable.

- Resolution (CR 608.2b): apply_instead_swap preserved the parent node's
  targets, which per-node re-validation had emptied under the narrow base filter,
  and discarded the override node's own validated target -> the swapped-in effect
  resolved with zero targets (silent no-op). Take the override's validated
  targets when the parent's were emptied.

Repairs teamwork-cast Too Evil to Stay Dead / Cruel Alliance and kicked
Bloodchief's Thirst (base-illegal/kicked-legal target previously no-op'd).
Bounded: functional surface = 2 teamwork cards + ~6 kicker cards, all no-op ->
correct; other three instead-condition kinds and same-filter/context-ref cases
byte-identical. Engine-only: card-data regen byte-identical (0 parse/coverage
delta). Full cargo test -p engine --lib (17011) + integration (3388) green.

Completes the option-(a) fix for phase-rs#6143 (DynQty series; predecessor phase-rs#6110).

Assisted-by: ClaudeCode:claude-opus-4.8

* fix(PR-6143): preserve override target set

Keep the override node’s complete validated targets whenever it owns a declared target filter; preserve parent targets for context-reference overrides.

Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>

---------

Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants