feat(analysis): combo-graph effect/trigger breadth + life-symmetry cost (Engine B, PR-4b) - #4534
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the PR-4b phase of the static analysis engine, adding support for projecting and modeling various game effects and trigger modes (such as Draw, Mill, Life, Tokens, Zone-Changes, Sacrifice, and Extra Turns/Phases) onto resource event axes. It also updates trigger axis mapping to exhaustively handle these new event triggers and introduces comprehensive unit tests for aristocrats and lifegain feedback loops. The review feedback suggests two improvements: enhancing type_filters_exclude_creature to support disjunctive non-creature constraints (e.g., Non(AnyOf(...))) to avoid spurious Death edges, and updating the Effect::Conjure projection to handle multi-card and variable-count conjuring rather than hardcoding a single ETB event.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 2b8699723a066db142c20f3ed6e389411fb75620 in worktree /tmp/forge-pr-4534, including the unresolved review threads and the only changed file (crates/engine/src/analysis/ability_graph.rs). I agree this needs another pass before it is ready.
[MED] type_filters_exclude_creature only recognizes direct Non(Creature) and misses composed exclusions. Evidence: crates/engine/src/analysis/ability_graph.rs:397-400 only checks for TypeFilter::Non(inner) where inner is exactly Creature, while the engine type model supports TypeFilter::Non(Box<TypeFilter>) and TypeFilter::AnyOf(Vec<TypeFilter>) (crates/engine/src/types/ability.rs:2434-2458). A filter like Non(AnyOf([Creature, ...])) also provably cannot match a creature, but this helper will still project Sacrifice/Destroy as producing Death. Please make this reasoning recursive over the existing TypeFilter composition shape, preserving the conservative behavior for positive non-creature card types.
[MED] Effect::Conjure projection collapses all battlefield conjures to one fixed ETB. Evidence: crates/engine/src/analysis/ability_graph.rs:771-773 ignores the cards field and always calls b.add_etb(1, AxisMagnitude::Fixed(1)), but Effect::Conjure carries cards: Vec<ConjureCard> and each ConjureCard has a count: QuantityExpr (crates/engine/src/types/ability.rs:7509-7521, 10462-10465). Multi-card or counted conjure effects will be under-projected in the combo graph. Please aggregate the fixed counts where possible and mark variable/unknown quantities conservatively, similar to the existing count_seed path used by token creation.
…r-card conjure ETB Addresses the phase-rs#4534 maintainer review (both MED): - `type_filters_exclude_creature` now recurses through `Non`/`AnyOf` composition, so a composed exclusion like `Non(AnyOf([Creature, …]))` ("neither a creature nor …") is recognized — was a direct `Non(Creature)` match only. Implemented as mutually-recursive `type_filter_excludes_creature` / `type_filter_matches_all_creatures` predicates with exhaustive (no-wildcard) matches over the full `TypeFilter` shape. Stays conservative for positive card types/subtypes (CR 205.2b — a creature can also be a land/artifact/etc.), so real dies edges are never dropped. - `Effect::Conjure` projection now seeds the ETB axis per `ConjureCard` via `count_seed` (was a single fixed ETB), so multi-card / counted / variable-count conjures are projected accurately (CR 603.6a), mirroring the token-creation `count_seed` path. Test: `type_filter_excludes_creature_handles_composed_negation` (discriminating — the pre-fix direct match returns false for `Non(AnyOf([Creature, Land]))`). cargo test -p engine 14008/0; clippy -D warnings 0. Assisted-by: ClaudeCode:claude-opus-4.8
2b86997 to
e5511a1
Compare
|
🤖 AI text below 🤖 Both addressed in [MED]
So [MED]
|
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head e5511a13af2bc41628d0f00fd55f0ab5e1e384df. The two issues from my prior review look addressed, but one sibling gap remains.
[MED] LifeLostAll should consume the Life axis too. Evidence: crates/engine/src/analysis/ability_graph.rs:1047 maps LifeLost/LifeChanged/PayLife to AxisKey::Life, but crates/engine/src/analysis/ability_graph.rs:1099 leaves LifeLostAll in the inert None bucket; runtime treats LifeLostAll as the same life-loss event class in crates/engine/src/game/trigger_matchers.rs:63 and indexes it with life-change triggers in crates/engine/src/game/trigger_index.rs:259. Why it matters: Engine B will miss candidates whose consumer is the all/batched life-loss trigger form even though this PR now models the corresponding life-loss producers. Suggested fix: include TriggerMode::LifeLostAll in the Life branch and add a discriminating trigger_axis or candidate-cycle test for a LifeLostAll trigger fed by a life-loss producer.
…st (Engine B, PR-4b) Extends the static ability-graph combo-candidate extractor (PR-4a) with broader effect/trigger projection coverage so more loop classes are detected: - Effect projections: Draw (cards-drawn axis), Mill (negative opponent library), GainLife / LoseLife (sign-aware per-player life), token creation (tokens + ETB), Sacrifice / Destroy / ChangeZone (sac/LTB/death-trigger disambiguation by filter and zone transition), extra turn / extra combat phase axes. - Trigger axes: ChangesZone (ETB vs death vs LTB), event triggers requiring their axes. - Cost: PayLife folds as the cost half of the R3-LIFE-SYMMETRY pair (with GainLife/LoseLife) — its own fold_cost arm, CR 119.4. Composite stays AND-fold, OneOf stays the disjunctive fold_one_of (PR-4a, CR 118.12a). Detects e.g. dies-token aristocrats loops and lifegain-feedback loops as candidates. fold_cost / project_mana_production / trigger_axis remain exhaustive no-wildcard drift gates; the model uses the typed ModelCompleteness enum (PR-4a). Tests: 17 PR-4b breadth/fixture tests + the 4 PR-4a-review tests; ability_graph 40/40. cargo test -p engine 14007/0; clippy --workspace --features engine/proptest -D warnings = 0. All CR annotations grep-verified. Assisted-by: ClaudeCode:claude-opus-4.8
…r-card conjure ETB Addresses the phase-rs#4534 maintainer review (both MED): - `type_filters_exclude_creature` now recurses through `Non`/`AnyOf` composition, so a composed exclusion like `Non(AnyOf([Creature, …]))` ("neither a creature nor …") is recognized — was a direct `Non(Creature)` match only. Implemented as mutually-recursive `type_filter_excludes_creature` / `type_filter_matches_all_creatures` predicates with exhaustive (no-wildcard) matches over the full `TypeFilter` shape. Stays conservative for positive card types/subtypes (CR 205.2b — a creature can also be a land/artifact/etc.), so real dies edges are never dropped. - `Effect::Conjure` projection now seeds the ETB axis per `ConjureCard` via `count_seed` (was a single fixed ETB), so multi-card / counted / variable-count conjures are projected accurately (CR 603.6a), mirroring the token-creation `count_seed` path. Test: `type_filter_excludes_creature_handles_composed_negation` (discriminating — the pre-fix direct match returns false for `Non(AnyOf([Creature, Land]))`). cargo test -p engine 14008/0; clippy -D warnings 0. Assisted-by: ClaudeCode:claude-opus-4.8
…4534 review) `trigger_axis` left `TriggerMode::LifeLostAll` in the inert `None` bucket while mapping `LifeLost`/`LifeChanged`/`PayLife` to `AxisKey::Life`. The runtime treats `LifeLostAll` as the same life-loss event class — `match_life_lost` handles both (`trigger_matchers.rs`) and `trigger_index` groups it with the life-change triggers — so Engine B missed candidates whose consumer is the batched/all life-loss trigger fed by a life-loss producer (the producers this PR already models). Move `LifeLostAll` into the Life branch. Test: `life_lost_all_trigger_consumes_life_axis` (discriminating — pre-fix it sat in the `None` bucket → `None`). cargo test -p engine 14011/0; clippy -D warnings 0. Assisted-by: ClaudeCode:claude-opus-4.8
|
🤖 AI text below 🤖 Good catch on the sibling gap — fixed in the latest push (rebased onto current [MED] Added
|
e5511a1 to
822c061
Compare
matthewevans
left a comment
There was a problem hiding this comment.
Approved current head 822c061e11bc6eda6cd9065c34f2b99fa1c5260a after re-review. The prior LifeLostAll trigger-axis blocker is fixed; the earlier recursive non-creature filter and per-card Conjure ETB findings remain addressed; exact-head CI is green.
Evidence checked: one-file backend diff in crates/engine/src/analysis/ability_graph.rs; unresolved review-thread gate is clear; no workflow/instruction/security-sensitive paths; no duplicate open PR surfaced by title/issue scan.
🤖 AI text below 🤖
Combo-detector series
Part of the staged, offline-first infinite-combo / loop detector. Each PR links its predecessor so humans can follow the implementation trail end to end.
ResourceVector+ modulo-resource loop equality (additive, no behavior change).GameRunner::actfeedingResourceVector.detect_loop→LoopCertificate+ loop-certificate corpus harness.cargo combo-verifyCLI over the 53-row corpus.Predecessor: PR-4a — #4493 — #4493 (the static ability-graph extractor this PR broadens — more effect/trigger families + the life-symmetry cost half).
Engine B, PR-4b — combo-graph effect/trigger breadth + life-symmetry cost
Builds on PR-4a (#4493, the static ability-graph combo-candidate extractor) by broadening effect/trigger projection so more loop classes are detected, plus the cost half of the life-symmetry pair.
Effect projections
Draw (cards-drawn axis), Mill (negative opponent library), GainLife / LoseLife (sign-aware, per-player), token creation (tokens + ETB), Sacrifice / Destroy / ChangeZone (sac / LTB / death-trigger disambiguation by filter and zone transition), and extra-turn / extra-combat-phase axes.
Trigger axes
ChangesZone disambiguation (ETB vs death vs LTB) and event triggers requiring their axes.
Cost
PayLifenow folds as the cost half of the R3-LIFE-SYMMETRY pair (withGainLife/LoseLife) — its ownfold_costarm (CR 119.4), so a gain-and-pay-life loop isn't vetoed as net-negative.Compositestays AND-fold;OneOfstays the disjunctivefold_one_offrom PR-4a (CR 118.12a).Together these detect e.g. dies-token aristocrats loops and lifegain-feedback loops as candidates.
Drift gates / typing
fold_cost,project_mana_production, andtrigger_axisremain exhaustive no-wildcard drift gates; model completeness uses the typedModelCompletenessenum (PR-4a). This PR is rebased onto currentmain(which includes the merged PR-4a review fixes —fold_one_of,ModelCompleteness, net-signaxis_key_to_resource); the reconciliation merged PR-4b's breadth with those structures and migrated the one test that referenced the removedany_unmodeledfield.Tests
17 PR-4b breadth/fixture tests + the 4 PR-4a-review tests; ability_graph module 40/40.
cargo test -p engine14007 passed / 0 failed;cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings= 0. All CR annotations grep-verified.