fix(ai): score the CR 732.2a loop-shortcut offer + cover the winner-liveness conjunct (#5672 follow-ups) - #5748
Conversation
There was a problem hiding this comment.
Code Review
This pull request implements the LoopShortcutPolicy to govern AI decisions when proposing or declining auto-detected loop shortcuts. The policy prevents the AI from declaring a shortcut that would hand an opponent the win or result in a rollback, while applying a tactical bonus when the AI is the predicted winner. It also registers the policy, introduces configuration settings for the winning declare bonus, and adds extensive integration and unit tests covering complex scenarios such as a predicted winner conceding mid-APNAP window. No review comments were provided for this pull request.
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.
…a losing shortcut PR phase-rs#5672 exposed both `DeclareShortcut` and `DeclineShortcut` at `WaitingFor::LoopShortcut` and deferred the choice "to the policy/search layer" — but no policy ever scored either action. `should_play_now_with_facts` returns 0.5 for both, and the class-bonus table docks `Pass` by 0.1 while `Utility` falls through untouched, so the tactical score preferred `DeclareShortcut` in every game state. On every path where the tactical score IS the whole score — the heuristic-only branch (VeryEasy/Easy, `SearchConfig::default()`, 5-6p pods at <= Medium) and the deadline-expired tactical floor — an AI holding priority on a loop that a DIFFERENT player wins would propose the shortcut and hand that player the game. Add `LoopShortcutPolicy`, a `TacticalPolicy` in the one scoring layer both the search-on and heuristic-only paths consult (`PlannerServices::tactical_score`), so the fix is difficulty-independent by construction rather than a picker special case. The verdict reads `proposer` from `state.waiting_for`, never `ctx.ai_player` — the fail-safe direction, since an `ai_player == proposer` gate would silently drop the veto on any divergence. It rejects exactly two states: - `predicted_winner == Some(w), w != proposer` + `UntilLethal`. `live_mandatory_loop_winner` partitions the living players into fallers and nonfallers and names a winner only when `nonfallers.len() == 1`, so a named winner other than the proposer PROVES the proposer is a faller — a deterministic CR 704.5a / CR 704.5c self-loss, with the opponent crowned per CR 104.2a. - `predicted_winner == None` + `UntilLethal`. Both crown gates test `Some(winner) == predicted_winner`, which is false for every winner when the latch is `None`, so the drive always ends in `until_lethal_fallback` — a full rollback that also clears the re-offer signal. Strictly dominated by declining. Every `Fixed(n)` stays neutral: `materialize_fixed_shortcut` never reads `predicted_winner` and commits each driven cycle, so a count-blind reject would be wrong for the class. The `match` has no wildcard on the count axis, so a future `IterationCount` variant is a compile error rather than a silent mis-gate. `DeclineShortcut` can never be rejected (the verdict exits on the first `let-else`), which keeps at least one finite score in the softmax. Assisted-by: ClaudeCode:claude-opus-4.8
…umption seam
`apply_confirmed_shortcut` gates the seam on BOTH authorities:
if !is_alive(state, proposal.proposer)
|| proposal.predicted_winner.is_some_and(|w| !is_alive(state, w))
The second conjunct had zero coverage. Both existing concede tests build
offers where `proposer == predicted_winner`, so `!is_alive(proposer)` short
circuits first and the second conjunct is never evaluated — deleting it flipped
no test.
Covering it needs three things at once, and the obvious fixtures fail all three:
- `IterationCount::Fixed(n)`, not `UntilLethal`. On the `UntilLethal` path the
conjunct is redundant: `live_mandatory_loop_winner` builds its living set from
the same `!is_eliminated` predicate `is_alive` uses, so a departed winner can
never be re-derived, and both crown gates re-filter on `predicted_winner`
anyway. `materialize_fixed_shortcut` never consults `predicted_winner` and
COMMITS each driven cycle, so there the conjunct is the only thing standing
between a departed winner and `n` committed loop cycles.
- A predicted winner who owns no loop enabler. If the winner owns the loop, CR
800.4a exiles it with them, the drive aborts, and the board is untouched with
or without the guard — a vacuous test.
- Equal ABSOLUTE life across the fallers, not merely equal deltas, or the
simultaneity floor refuses to raise an offer at all.
`setup_3p_bystander_winner` satisfies all three. P0's symmetric plague engine
drains every player including P0, P1 is a second faller, and P2's life simply
can't change — so P2 is the sole non-faller and the engine itself latches
`predicted_winner = Some(P2)`, a winner who controls nothing. The test asserts
that latch on the engine-raised offer, which is what makes the fixture
engine-derived rather than hand-injected.
P0 declares `Fixed(3)`; P2 — the winner, not the proposer — concedes inside the
CR 732.2b window; the last living opponent accepts. The proposer is still alive,
so the first conjunct passes and the second is the only thing that can fire.
Revert-probe (measured): delete the `predicted_winner.is_some_and(..)` term and
the engine drives and commits 3 real cycles on the stale proposal —
`life(P1)` 998 -> 995, and the test FAILS `left: 995, right: 998`.
Note the crown/priority assertions are CR 800.4a post-remedy INVARIANTS, not
discriminators: `waiting_for` is `Priority { P0 }` in both arms and `GameOver` is
reached in neither, so they pass with the guard deleted. Only the life-delta
assertions have teeth. The test's doc comment says so, to stop a future
"simplification" from silently vacuuming it out.
Assisted-by: ClaudeCode:claude-opus-4.8
6898fba to
cb2853c
Compare
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
There was a problem hiding this comment.
Approved — the AI now declines loop-shortcut offers that cannot benefit its proposer while preserving winning and fixed-count paths.
🔴 Blocker
- None.
🟡 Non-blocking
- The policy’s configuration knob is intentionally excluded from automatic penalty tuning because loop-shortcut samples are sparse.
✅ Clean
- The shared tactical-policy registry covers both heuristic and search scoring paths.
- Losing and no-crown `UntilLethal` declarations are hard-rejected; proposer wins remain positively scored and `Fixed(n)` remains available.
- Current-head AI/performance/CI checks are green; the parse artifact confirms no card-data change.
Recommendation: approve and enqueue via the merge queue.
🤖 AI text below 🤖
Summary
Two follow-ups to the merged PR-7 (#5672). D1 closes the Blocker-2 gap PR-7 left open: it exposed both loop-shortcut candidates and deferred the choice "to the policy/search layer", but no policy ever scored either — so the AI preferred to propose a shortcut that crowns its opponent. D2 covers the winner-liveness conjunct PR-7's own tests miss (deleting it flipped no test).
No engine game logic changes. D1 is entirely in
phase-ai; D2 is a test against an existing guard. The onlycrates/engine/srcdiff is a one-line comment pointing at the policy that now scores the candidates.Implementation method (required)
Method: /engine-implementer
CR references
CR 732.2a(proposing a shortcut is optional; the offer routes to the priority holder, who need not be the winner),CR 732.2b/CR 732.2c(the APNAP accept-or-shorten window and its consumption),CR 104.2a(a player wins when their opponents have all left),CR 104.3a(concede = leave the game immediately),CR 104.4b(mandatory loop => draw),CR 704.5a/CR 704.5c(life/poison loss — the "faller" axes),CR 800.4a(priority passes to the next player still in the game),CR 101.2("can't" takes precedence — the life-loss-immune bystander),CR 119.8,CR 704.3.All grep-verified against
docs/MagicCompRules.txtbefore being written. (732.2d/732.2edo not exist and are not cited.)D1 — the AI's conservative escape was the dispreferred candidate
candidates.rsemits bothDeclareShortcut(TacticalClass::Utility) andDeclineShortcut(TacticalClass::Pass) atWaitingFor::LoopShortcut, with a comment deferring the choice to the policy layer. No policy scored either. Measured:should_play_now_with_facts->_ => 0.5for both.Pass-> -0.1 (-0.25 underDevelop/PushLethal);Utility-> falls through, +0.0.=> the tactical score preferred
Declare0.5 vs 0.4 in every game state. On every path where the tactical score is the whole score — the heuristic-only branch (VeryEasy/Easy,SearchConfig::default(), 5-6p pods at <= Medium) and the deadline-expired tactical floor — that is the entire decision. An AI holding priority on a loop a different player wins would declare, and hand that player the game.Fix:
LoopShortcutPolicy, aTacticalPolicyin the one scoring layer that both the search-on and heuristic-only paths consult (PlannerServices::tactical_score) — difficulty-independent by construction, not a picker special case.The verdict reads
proposerfromstate.waiting_for, neverctx.ai_player(fail-safe: anai_player == proposergate would silently drop the veto on divergence). It rejects exactly two states:predicted_winnercountSome(w),w != proposerUntilLethallive_mandatory_loop_winnerpartitions the living into fallers/nonfallers and names a winner only whennonfallers.len() == 1=> a named winner != proposer proves the proposer is a faller => deterministic CR 704.5a/704.5c self-loss, opponent crowned per CR 104.2aNoneUntilLethalSome(winner) == predicted_winner, false for every winner when the latch isNone=> alwaysuntil_lethal_fallback= full rollback that also clears the re-offer signal => weakly dominated by decliningSome(proposer)UntilLethalFixed(n)materialize_fixed_shortcutnever readspredicted_winnerand commits each cycle => real board progress; a count-blind reject would be wrong for the classDeclineShortcutcan never be rejected (the verdict exits on the firstlet-else), so a finite score always survives in the softmax. Thematchhas no wildcard on the count axis — a futureIterationCountvariant is a compile error, not a silent mis-gate.D2 — the winner-liveness conjunct had zero coverage
apply_confirmed_shortcutgates the seam on both authorities:Both existing concede tests build offers where
proposer == predicted_winner, so the first conjunct short-circuits and the second is never evaluated. Deleting it flipped no test.Covering it requires three things at once, and the obvious fixtures fail all three:
Fixed(n), notUntilLethal. On theUntilLethalpath the conjunct is redundant —live_mandatory_loop_winnerbuilds its living set from the same!is_eliminatedpredicateis_aliveuses, so a departed winner can never be re-derived, and both crown gates re-filter anyway.materialize_fixed_shortcutnever consultspredicted_winnerand commits each cycle, so there the conjunct is the only thing between a departed winner andncommitted cycles.setup_3p_bystander_winnersatisfies all three: P0's symmetric plague engine drains every player including P0, P1 is a second faller, and P2's life can't change — so P2 is the sole non-faller and the engine itself latchespredicted_winner = Some(P2), a winner who controls nothing. The test asserts that latch on the engine-raised offer, which is what makes the fixture engine-derived rather than hand-injected.Verification
cargo fmt --allcargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warningscargo nextest run --profile ci --workspace --exclude phase-tauri --exclude mtgish-import --features engine/proptestevery_policy_penalty_is_tuning_registered_or_explicitly_untunedUNTUNED_POLICY_PENALTY_FIELDS, notACTIVE_— a game-deciding CR 104.2a crown scalar does not belong in the CMA-ES vector)score_contract_lint+activation_marker_lintcargo ai-gate./scripts/check-parser-combinators.sh(Gate A)./scripts/check-engine-authorities.sh(Gate B)cargo ai-gate— zero delta, proven by a paired controlThe gate emits 1 WARN (red-mirror baseline 70% -> 80%;
W->L=1, L->W=2; sign-test p = 0.3125). Nothing was rebaselined. Instead the gate was re-run withLoopShortcutPolicyunregistered on the same machine, and the control is byte-identical:=> the WARN is pre-existing baseline-vs-machine drift, not this change. (
ai-gateis wall-clock-deadline bounded, so it is not bit-reproducible; one red-mirror seed also aborts on an unrelated, pre-existingKappa CannoneerManaPaymentpanic — pre-existing, not introduced here.) Our delta is exactly zero, as expected: the policy returnsneutral(0.0)at every non-LoopShortcutstate, and no gate fixture reaches one.Discriminating tests — 10 revert-probes, all measured
Every behavioral claim has a test that fails without the change. Measured, not predicted:
predicted_winner.is_some_and(..)conjunctlife(P1)left: 995, right: 998Some(P2)->Some(P0))left: Some(PlayerId(2)), right: Some(PlayerId(0))— proves the offer is engine-latched on a bystander winnerLoopShortcutPolicy(heuristic)left: Some(DeclareShortcut { .. }), right: Some(DeclineShortcut)the Reject must survive the tactical_weight multiply, got -9999.925...(finite, not-inf)declare = 0.5, decline = 0.4DeclineShortcutexpected Score, got Reject(the NaN guard has teeth)must be vetoed, got Score { delta: 8.0, ... }winner != proposerguardexpected Score, got Reject ...hands_opponent_the_win(None, UntilLethal)rejectmust be vetoed, got Score { delta: 0.0, ... }IterationCountgate from both reject armsexpected Score, got Reject ...untillethal_cannot_crown(the over-rejection class guard)Two assertions are deliberately labelled non-discriminating invariants in their own doc comments, because they pass with the change reverted and a future "simplification" could otherwise vacuum the tests out:
waiting_forisPriority { P0 }in both arms andGameOveris reached in neither, so only the life-delta assertions discriminate.is_infinite() && is_sign_negative()assertion discriminates.Gate A
Gate A PASS head=6898fba8bc0b66893763b6c6e6e9195b3c57cc15 base=3b52c67a9025cd20da8b68243667eb8cdad76060
Anchored on
crates/phase-ai/src/policies/x_cast_gate.rs:55-72— anactivation-constantpure state-machineTacticalPolicythat routes a hard veto throughPolicyVerdict::rejectrather than a sentinel score. Same seam, same shape.crates/phase-ai/src/policies/anti_self_harm.rs:100-136— a reject gated on the candidate action (let ... else { neutral }), which is the pattern that keepsDeclineShortcutunrejectable here.Final review-impl
Final review-impl PASS head=6898fba8bc0b66893763b6c6e6e9195b3c57cc15
Pre-existing issue found (NOT fixed here — out of scope, needs its own PR)
While proving D2's reachability we measured a pre-existing DoS shipped with #5672, untouched by this PR:
IterationCount::Fixed(u32)carries an unboundedu32.game_action_payload_guard.rsdestructurescountaway with..and bounds onlytemplate.decisions. Its comment asserts "countis a small enum — nothing unbounded" — which is false, and is why the guard skips it.handle_declare_shortcutadds no validation, andmaterialize_fixed_shortcut'sfor i in 0..nhas no outer cap (cycle_beat_capbounds beats within a cycle, not the cycle count).=> a hostile client can send
DeclareShortcut { count: Fixed(u32::MAX), template: None }and drive ~4.3e9GameStateclones on the multiplayer server.This PR does not touch it — a payload-guard change belongs in its own PR with its own review. It is flagged here rather than buried; the only thing changed is a comment in our own test that had wrongly claimed the server bounds
n. A follow-up should boundcountat the payload guard.Combo-detector series
ability_rw.rs, latent CR 603.3b fix).LoopCertificate-> top line).Predecessor: PR-7 — #5672 — #5672 — which shipped the CR 732.2a offer, the APNAP window, and the consumption seam. This PR closes the scoring gap it left open (the candidates were exposed but never scored) and covers the liveness conjunct its tests miss.