feat(analysis): static ability-graph combo-candidate extractor (Engine B, PR-4a) - #4493
Conversation
There was a problem hiding this comment.
Code Review
This pull request introduces Engine B, a static ability-graph extractor that generates offline candidate cycles by building a directed resource graph and finding strongly-connected components. It also refactors the controller-scoped net-progress logic in loop_check.rs and resource.rs to be shared between Engine A and Engine B. The review feedback highlights two violations of the style guide's prohibition on boolean fields (R2) for tracking model completeness, and identifies a correctness issue where player-keyed axes are hardcoded to the opponent instead of dynamically resolving the active player ID.
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.
| /// Axes this node costs/needs + the trigger-event axis that fires it. | ||
| pub requires: BTreeSet<AxisKey>, | ||
| /// ≥1 collected effect was `Unmodeled` (candidate confidence flag). | ||
| pub any_unmodeled: bool, |
There was a problem hiding this comment.
R2 Violation: Use of bool field any_unmodeled
According to the Repository Style Guide (R2), bool fields should not be used to express the design space. Instead, we should parameterize with a typed enum to carry more meaning and allow for future extensibility.
Consider introducing a ModelCompleteness enum:
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum ModelCompleteness {
FullyModeled,
ContainsUnmodeled,
}And replacing any_unmodeled: bool with completeness: ModelCompleteness in both AbilityNode and CandidateCycle.
References
- R2. No bool fields — parameterize with existing typed enums (link)
| /// Tentative win classification. | ||
| pub win_kind: WinKind, | ||
| /// ≥1 member node had unmodeled effects (lower confidence). | ||
| pub any_unmodeled: bool, |
There was a problem hiding this comment.
R2 Violation: Use of bool field any_unmodeled
According to the Repository Style Guide (R2), bool fields should not be used to express the design space. Consider replacing any_unmodeled: bool with the ModelCompleteness enum suggested on AbilityNode.
References
- R2. No bool fields — parameterize with existing typed enums (link)
| AxisKey::Damage => Some(ResourceAxis::DamageDealt(OPPONENT)), | ||
| AxisKey::Life => Some(ResourceAxis::Life(OPPONENT)), | ||
| AxisKey::Library => Some(ResourceAxis::LibraryDelta(OPPONENT)), |
There was a problem hiding this comment.
Correctness: Player-keyed axes should dynamically resolve the correct player ID
Currently, AxisKey::Damage, AxisKey::Life, and AxisKey::Library are hardcoded to map to OPPONENT in axis_key_to_resource. While this works for opponent-directed loops (like drain or mill), it is incorrect for controller-directed loops (such as unbounded lifegain or self-mill advantage engines).
By dynamically inspecting the net resource vector, we can correctly attribute the unbounded axis to either CONTROLLER or OPPONENT based on which player's resource is actually moving.
AxisKey::Damage => {
if net.damage_dealt.get(&CONTROLLER).copied().unwrap_or(0) > 0 {
Some(ResourceAxis::DamageDealt(CONTROLLER))
} else {
Some(ResourceAxis::DamageDealt(OPPONENT))
}
}
AxisKey::Life => {
if net.life.get(&CONTROLLER).copied().unwrap_or(0) > 0 {
Some(ResourceAxis::Life(CONTROLLER))
} else {
Some(ResourceAxis::Life(OPPONENT))
}
}
AxisKey::Library => {
if net.library_delta.get(&CONTROLLER).copied().unwrap_or(0) != 0 {
Some(ResourceAxis::LibraryDelta(CONTROLLER))
} else {
Some(ResourceAxis::LibraryDelta(OPPONENT))
}
}
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the large Engine B pass. I found one blocker in the current head.
AbilityCost::OneOf is disjunctive runtime semantics, but fold_cost currently treats it the same as Composite:
AbilityCost::Composite { costs } | AbilityCost::OneOf { costs } => {
for c in costs {
fold_cost(acc, c);
}
}That sums every alternative cost into one node, so a card with {G} or {2} ends up requiring/spending both branches. The existing type docs say OneOf means the paying player chooses one branch, and the runtime/payability code confirms that by detouring to ActivationCostOneOfChoice and using any() payability for sub-costs. For the static candidate generator this is not just a conservative false positive: it can become a false negative by introducing extra required axes or net-negative mana that the real loop never has to pay.
Please model OneOf as alternative cost branches rather than an AND fold. A good regression would build a candidate where one OneOf branch closes the cycle but another branch would add an unrelated/unsustainable requirement; the candidate should still be emitted for the payable branch. Composite can stay as the all-subcost fold.
Evidence checked: current PR head f4d08533f81bdbd194678b3b6c260cfb95c64357, GitHub files API diff, exact-head CI green/CLEAN, and local worktree inspection. Relevant repository evidence: AbilityCost::OneOf docs in types/ability.rs describe one-of-payment semantics; runtime casting.rs routes it through ActivationCostOneOfChoice; cost_payability.rs treats it as payable when any branch is payable.
…elCompleteness enum, player-keyed axes Resolves the maintainer blocker and the gemini findings on phase-rs#4493. BLOCKER: fold_cost treated AbilityCost::OneOf as a Composite AND-fold, summing every alternative branch (a {G} or {2} cost required BOTH) and inventing required axes / net-negative mana no single branch pays -> false negatives in candidate generation. Split OneOf into fold_one_of: the optimistic envelope over branches (produces union, requires intersection, net per-axis max with missing-component=0, unbounded union, completeness join). A static proposer maximizes recall; Engine A is the sound confirmer that filters false positives. CR 118.12a (disjunctive alternative cost); Composite stays the conjunctive total-cost fold (CR 601.2h). any_unmodeled: bool -> ModelCompleteness enum (FullyModeled / ContainsUnmodeled) on AbilityNode, CandidateCycle, NodeAcc, with an exhaustive lattice-join merge() (no wildcard, mirroring the drift-gate discipline). axis_key_to_resource now resolves CONTROLLER vs OPPONENT for Damage/Life/Library from net sign. Tests: OneOf envelope discrimination (revert to AND-fold => candidate count 1->0), per-axis-max + requires-intersection, player resolution per axis, completeness merge. clippy -D warnings: 0; analysis::ability_graph: 23/23. Assisted-by: ClaudeCode:claude-opus-4.8
|
🤖 AI text below 🤖 Thanks for the careful review. All four items are addressed in BLOCKER — Regression added as you suggested: a candidate where one
Player-keyed axes (gemini). clippy |
…e B, PR-4a) Add `analysis::ability_graph` — an offline, static extractor that builds a directed ability/resource graph from card-face ASTs, finds SCCs (Tarjan), and emits coverable candidate cycles naming the unbounded ResourceAxis each would pump. This is Engine B (the over-approximate proposer); Engine A (detect_loop) remains the sound confirmer. Either runs standalone. PR-4a is the graph scaffold + the 5 priority effect families (mana, counter, damage, tap, cast) + SCC + candidate coverability. Edge keying uses a single color-agnostic AxisKey::Mana (a deliberate recall-first over-approximation; Engine A's exact color accounting filters off-color cases downstream) and a single AxisKey::Landfall. Four exhaustive no-wildcard classifiers are the compile-time drift gates: effect_projection over all 207 Effect variants, trigger_axis over all 169 TriggerMode variants, From<&ResourceAxis> over all 16 ResourceAxis variants, and the AbilityCost fold over all 29 variants — a new combo-relevant variant is a compile error until classified. The candidate coverability check (with a PR-4-only unbounded-production override) is layered ON TOP of a shared, behavior-preserving extraction: net_progress_for / unbounded_axes_for move from private loop_check fns to pub(crate) ResourceVector methods that detect_loop now delegates to — Engine A behavior is unchanged. CandidateCycle is a distinct type from LoopCertificate (an unconfirmed candidate must never masquerade as a sound certificate). CR 500.8 (extra phases), CR 701.21a (sacrifice), CR 701.26a/b (tap/untap), CR 732.2a (loop shortcut), among others (grep-verified). Remaining effect families, trigger-event edge breadth, and the life axis land in PR-4b. Assisted-by: ClaudeCode:claude-opus-4.8
…elCompleteness enum, player-keyed axes Resolves the maintainer blocker and the gemini findings on phase-rs#4493. BLOCKER: fold_cost treated AbilityCost::OneOf as a Composite AND-fold, summing every alternative branch (a {G} or {2} cost required BOTH) and inventing required axes / net-negative mana no single branch pays -> false negatives in candidate generation. Split OneOf into fold_one_of: the optimistic envelope over branches (produces union, requires intersection, net per-axis max with missing-component=0, unbounded union, completeness join). A static proposer maximizes recall; Engine A is the sound confirmer that filters false positives. CR 118.12a (disjunctive alternative cost); Composite stays the conjunctive total-cost fold (CR 601.2h). any_unmodeled: bool -> ModelCompleteness enum (FullyModeled / ContainsUnmodeled) on AbilityNode, CandidateCycle, NodeAcc, with an exhaustive lattice-join merge() (no wildcard, mirroring the drift-gate discipline). axis_key_to_resource now resolves CONTROLLER vs OPPONENT for Damage/Life/Library from net sign. Tests: OneOf envelope discrimination (revert to AND-fold => candidate count 1->0), per-axis-max + requires-intersection, player resolution per axis, completeness merge. clippy -D warnings: 0; analysis::ability_graph: 23/23. Assisted-by: ClaudeCode:claude-opus-4.8
Rebasing onto current upstream/main surfaced two variants the exhaustive no-wildcard drift gates did not yet cover: - ManaProduction::AnyCombinationOfObjectColors -> project_mana_production's count-driven colorless-sentinel group (color-flexible, count-seeded). - TriggerMode::EntersOrHauntedCreatureDies -> trigger_axis's deferred-to-PR-4b None group, beside its sibling HauntedCreatureDies (zone-entry/death events are not modeled as producers in PR-4a). No wildcard added; cargo check confirmed these were the only two. Verified with the full CI command (clippy --workspace --features engine/proptest + test-engine). Assisted-by: ClaudeCode:claude-opus-4.8
98c1bdb to
3a4e9e5
Compare
|
🤖 AI text below 🤖 Rebased onto current |
matthewevans
left a comment
There was a problem hiding this comment.
Reviewed current head 3a4e9e5. Prior OneOf blocker is resolved with a disjunctive envelope plus discriminating regression coverage; Gemini R2/player-axis findings are addressed on head. Exact-head CI is green and local merge check with current main is conflict-free.
|
🤖 AI text below 🤖 Thanks for the review. The
Also addressed in the same head (gemini review comments):
CI is green on the current head. Could you re-review |
🤖 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-3 — #4480 — #4480 (Engine A's live mandatory-loop winner shortcut; Engine B introduced here is the over-approximate static proposer that Engine A's sound confirmer downstream-filters).
What
Adds
analysis::ability_graph— Engine B of the infinite-combo detector: an offline, static extractor that, given a list of card faces, builds a directed ability/resource graph, finds strongly-connected components (Tarjan), and emits candidate cycles naming the unboundedResourceAxiseach would pump. This is the over-approximate proposer; Engine A (detect_loop, already shipped) remains the sound, stateful confirmer. Either runs standalone. Purely additive — zero game-behavior change.This is PR-4a of the static extractor, sliced from PR-4: the graph scaffold + the five priority effect families (mana, counter, damage, tap, cast) + SCC + candidate coverability. The remaining effect families, trigger-event edge breadth, and the life axis land in PR-4b.
Design highlights
effect_projectionover all 207Effectvariants (5 modeled, restUnmodeled)trigger_axisover all 169TriggerModevariants (Some-set = {cast, counter, tap, mana})impl From<&ResourceAxis> for AxisKeyover all 16ResourceAxisvariantsAbilityCostfold over all 29 variants (polarity/sign-aware)AxisKey::Mana(an off-color cost satisfied by off-color production is a deliberate, recall-safe false positive that Engine A's exact floating-pool color accounting filters downstream) and a singleAxisKey::Landfall.net_progress_for/unbounded_axes_formove from privateloop_checkfns topub(crate)ResourceVectormethods thatdetect_loopnow delegates to — byte-identical logic. Engine B's PR-4-onlyunbounded_productioncoverability override lives only inability_graphand never touches Engine A.CandidateCycleis a distinct type fromLoopCertificate— an unconfirmed candidate must never masquerade as a sound certificate (soundness invariant).Comprehensive Rules
Edge/coverability semantics annotated and grep-verified: CR 500.8 (extra phases), CR 701.21a (sacrifice), CR 701.26a/b (tap/untap), CR 732.2a (loop shortcut), CR 106.1 (mana), CR 119.1 (life), CR 120.1 (damage), CR 122.1 (counters), CR 704.5a, among others. (Tarjan / coverability are annotated as CS theory, not CR.)
Testing
ability_graphunit tests + the analysis suite green;cargo fmt/clippy -D warningsclean; fullcargo test -p enginegreen.{Q}-untap-cost → Tap edge, the sacrifice-event producer polarity, and the color-agnostic mana collapse (a colored producer feeding a generic cost forms a candidate — flips to 0 candidates under per-color keying).Mana(Green)from a committed fixture (skips gracefully when the gitignored card export is absent).