feat: combo-detector PR-7 — loop-shortcut offer + APNAP response window + combo-declaration UI (CR 732.2a–c)#5672
Merged
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Code Review
This pull request implements the interactive loop-shortcut protocol (Phase 3) and trigger-ordering templates (B2) to prevent redundant prompts on simultaneous triggers. It also re-keys poison counters to be per-victim for correct HUD attribution. The code review feedback identifies that Effect::Clash must be classified as randomness-bearing under CR 732.2a to prevent illegal shortcuts. Additionally, a redundant nullish coalescing guard in LoopShortcutModal.tsx should be removed, and several bool fields (take, pay, and uses_buyback) must be refactored into typed enums to comply with repository style rule R2.
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.
Parse changes introduced by this PR✓ No card-parse changes detected. |
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 12, 2026
…2a), R2 typed enums, FE dead code
- Classify Effect::Clash as randomness-bearing: CR 701.30a reveals the top of a shuffled
library (hidden info at pin time) and CR 701.30d decides the winner by revealed mana value,
so a loop whose recast body contains a clash is not shortcut-eligible under CR 732.2a. Moved
the false->true arm in effect_is_randomness_bearing + added a discriminating assertion to
randomness_classifier_discriminates (revert-probe: moving it back flips the assertion).
- R2 (no bool fields): PinnedDecision/ConcreteDecision take/pay bool -> MayChoiceOption{Take,
Decline} / UnlessPaymentOption{Pay,Decline}; RecastContext.uses_buyback bool -> BuybackUsage
{Used,NotUsed} with a pays() accessor for the DecideOptionalCost consumer.
- Remove the dead-code '?? []' guard in LoopShortcutModal (schema.points is non-optional).
Assisted-by: ClaudeCode:claude-opus-4.8
lgray
force-pushed
the
feat/combo-detect-pr7
branch
from
a5822e3 to
f883450
Compare
matthewevans
requested changes
Jul 12, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
[HIGH] The shortcut controller is forced to propose the shortcut. Evidence: crates/engine/src/types/actions.rs:797-812 exposes only DeclareShortcut while crates/engine/src/game/engine.rs:4218-4238 accepts only that action for WaitingFor::LoopShortcut; client/src/components/modal/LoopShortcutModal.tsx:52-95 renders a confirm-only modal. Why it matters: CR 732.2a says the priority holder may suggest a shortcut, so an opt-in game can deadlock at this prompt or force a shortcut instead of returning to ordinary priority. The PR description acknowledges the gap, but this is rules behavior, not a deferrable UI refinement. Suggested fix: add a typed controller decline action handled at the engine offer seam; clear the offer's re-detection state, restore priority, and add an end-to-end test proving decline continues normal play without immediate re-offer.
[MED] The modal derives convokeTappable by filtering and counting decision-schema points in the frontend. Evidence: client/src/components/modal/LoopShortcutModal.tsx:75-85. Why it matters: the frontend must render engine-provided game state, not infer a derived game value; this creates a parallel display authority that can drift from the engine's legal-tap semantics. Suggested fix: publish the display count from the engine-owned shortcut schema/view model and render it directly in the adapter/modal.
CR verification: local docs/MagicCompRules.txt:6372-6378 confirms CR 732.2a's optional proposal and the separate other-player response in 732.2b/c. The parse-diff sticky is current for f883450c and reports no card-parse changes. Gemini's five findings are reconciled at current head: Clash is in the randomness arm with a discriminating assertion, the nullish guard is removed, and the three bool payloads are typed.
Repeat-after-feedback note: this review observes the policy's recurring wrong-seam and runtime-test-gap signals; the green checks do not exercise the controller-decline path.
lgray
force-pushed
the
feat/combo-detect-pr7
branch
from
f883450 to
9e5cc22
Compare
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 12, 2026
…2a), R2 typed enums, FE dead code
- Classify Effect::Clash as randomness-bearing: CR 701.30a reveals the top of a shuffled
library (hidden info at pin time) and CR 701.30d decides the winner by revealed mana value,
so a loop whose recast body contains a clash is not shortcut-eligible under CR 732.2a. Moved
the false->true arm in effect_is_randomness_bearing + added a discriminating assertion to
randomness_classifier_discriminates (revert-probe: moving it back flips the assertion).
- R2 (no bool fields): PinnedDecision/ConcreteDecision take/pay bool -> MayChoiceOption{Take,
Decline} / UnlessPaymentOption{Pay,Decline}; RecastContext.uses_buyback bool -> BuybackUsage
{Used,NotUsed} with a pays() accessor for the DecideOptionalCost consumer.
- Remove the dead-code '?? []' guard in LoopShortcutModal (schema.points is non-optional).
Assisted-by: ClaudeCode:claude-opus-4.8
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Verdict: blocked — the loop-shortcut offer is rules-incorrect because its controller cannot decline it; request changes before merge.
🔴 Blocker
crates/engine/src/types/actions.rs:797, crates/engine/src/game/engine.rs:4218, and client/src/components/modal/LoopShortcutModal.tsx:52 expose only a DeclareShortcut/confirm route. The controller is therefore forced to propose a shortcut. CR 732.2a was verified from docs/MagicCompRules.txt: “may suggest a shortcut.” A shortcut proposal is optional; forcing it can replace ordinary priority with an unwanted shortcut (or leave the offer unresolved). Reuse the existing typed GameAction → WaitingFor round trip for an explicit controller-decline action, restore ordinary priority, and prevent immediate re-offering. Add a registered end-to-end test that declines the offer and proves normal play continues.
🟡 Non-blocking
client/src/components/modal/LoopShortcutModal.tsx:75 derives convokeTappable by filtering decision-schema state in React. This is display-state derivation that belongs to the engine-owned shortcut view/schema. Expose the display count from the engine and render it directly.
✅ Clean
The current-head parse-diff artifact reports no card-parse changes. Gemini’s five earlier findings were reconciled at f883450c: the Clash assertion reaches the randomness arm, the nullish guard is gone, and the three payload booleans are typed.
Recommendation: request changes — add the typed decline path and discriminating runtime test, then move the derived modal value to the engine-owned view model.
matthewevans
requested changes
Jul 12, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Verdict: blocked — current head f9e9ed7 still forces an optional shortcut and keeps display-state derivation in React; request changes before merge.
🔴 Blocker
crates/engine/src/types/actions.rs:803 defines DeclareShortcut and the adjacent RespondToShortcut, but no controller-decline action; crates/engine/src/game/engine.rs:4222 accepts only GameAction::DeclareShortcut while waiting on LoopShortcut; and client/src/components/modal/LoopShortcutModal.tsx:54 explicitly renders a confirm-only controller modal. The verified CR 732.2a text says the player with priority “may suggest a shortcut”—the proposal is optional. This offer therefore replaces ordinary priority with a state that has no rules-correct controller-decline transition. Add a typed decline action at the existing WaitingFor::LoopShortcut dispatch seam, restore priority without immediately re-offering the same loop, and add a registered end-to-end test that exercises that path.
🟡 Non-blocking
client/src/components/modal/LoopShortcutModal.tsx:79 derives convokeTappable by filtering and counting schema.points in React. The schema is engine-owned state, but this aggregate display value is still computed in the display layer; publish it in the engine-owned shortcut view/schema and render it directly. This does not change the blocking verdict, but it must be corrected before a clean approval.
✅ Clean
The current-head parse-diff artifact reports “No card-parse changes detected,” so no card Oracle text is claimed or affected by this PR. The current-head change is limited to frontend setup/test-factory cleanup and does not introduce a new security/workflow surface. Gemini's resolved feedback remains reconciled: Clash is randomness-bearing, the redundant nullish guard is removed, and the reviewed bool payloads use typed enums.
Recommendation: request changes — implement and runtime-test the controller decline path, then move the Convoke display aggregate into engine-provided state; do not approve or enqueue while the required checks remain pending.
…ase 1)
Phase 1 (foundations) of combo-detector PR-7: pure/offline analysis-layer primitives, zero live engine wiring, no gate yet.
B1: DecisionTemplate {owner, decisions, replay} + resolve() replay engine + predictability_gate() (CR 732.2a firewall). DecisionSource reuses YieldTarget (CR 117.3d provenance + CR 400.7 identity). ReplayFailure is selected per pin kind: absent Order source -> MissingSource (CR 400.7); illegal/absent target -> IllegalTarget (CR 608.2b). IterationCount::Fixed only; TargetSchedule = {Constant, RoundRobin, Piecewise}.
B4: BoardDelta/ResidualPermanent (CR 110.1) + pure board_delta() producer + structurally-empty LoopCertificate.residual_board_delta. The field is empty by construction for every certificate detect_loop can currently produce (loop_states_equal_modulo_resources requires an identical battlefield); board_delta() is the single population seam that lights up once a future object-growth detection path relaxes that gate -- not a silent stub.
Verification: cargo fmt; clippy --all-targets -D warnings (clean); cargo test -p engine (18543 passed, 0 failed); 12 discriminating inline unit tests. coverage/semantic-audit skipped -- PR-7 touches zero parser/card-data surface so they carry no signal; coverage runs once at final pre-push as a no-regression check.
Deferred: DecisionGroupKey/DecisionKind/key + Ord-on-newtypes -> Phase 2/B2 (first consumer); IndexedClass schedule -> Phase 4/B3 (needs live FilterContext); non-empty residual materialization + board_delta output-order determinism -> object-growth detection phase.
Assisted-by: ClaudeCode:claude-opus-4.8
…7 phase 2)
Phase 2 (B2) of combo-detector PR-7: one resolver at the begin_trigger_ordering
auto-order gate, two tiers over a shared DecisionTemplate, plus the deferred B1
key apparatus.
Ephemeral tier (CORRECTNESS FIX, CR 603.3b): a simultaneous trigger batch is
ordered ONCE. Previously, when a targeted trigger paused for target selection,
the already-ordered deferred tail lost its 'ordered' flag and
drain_deferred_trigger_queue_unchecked re-prompted after every target. Now
handle_order_triggers registers an ephemeral ThisObject-keyed coverage-only
template; the gate's 3rd arm verifies sub-multiset coverage and keeps the tail's
chosen order without re-prompting. Cleared at the batch resolution boundary.
Persistent tier (UX): opt-in save/reapply keyed by AllCopies/oracle identity, via
GameAction::SetTriggerOrderTemplate{Save,Remove,ClearAll} mirroring the merged
session bypass + payload guard + manabrew-compat + per-viewer redaction). Permutes
the fresh batch once, then registers an ephemeral marker so all parked-tail
re-drains are coverage-only for both tiers. Frontend list deferred to phase 5.
B1 key apparatus (deferred from phase 1, first consumer here): DecisionGroupKey/
DecisionKind + DecisionTemplate.key + Ord on ObjectId/CardId + decision_templates
Vec on GameState with get/set/remove/clear accessors. Excluded from
loop_fingerprint, kept in PartialEq (safe direction), per-viewer redacted in
filter_state_for_viewer.
Gate OFF byte-identical: empty decision_templates no-ops the 3rd arm; neither tier
rides LoopDetectionMode.
Verification: cargo fmt; clippy --all-targets -D warnings (clean); cargo test -p
engine (18597 passed, 0 failed, post-rebase); 8 discriminating tests incl. the
headline single-OrderTriggers-prompt on a paused multi-targeted batch.
FORGE ordering_parity_sweep (full-DB, corpus regenerated at the rebased tip): B2's
delta vs main is ZERO. The sweep's decision inputs -- profiles_conflict/
ability_rw_profile (ability_rw.rs), build_resolved_from_def (ability_utils.rs), and
the allowlist (triggers_ordering_parity_tests.rs) -- are byte-identical to main and
the full-DB path never calls the (unmodified-body) group_is_order_independent, so
the run equals main's own full-DB result. It reports 20 PRE-EXISTING over-prompt
divergences (the Urza's-era hidden/veiled/opal/lurking enchantment cycle) that
predate this change; all are the safe auto->prompt direction (zero under-prompts =>
zero CR 603.3b violations). Default CI runs the fixture path (unexplained=0). These
are a pre-existing full-DB-only allowlist gap, out of PR-7 scope, tracked as a
follow-up.
Assisted-by: ClaudeCode:claude-opus-4.8
…-7 phase 3)
Phase 3 (Part A) of combo-detector PR-7: at a CR 704.3 priority point an opt-in
LoopDetectionMode::Interactive routes a confirmed live loop certificate through a
shortcut-offer protocol instead of the pre-feature halt. Default (Off) and On stay
byte-identical (samples() == is_on() at both live gates; the On reconcile arm is
byte-verbatim inside the mode match).
Bridge (interactive_loop_bridge): on a determinate lethal single-winner drain,
mandatory loops auto-win (identical to the On path); optional loops OFFER
WaitingFor::LoopShortcut to the determinate winner (CR 732.2a), then an APNAP
WaitingFor::RespondToShortcut accept-or-shorten window over all living opponents
(CR 732.2b/c) reusing the OpponentMayChoice remaining_players drain-one-advance
fan-out. GameAction::DeclareShortcut{count,template} + RespondToShortcut{response}
drive it. CR 732.4 all-mandatory net-progress no-loss loops end in a draw
(CR 104.4b); any loss axis or optional loop falls through to the pre-feature halt.
Multiplayer (>=3p APNAP) from the start.
Winner authority: the crown is the sole non-faller from live_mandatory_loop_winner
(nonfallers.len() == 1 requires every OTHER living player to fall, CR 104.2a), not
the mechanical loop controller. Subset-lethal pods (an opponent survives) return
None and never crown; a >=2-faller simultaneity floor requires equal per-cycle
life deltas so all fallers cross lethal in one CR 704.3 SBA batch.
Soundness (CR 608.2b): the offer latches the determinate winner; the dispatch
firewall admits only DeclareShortcut/RespondToShortcut between offer and accept
(a live ring re-scan is unsound -- intervening finalize/SBA/layer steps drift the
paused state). Concede and Debug bypass that firewall, so apply_confirmed_shortcut
re-validates the latched controller's liveness at consumption: CR 104.3a (a player
who conceded has lost and cannot be crowned), CR 104.2a (the winner must still be
in the game), CR 800.4a (the departed proposer's loop objects have left, so the
loop dissolved). On a departed proposer it hands priority to the next LIVING player
(is_alive(active_player) ? active_player : next_player_in_turn_order) rather than
crowning a departed player or leaving priority on a departed holder; the APNAP
remaining_players advance likewise filters out opponents who left mid-window.
New serialized surface (LoopDetectionMode::Interactive, WaitingFor::LoopShortcut/
RespondToShortcut, GameAction::DeclareShortcut/RespondToShortcut,
IterationCount::UntilLethal, ShortcutProposal/ShortcutResponse, LoopCertificate
serde derives) is additive; Off/On configs serialize byte-identically. Frontend
modal UI, smart-Shorten AI, and Fixed-count materialization are Phase 4/5 (the
WaitingFor types are registered so UnhandledWaitingForModal does not fire; the AI
answers RespondToShortcut with Accept and DeclareShortcut with UntilLethal via
explicit non-wildcard arms).
Verification: cargo fmt; clippy --all-targets -D warnings (clean); cargo test -p
engine (18616 passed, 0 failed); loop_shortcut 10/10 -- On+Off byte-identity
goldens, 3p APNAP cascade accept-win, CR 732.4 draw, Shorten priority window,
authorization routing, and revert-failing guards: controller-concede-not-crowned,
queued-opponent-concede-no-deadlock, and 3p-subset-lethal-no-crown. FORGE
ordering_parity_sweep not run -- Phase 3 touches zero trigger-ordering surface;
the three decision-input recipe files (ability_rw.rs, ability_utils.rs,
triggers_ordering_parity_tests.rs) are byte-identical to the branch base and no
parser/card-data is touched (delta=0 by construction). Independent review-impl
clean after catching + fixing one soundness blocker (the Concede/Debug firewall
bypass and its dead-priority remedy path).
Assisted-by: ClaudeCode:claude-opus-4.8
…hase 4a-core) Phase 4a-core of combo-detector PR-7: the OFFLINE object-growth loop detector — the object-axis analog of the existing stack-axis omega-coverability. Certifies (or fail-closed rejects) loops whose battlefield GROWS by inert tokens each iteration, which the strict board-equality gate + MAX_OBJECT_GROWTH ceiling could not detect. Offline only: no reducer/sampler/bridge change, so Off/On/Interactive runtime stays byte-identical to HEAD; the live empty-stack sampler (4a-live) is deferred until a certifiable live object-growth loop exists (plan ruling). Detector (analysis/resource.rs) loop_states_cover_modulo_object_growth reuses the Karp-Miller discipline on the object axis: - board_covers: absolute-ObjectId embed + inert-class confine; grown_ids = the battlefield after-before absolute-id set. - eq_except_growable: strip grown objects + clear battlefield/stack, then reuse the GameState PartialEq wholesale so EVERY non-object field strict-compares (incl. delayed_triggers / deferred_triggers / pending_trigger* / epic_effects accumulators) — the excepted set is derived, not hand-listed. - grown_objects_are_inert: no static/trigger/replacement/activated ability, non-CDA P/T, non-legendary/world, no counters (CR 704.5). Wired into the OFFLINE detect_loop classifier; the fail-loud residual_board_delta seam lights up on object growth. Fail-closed firewalls (CR 732.2a predictable-results; false-negative = grind-to-cap = today's behavior, false-positive is not acceptable): - Observation (fire_time_conditions_read_growing_class): scans the CONDITION and full EFFECT-BODY AST of every trigger/activated/static/TCE/granted-keyword ability on every battlefield object + the delayed/deferred/pending/epic store bodies, on the projected||sibling axis via the recursive scan_effect (opaque => CONSERVATIVE). - Cost (cost_surface_references_growing_class): ONE predicate over the whole cost surface — an EXHAUSTIVE no-`_` match over all 117 StaticMode variants (ModifyCost / ReduceAbilityCost dynamic_count for Reduce AND Raise; the AbilityCost-bearing Impose/Alternative/CastWith variants + the three cast-permission variants; CastWithKeyword) and an EXHAUSTIVE no-`_` match over all 198 Keyword variants (Affinity/Convoke/Improvise/Delve/Emerge/Offering/Bargain/Assist/Crew/Saddle/ Station/Teamwork/Conspire/Waterbend/Harmonize/Craft/Casualty reject on grown-class overlap; Undaunted is opponent-count SAFE), plus nested sub_ability/else_ability cost recursion. A per-creature cost INCREASE (ModifyCost Raise + ObjectCount) is the false-positive-infinite direction and rejects. Both matches are compile-break tripwires — a future StaticMode/Keyword variant cannot silently fail open. Totality guards: compile-time _gameobject_partition_is_total (all 136 GameObject fields) + _gamestate_partition_is_total (all 259 GameState fields), no `..`, so a future field is a build break until classified. objects_content_eq is extended with the 14 mutable per-object accumulators the hand-list had drifted behind (chosen_attributes / intensity / stickers / perpetual_mods / class_level / modal_back_face / goaded_by / detained_by / casting_permissions / saddled_by / ...), each classified by its write sites, not its doc-string. Stricter equality on the shared 2p CR 104.4b path is fail-safe (only suppresses a wrongful draw where an accumulator differed = not a true fixed-point); T-ON-golden stays byte-identical. Human rulings (plan section 14): keystone Witherbloom + Sprout Swarm => FAIL-CLOSED REJECT (cast-affordability preservation is unprovable from resolution deltas — affinity monotonicity is necessary but insufficient without a convoke-supply invariant the ResourceVector does not capture); object growth certifies nothing LIVE (4a-live deferred; keystone ships as the offline structural K-offline REJECT); B5 defuse tracks every enabler (phase 4c). Verification: cargo fmt; clippy --all-targets -D warnings (clean); cargo test -p engine (18642 passed, 0 failed); 20 offline object-growth predicate tests incl. K-offline keystone REJECT + R-e2 cost-increase-infinite + previously-fail-open cost keywords + one REJECT per observation/cost axis, each with a paired COVER control; T-ON-golden (Heliod+Ballista live 2p CR 104.4b) green — the fail-safe proof the objects_content_eq ADD did not over-suppress a legitimate draw loop; full loop suites green (loop_check/resource/loop_shortcut/corpus). Both firewall matches exhaustive-no-`_`. FORGE ordering-parity recipe files byte-identical (zero trigger-ordering/parser/card-data surface). Independent review: a 6-round soundness plan-review (S1-S6, each a fail-open-allowlist / equality-loosening class) + an implementation review that caught + structurally closed two cost-firewall gaps the green tests masked (the StaticMode type-misread and the cost-keyword fail-open allowlist). Assisted-by: ClaudeCode:claude-opus-4.8
…se 4b) Materialize a confirmed Fixed(N) loop shortcut (CR 732.2a) by driving N whole cycles of the constant-depth loop, committing atomically per cycle. Three-way per-beat drive-outcome split: cross-lethal (CR 704.5a) commits the GameOver already applied to `work` and stops; a recurred settle beat commits the cycle; any other prompt or engine error aborts to manual play (last complete cycle + priority to the living seat, CR 800.4a). Per-iteration `resolve` re-check enforces target legality (CR 608.2b) and object incarnation (CR 400.7) against the last committed board; stale/absent source aborts. Threads an Option<DecisionTemplate> onto ShortcutProposal (serde default, skip-if-none, so On/Off serialized streams are unchanged). The Fixed arm is reachable only under LoopDetectionMode::Interactive; Off/On paths stay byte-identical (candidates.rs still emits only UntilLethal). Per-beat fresh event buffers avoid re-scanning prior beats' events, matching run_auto_pass_loop. Assisted-by: ClaudeCode:claude-opus-4.8
…serving shortcut
CR 104.4b: an OPTIONAL beneficial (non-winning) loop is neither crowned (Path A: no faller) nor drawn (Path B: !mandatory) — mark it as a revocable-∞ capability (Path C) with its battlefield enabler set, so an enabler's departure (zones.rs apply_zone_exit_cleanup) REVOKES it. LOW-2: the AI's RespondToShortcut self-preserves via the shared smart_shortcut_response authority.
Documents has_no_loss_axis's two-gate asymmetry (measured): REDUNDANT at Path C (poison caught by ==Advantage/PoisonLoss, life by is_net_progress, library by recurrence — discriminator unsatisfiable, waived) but LOAD-BEARING at Path B (sole loss-axis veto, no ==Advantage backstop — kept; a poison loop reaching the gate would be wrongly drawn without it). The Path-B runtime discriminator is waived as measured-unsatisfiable: no constructible fixture carries poison>0 to the gate — the 2-trigger form clears the loop-detect ring on OrderTriggers beats, and the single-compound-trigger form drops the poison conjunct via a parser gap. Ships the Path-B draw-gate behavioral test (control draws / variant poisons out).
Follow-up (LOW, 0 live cards, synthetic-only): silent-clause-swallow parser gap — a bare-"and" cross-subject second conjunct ("...and each opponent gets a poison counter") is dropped at parse (kept only in description, not Unimplemented); fixing it unblocks a genuine Path-B has_no_loss_axis runtime discriminator.
Assisted-by: ClaudeCode:claude-opus-4.8
…tate fields for the loop cover gate Rebase of PR-7 phases 1–4c onto upstream/main d1a1e99 (phase-rs#5546) surfaced two new GameState fields that the object-growth cover gate must account for. The `_gamestate_partition_is_total` totality guard (exhaustive no-`..` destructure) forces an explicit classification of every field; ONE-SIDED-SAFETY governs it (COMPARED is fail-safe, EXCLUSION is the fail-dangerous direction — exclude a field only when comparing it would break legitimate loop detection): - pending_player_scope_sacrifice_choice: COMPARED (already in upstream's `impl PartialEq`) — a differing paused-sacrifice state is correctly not a fixed-point repeat. - post_replacement_token_substitution_count (CR 614.1a copy-token count): COMPARED via one contained conjunct in `eq_except_growable` — upstream's PartialEq deliberately excludes it, but excluding a count from the cover gate is the fail-dangerous direction. It is None at every sample beat (cleared whenever waiting_for == Priority) or a constant direct-assigned count across a real copy-token loop, so comparing never suppresses a legitimate loop. Upstream's PartialEq is left untouched. - resolution_source_relatch (CR 400.7j self-move re-latch): EXCLUDED-required (measured by ordering trace, not doc-trust). The clear at stack.rs fires at the START of the next resolution; record_loop_detect_sample fires at the Priority window AFTER this resolution's self-move set it, so at the sample beat it holds this iteration's current_incarnation, which bumps every iteration. Comparing it would make every self-moving loop compare unequal (a false negative). Object growth lives in `objects` (stripped and compared by eq_except_growable), so excluding this single-object identity field cannot hide growth. Gate: fmt, clippy --workspace --all-targets -D warnings, test -p engine -p phase-ai (0 failed / 22 binaries), FORGE byte-id (ability_utils/ability_rw byte-identical) all green; 0-behind upstream/main. Assisted-by: ClaudeCode:claude-opus-4.8
…LOCKER-2 structural sign-check Offline soundness core for object/token-growth loop detection (Witherbloom, the Balancer + Sprout Swarm class). NO live caller — the fodder predicate is exercised only by unit tests + the T-B1i discriminator; 4d-ii wires the live clone-drive hook + materializer. LoopDetectionMode stays OFF ⇒ byte-identical to pre-PR-7. New (analysis/resource.rs): - loop_states_cover_modulo_fodder_growth (pub(crate)) + board_covers_modulo_fodder + fodder_content_eq + is_fodder — tapped-split multiset cover for inert fodder growth (untapped(cur) >= untapped(prior) + strict total growth); stable-engine content via objects_content_eq (sole authority — GameState PartialEq is objects.len()-only). Drops the abstract cost-surface firewall (driven-path-only); detect_loop STAYS on loop_states_cover_modulo_object_growth (pinned by T-B1i). - driving_resources_non_decreasing + projected_player_axes + project_out_player_consumables refactor (no-`..` compiler-total destructure shared with project_out_resources) + _projected_player_axes_is_total — BLOCKER-2 structural sign-check: blanket fail-closed veto on any controller-side decrease of a projected consumable (energy/poison/per-kind player_counters + per-kind monotone object counters), closing the project_out_resources sign-unchecked hole. CR 106.1/119/122.1/122.1a/ 306.5c/310.4c/606.3/613.4c (all grep-verified). Tests: 15 inline (fodder cover + siblings; the 8-fixture sign-check family incl the structural per-kind player_counter discriminator; _projected_player_axes_is_total) + T-B1i (detect_loop returns None on a convoke-fodder pair; asserts both None AND fodder-cover==true). Reject-preservation regressions object_growth_k_offline_* / object_growth_r_a2_* stay green. Gate (direct cargo, worktree): fmt clean; check; clippy --all-targets -D warnings 0 warn; test -p engine 16175 passed / 0 failed. Plan and impl each reviewed clean by independent agents. 4d-ii carries (flagged, unreachable in 4d-i — no live caller): (1) tie the map-consumable sign-veto to the projected destructure (a projected_player_maps helper from the same no-`..` site) before wiring the live caller, so a future 2nd map consumable cannot be projected-out-and-sign-unchecked (BLOCKER-2 one field over); (2) veto controller-side damage_marked INCREASE (CR 704.5g) once object_resource_axes_match is dropped on the driven path. Assisted-by: ClaudeCode:claude-opus-4.8
…on + CR 732.2a offer Detect infinite token-growth loops live end-to-end and offer the CR 732.2a shortcut. Acceptance bar (the 51st): Witherbloom, the Balancer + Sprout Swarm parses, casts (convoke + affinity + buyback), detects, OFFERS, and materializes N real untapped Saprolings on Accept. Foundation (injector-independent): - RecastContext + GameState.last_recast_context. EXCLUDED from impl PartialEq so the live CR 104.4b 2p-draw comparison is byte-identical to pre-PR-7; compared only via explicit cover conjuncts (eq_except_growable + loop_states_equal_modulo_resources), keeping the N7 discriminator non-vacuous. - projected_player_maps: no-`..` structural tie for map-typed player consumables (a future map consumable build-breaks); damage_marked-INCREASE veto (CR 704.5g). - triggers.rs trigger-order filter_map made exhaustive (future PinnedDecision variant build-breaks). Live-drive: - PinnedDecision::ConvokeTaps (CR 601.2h + CR 702.51a/b) with select_convoke_taps as the single convoke cost-selection authority (the ConvokeTaps replay routes through it; shares is_convoke_eligible/object_cant_tap; no parallel path). - drive_recast_iteration injector: exhaustive on WaitingFor, fail-closed (Err(RecastAbort)) on every unpinned prompt; the ManaPayment decision loop is an exhaustive non-`_` match so a future ConcreteDecision variant build-breaks. - try_offer_object_growth_shortcut hook: read-only &GameState (live write type-impossible); OFFERS via WaitingFor::LoopShortcut, never auto-resolves (CR 732.2a); SimulationProbeGuard spans both drives (no hook recursion). - Materializer third disjunct (loop_states_cover_modulo_fodder_growth) + Fixed(N) object-growth cycle; leaves a valid state (right controller, ring cleared, last_recast_context cleared, priority to next living seat CR 800.4a). - RecastContext capture gated on loop_detection.samples() — phase-rs#4603 opt-in gate: in default LoopDetectionMode::Off nothing is written, so the serialized surface is byte-identical to pre-PR-7. Tests (real Witherbloom + Sprout Swarm, no synthetic on the positive): - P1 offers live (LoopShortcut, TokensCreated cert, exactly one real Saproling from the single real cast); P2 materializes N on Accept. - N1/N3 reject (no affinity / no buyback); N6 live no-offer control (damage-drain recast, CR 704.5g branch d) with a positive reach-guard, revert-probed. - off-mode phase-rs#4603 gate (OFF is_none + ON reach-guard, revert-probed); select_convoke_taps x4 + convoke_pin. - The 51st loop body has NO auto-resolved randomness (vanilla Saproling, no ETB, deterministic convoke) — runtime-confirmed by P1. N4 (energy) / N5 (player-counter) no-offer controls are covered at the unit + structural-wiring level, not live fixtures: a live per-recast drain on these axes is genuinely infeasible in today's buyback-recast mechanism (an energy cost breaks buyback recurrence — the naive live test was proven vacuous by revert-probe and removed rather than shipped; no engine effect drains Experience/Ticket counters). The shared driving_resources_non_decreasing seam (branches a/b/d) is live-verified via N6. The branch-(a)/(b) sign-checks are fail-closed defensive guards, live-unreachable today but not dead code. The offer path is fail-closed-modulo-auto-randomness until the A2 determinism gate (separate follow-up pass). Assisted-by: ClaudeCode:claude-opus-4.8
…ness-bearing recast loops (CR 732.2a)
A CR 732.2a shortcut "can't include conditional actions, where the outcome of
a game event determines the next action" — so an object-growth loop whose
recast cycle draws game randomness must NOT be offered as a fixed-count
shortcut. This wires a two-layer fail-closed determinism gate into the live
offer path (`try_offer_object_growth_shortcut`), discharging the b132ad9f8
"fail-closed-modulo-auto-randomness" carry.
(a) Static, compile-time-exhaustive scan of the recast spell body before
driving: `effect_is_randomness_bearing` (a wildcard-free match over `Effect`
— a future random-bearing variant BUILD-BREAKS rather than being silently
offered) + `spell_ability_bears_randomness`; the `collect_effects` walker
gains the two nested `replacement_effect` arms it was missing. Covers
auto-resolved coin (CR 705.1) / die (CR 706.1a) and the field-level
"game selects at random" selections (CR 701.9b) via `is_random()`.
(b) Post-drive RNG stream-position backstop: the driven clone starts as an
equal `state.clone()` (ChaCha20 derives `Clone`, preserving word position),
so `s_n2.rng.get_word_pos() != state.rng.get_word_pos()` iff any randomness
was consumed during the deterministic detection drive — from ANY source,
including external triggered/replacement randomness the static scan can't
see. Strictly-more-conservative (only turns OFFERs into NO-OFFERs).
Soundness (measured): external coin/die/`Choose` are already rejected by the
fodder cover (`scan_effect => Axes::CONSERVATIVE` reads the growing sibling
axis); the recast spell body is NOT scanned by the cover, so (a) is the gate
there. (b) is the universal runtime backstop. The custom-classified random
card-selections (`Discard`/`RevealHand`/`ChooseFromZone`) can classify
`sibling=false` and pass the cover, but each draws RNG inline inseparably from
an RNG-outcome-dependent object move/mark — breaking byte-identical loop
reproduction — so no reachable input makes (b) the SOLE gate today; (b) is a
complete future-proof backstop rather than a currently-isolable path. Verified
by an independent /review-impl pass (CLEAN-FOR-COMMIT).
Tests: `object_growth_random_recast_body_does_not_offer` (recast-body coin →
no offer; coin-free twin shell still OFFERs, proving the input reaches the
offer path and isolating the coin as the sole disqualifier) + leaf tests
discriminating `is_random()` on both selection-mode types. The paired positive
`object_growth_51st_sprout_swarm_covers_and_offers` is unchanged.
Assisted-by: ClaudeCode:claude-opus-4.8
…-trigger OrderTriggers beat (CR 603.3b/732.2a)
A self-refilling mandatory loop whose cycle emits 2+ simultaneous distinguishable
triggers parks at `WaitingFor::OrderTriggers` each iteration (CR 603.3b). The
loop-detection ring was WIPED on that beat by two clears, so it never accrued the
>=2 samples CR 732.2a detection needs — multi-trigger loops were undetectable
while single-trigger loops (which settle at Priority each cycle) worked. Ordering
simultaneous triggers is a forced step of putting them on the stack, not a
deliberate cascade-break, so the ring must survive it.
Two match-arm edits in game/engine.rs (no new variant/field):
- SEAM1 (pass_priority_once_with_pipeline): guard the ring-clear `else` with
`!matches!(wf, WaitingFor::OrderTriggers { .. })` — settling into the mandatory
ordering window leaves the ring intact (the record path stays gated on
Priority{active}; the triggers are staged off-stack in pending_trigger_order,
so the stack is momentarily shrunk here). Every other settle (drain-to-empty,
shrinking stack, interactive non-Priority window) still clears.
- SEAM2 (apply_action): exempt `GameAction::OrderTriggers` from the deliberate-
action clear (`!matches!(action, PassPriority | OrderTriggers { .. })`) — the
ordering response continues a mandatory cascade. Every other non-Pass action
(cast/activate/play-land) still invalidates the ring. SetTriggerOrderTemplate
early-returns before this clear, so it is unaffected.
Both edits are required: the ring is wiped at two moments of one cycle — SEAM1
when the resolution settles into the window, SEAM2 when the window is answered.
Test (mechanism-scoped): drive_multi_trigger_ring_survives_order_triggers_beat
asserts an OrderTriggers beat occurs (reach-guard) + max_ring>=2 (ring survival).
Revert-probe: reverting EITHER seam alone drops max_ring 2->1 (and the measured
end-to-end GameOver Some(P0) -> None), proving both seams load-bearing. Adds a
drive_with_trigger_ordering helper, an idx18 single-trigger no-order-beat
non-regression, and a no-refiller false-positive control. The fixture also
reaches end-to-end GameOver at beat 10 (measured, documented as a NOTE not
asserted — max_ring==2 sits at the 2-sample detection edge; the robust E2E
multi-trigger witness is the 52nd/G1).
Assisted-by: ClaudeCode:claude-opus-4.8
…-authority census (PR-7 CI) The CI-only engine-authority ratchet (scripts/check-engine-authorities.sh -> zone_authority_census.py; not run by clippy/test, so the local gate was green) flagged two NEW raw zone-container mutations in PR-7 code: - analysis/resource.rs::eq_except_growable — clears battlefield on a DISCARDED comparison CLONE for loop-cover equality. Takes &GameState and mutates a local clone consumed by ==; no gameplay zone event can fire on it. - game/engine.rs::normalize_recast_frame — prunes hand/graveyard/library on a DISCARDED recast comparison-frame CLONE. Takes &GameState and returns a normalized clone; no gameplay zone event fires on it. Both are genuinely non-replaceable analysis-time normalizations (not live gameplay zone changes routed through zone_pipeline), so each site is annotated // allow-raw-zone: <reason> and the frozen baseline (scripts/zone-authority-baseline.txt) is regenerated via --write (2 exempt rows added; no baseline row dropped). Gate B PASS. Comment-only code change; no behavior change. Assisted-by: ClaudeCode:claude-opus-4.8
…2a), R2 typed enums, FE dead code
- Classify Effect::Clash as randomness-bearing: CR 701.30a reveals the top of a shuffled
library (hidden info at pin time) and CR 701.30d decides the winner by revealed mana value,
so a loop whose recast body contains a clash is not shortcut-eligible under CR 732.2a. Moved
the false->true arm in effect_is_randomness_bearing + added a discriminating assertion to
randomness_classifier_discriminates (revert-probe: moving it back flips the assertion).
- R2 (no bool fields): PinnedDecision/ConcreteDecision take/pay bool -> MayChoiceOption{Take,
Decline} / UnlessPaymentOption{Pay,Decline}; RecastContext.uses_buyback bool -> BuybackUsage
{Used,NotUsed} with a pays() accessor for the DecideOptionalCost consumer.
- Remove the dead-code '?? []' guard in LoopShortcutModal (schema.points is non-optional).
Assisted-by: ClaudeCode:claude-opus-4.8
…d convoke count (phase-rs#5672 review) Addresses maintainer matthewevans' CHANGES_REQUESTED on phase-rs#5672 (un-holds follow-up phase-rs#24). BLOCKER (CR 732.2a): the loop winner was forced to propose a shortcut (only DeclareShortcut was accepted at WaitingFor::LoopShortcut). CR 732.2a makes proposing a shortcut a MAY. Add a unit GameAction::DeclineShortcut: the controller-only decline restores ordinary priority (living_priority_seat) and clears the object-growth routing context (last_recast_context) so the post-return reconcile does not re-offer. Seam-1 (interactive/loop_detect_ring) re-offer is already suppressed by apply_action's deliberate-action ring invalidation (a deliberate break clears the ring); the handler owns only the Seam-2 gap. A genuine re-recurrence re-arms the offer. Controller-only authorization is enforced upstream via check_actor_authorization. NON-BLOCKING: move the convoke tappable-count derivation out of React into the engine-owned ShortcutDecisionSchema (convoke_tappable_count, CR 702.51a); the modal renders it directly. Adds a FE Decline button (display-only) + comboShortcut.decline in all 7 locales. Two discriminating end-to-end decline tests (interactive dismissal + object-growth suppression, each with an independent revert-probe) + a wrong-actor auth-reject test. phase-rs#4603 OFF stays byte-identical (no new GameState field; the offer is never installed when OFF). Assisted-by: ClaudeCode:claude-opus-4.8
…d renames + drain/draw_sequence adds in the partition guard Rebase onto upstream/main 6f7cee8 crosses phase-rs#5686, which renamed six GameState fields to their legacy_ serde-migration names (post_replacement_{continuation,source,applied,event_source,event_target} -> legacy_*, pending_multi_draw -> legacy_pending_multi_draw) and added post_replacement_drains + draw_sequences. Update the exhaustive _gamestate_partition_is_total destructure (no '..', so a field-set mismatch is a hard E0027/E0559) to the new field names; the two new fields join the destructure so the cover-gate re-audit tripwire stays total. Soundness unchanged: draw_sequences is loop_equal-compared in GameState PartialEq; post_replacement_drains is skip_serializing_if=is_empty (settle-empty, loop-neutral). Assisted-by: ClaudeCode:claude-opus-4.8
lgray
force-pushed
the
feat/combo-detect-pr7
branch
from
f9e9ed7 to
0cd9a0f
Compare
Contributor
Author
|
🤖 AI text below 🤖 Thanks — building directly on your 1. 2. Rebase note: rebased onto current |
matthewevans
requested changes
Jul 12, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Request changes: the human decline path is implemented, but shortcut authority is still assigned to the wrong player and AI controllers remain forced to propose.
🔴 Blocker
-
crates/engine/src/game/engine.rs:490-523buildsWaitingFor::LoopShortcut { controller: winner, .. }, while the surrounding gate only establishesWaitingFor::Priority { .. }; it does not prove that the detected winner owns that priority.WaitingFor::acting_player()then authorizes that winner. CR 732.2a states that “the player with priority may suggest a shortcut.” In a loop where the nonactive winner differs from the priority holder, the engine offers the proposal to the wrong player. Carry the priority holder as the proposer separately from the predicted shortcut outcome/winner, and add a runtime case where those seats differ. -
crates/engine/src/ai_support/candidates.rs:3006-3018,crates/phase-ai/src/search.rs:758-764, andcrates/phase-ai/src/projection.rs:418-424hard-code onlyDeclareShortcut. AlthoughGameAction::DeclineShortcutis now valid in the reducer, an AI controlling the offer has no legal/policy route to decline the optional CR 732.2a proposal. Add the decline candidate and route AI selection through the same action set, with a regression proving it can select decline. -
The required
<!-- coverage-parse-diff -->evidence is still baseline-pending, and Rust lint/tests, card-data, and AI checks are still in progress for this head. Refresh the evidence and wait for terminal current-head checks before approval.
🟡 Non-blocking
- None.
✅ Clean
crates/engine/src/types/actions.rs:813-817,crates/engine/src/game/engine.rs:4295-4300, andcrates/engine/tests/integration/loop_shortcut.rs:566-715,2549-2628correctly add, authorize, and runtime-test the human decline transition and immediate re-offer suppression.client/src/components/modal/LoopShortcutModal.tsx:75-85now renders the engine-providedschema.convoke_tappable_countdirectly rather than deriving it from decision points.
Recommendation: request changes—separate proposer priority from predicted winner, thread decline through all AI action paths, then regenerate the parse diff and re-review on terminal-green CI.
# Conflicts: # crates/engine/tests/fixtures/integration_cards.json
Port the generated integration fixture by preserving current main and The One Ring record. Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
matthewevans
approved these changes
Jul 12, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Approved after current-head CI and AI gates passed, the parse-diff sticky refreshed with no card-parse changes, and the final authority/AI-decline review found no remaining behavioral issues.
matthewevans
added a commit
to jaytbarimbao-collab/phase
that referenced
this pull request
Jul 13, 2026
… parity rows it exposes (CR 603.3b + CR 603.4) (phase-rs#5732) `integration_cards.json` is a cached subset of the card-data export, but it was last FULLY regenerated at b9685cc (phase-rs#5695). Nineteen parser/types PRs merged since then; every fixture touch in between was surgical (phase-rs#5672 +1, phase-rs#5679 +2, phase-rs#5727 2 entries), so the parse values silently drifted. Regenerated from the export at 8c35dc5 (oracle-gen, MTGJSON 5.3.0+20260629). Population (json deep-equality, not line counts — the file is one line): committed 2658 entries -> 2726. 70 added, 2 removed, 44 changed values. Attribution of the 44 changed (causal: exports built at phase-rs#5720 / phase-rs#5717 / phase-rs#5730 and compared, NOT shape-guessing): phase-rs#5723 (P02-U3b shared condition grammar) ...... 3 archive trap, temple of civilization, thaumaton torpedo (all gained the comparator/lhs/rhs/qty/scope condition shape) phase-rs#5721 + phase-rs#5719 (where-X quantity channel + ..... 21 restriction grammar; both merged BEFORE phase-rs#5717 — merge order != PR-number order) bellowsbreath ogre, cryptex, deadly rollick, deflecting swat, desert, dread wanderer, esquire of the king, flesh, fraying sanity, gloomlake verge, great desert hellion, gutterbones, officious interrogation, once upon a time, potioner's trove, ribald shanty, rock jockey, second little pig, shifting woodland, snuff out, starport security phase-rs#5695..phase-rs#5720 no-regen window (bloc) ........... 20 Stale already at phase-rs#5720, so attributable to the 19-PR window above the b9685cc anchor, not to any single PR: alrund god of the cosmos, animal friend, approach of the second sun, cavernous maw, fblthp the lost, from father to son, hour of revelation, increasing vengeance, jodah the unifier, mana reflection, misty salon, puca's eye, ram through, reidane god of the worthy, secrets of the key, sevinne's reclamation, temple of the dead, the dining car, unleash the flux, valgavoth terror eater The 70 added keys are new test-source card references the generator collects; phase-rs#5729 (tests-only) contributed zero parse delta, as expected. Corrected premise: 44 entries are truly stale, not 7. Six of the seven originally reported reproduce; `osteomancer adept` is NOT stale (committed == fresh). The regen turns `ordering_parity_sweep` red, so the gate's evidence rows ship ATOMICALLY with it. Both rows are population entries, not ordering regressions: the sweep skips Unimplemented-bearing triggers, so a card only enters it once its parse binds. great desert hellion -> BATCH_GENUINE_ROWS. Its LTB Draw was Unimplemented until phase-rs#5721/phase-rs#5719 bound Intensity{Source}. Each co-departing Hellion draws off its OWN intensity but discards the SHARED hand, so the second trigger discards the cards the first just drew: with intensities a != b the final hand, graveyard and library differ by order. The members are not identical functions, so commutation genuinely fails and the new prompt is the CR 603.3b choice the legacy serde walk wrongly auto-ordered (CR 603.5: each "may" is chosen on resolution). planar collapse -> DOCUMENTED_OVER_PROMPT (L8-held family). New fixture key. Upkeep ObjectCount(Creature) >= 4 intervening-if x DestroyAll + self-Sacrifice: the first copy's sweep drives the census to 0, so the sibling's CR 603.4 re-check is false and it does nothing. Monotone and self-limiting — identical siblings commute up to relabeling, so the prompt is conservative, fail-closed and rules-correct. Neither row weakens the gate: both are direction-gated over-prompts (an under-prompt is never suppressible), and both are consumed by the ledger's exact-set asserts (over_prompt_hit 18->19, batch_genuine_hit 1->2), so a misclassification still trips the STRICT PROOF-GATE. Verification: engine lib 16481/16481 pass (was 16480 + 1 red); integration 2929/2929 pass. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
4 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 13, 2026
…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
4 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 13, 2026
…iveness conjunct (phase-rs#5672 follow-ups) (phase-rs#5748) * feat(ai): score the CR 732.2a loop-shortcut offer so the AI declines 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 * test(engine): cover the winner-liveness conjunct at the shortcut-consumption 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
Merged
4 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Jul 13, 2026
…-rs#5672) DeclareShortcut carried an unvalidated IterationCount::Fixed(u32): the server payload guard discarded count with .. and handle_declare_shortcut moved it into the proposal unchecked, so materialize_fixed_shortcut drove for i in 0..n — one GameState clone per cycle, up to ~4.3e9. A u32 encodes that in ~10 JSON bytes, sailing through the 8 KB WS frame cap. Engine-authoritative fix: a single MAX_SHORTCUT_CYCLES cap rejects an over-cap Fixed count into the existing fail-closed handback before the sole ShortcutProposal build site (covers WS, WASM, Tauri, local); the same const clamps shortcut_drive_period, bounding the templated UntilLethal drive on every caller. Wire defense-in-depth: bound the Fixed count and the nested template vecs at the payload guard; the false "count is a small enum" comment is corrected. IterationCount is now matched exhaustively at both bound sites so a future count variant build-breaks rather than silently regressing the cap. CR 732.2a (safety limit, no rules ceiling) / CR 800.4a (handback to living seat). Assisted-by: ClaudeCode:claude-opus-4.8
matthewevans
pushed a commit
that referenced
this pull request
Jul 13, 2026
#5756) DeclareShortcut carried an unvalidated IterationCount::Fixed(u32): the server payload guard discarded count with .. and handle_declare_shortcut moved it into the proposal unchecked, so materialize_fixed_shortcut drove for i in 0..n — one GameState clone per cycle, up to ~4.3e9. A u32 encodes that in ~10 JSON bytes, sailing through the 8 KB WS frame cap. Engine-authoritative fix: a single MAX_SHORTCUT_CYCLES cap rejects an over-cap Fixed count into the existing fail-closed handback before the sole ShortcutProposal build site (covers WS, WASM, Tauri, local); the same const clamps shortcut_drive_period, bounding the templated UntilLethal drive on every caller. Wire defense-in-depth: bound the Fixed count and the nested template vecs at the payload guard; the false "count is a small enum" comment is corrected. IterationCount is now matched exhaustively at both bound sites so a future count variant build-breaks rather than silently regressing the cap. CR 732.2a (safety limit, no rules ceiling) / CR 800.4a (handback to living seat). Assisted-by: ClaudeCode:claude-opus-4.8
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.
🤖 AI text below 🤖
Summary
PR-7 of the combo-detector series: the interactive loop-shortcut offer + opponent accept/shorten response window (CR 732.2a–c) with a full combo-declaration UI. When the engine detects a determinate mandatory (or offerable) loop, the loop's winner is offered a
WaitingFor::LoopShortcut— declare the shortcut instead of manually iterating — and every living opponent then gets an APNAPRespondToShortcutaccept-or-shorten window. Adds theDecisionTemplateprimitive (pinned choices for conditionally-infinite combos; CR 608.2b per-iteration re-check + CR 732.2a predictability gate), object-growth loop detection (unbounded token/counter growth via coverability modulo growth), poison/lethal win authorities, and the React display-layer modals. Ships behind the #4603 default-OFFLoopDetectionModetoggle.Implementation method (required)
Method: /engine-implementer
CR references
CR 732.2a (propose shortcut) · CR 732.2b (each other player accepts/shortens, APNAP) · CR 732.2c (shortcut taken) · CR 732.4 (net-progress draw) · CR 732.6 ("unless") · CR 704.5a (0-or-less life loses) · CR 704.5c (10 poison loses) · CR 104.2a (sole survivor wins) · CR 800.4a (priority to next living seat) · CR 608.2b (per-iteration target re-check) · CR 603.3b (simultaneous-trigger ordering) · CR 702.51a (convoke) · CR 122.1 (counters). Full annotations in the diff.
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.
cargo check --workspace --all-targets— PASScargo clippy --workspace --all-targets -- -D warnings— PASScargo test -p engine --lib— 16332 passed / 0 failed / 6 ignoredcargo test -p engine --test integration— 2774 passed / 0 failed / 2 ignoredpnpm --dir client run type-check(protocol:check && tsc -b --noEmit) — PASSpnpm --dir client exec vitest run(full FE suite, inclboundary-guardrails.test.tsengine↔FE lockstep) — 231 files / 1963 passed / 0 failedfeat(engine): ∞ unbounded-resource display for confirmed infinite loops (combo PR-6) #4603 default-OFF:
LoopDetectionMode::Off⇒samples()==false⇒ neither offer fires ⇒ Off behavior byte-identical to pre-feature mainRebased onto current
upstream/main(fa7f157fa, includes ship/p02 u2 per unit swallow audit #5668 swallow-audit): 0-behind / 27-ahead; range-diff 27=(0!, 0>) — fully patch-equivalent; MAIN-clean. ship/p02 u2 per unit swallow audit #5668 is parser/swallow-only (no shared enum/Effect/StaticMode/AbilityCost surface) —cargo check --workspace --all-targetsgreen confirms no exhaustiveness drift.Gate A
Gate A PASS head=9e5cc223e14f2186c7f5174c177e2c07d0e763a3 base=fa7f157fae0fe8bddd774f2203b9170de385465f
Anchored on
live_mandatory_loop_winner, the win authority the UI crown reuses VERBATIM (drive-and-measure over per-cycleResourceVectordeltas)WaitingFor-modal pattern the combo-declaration modals mirror (useGameStore + local selection state + self-gate + dispatch)Final review-impl
Final review-impl PASS head=9e5cc223e14f2186c7f5174c177e2c07d0e763a3 (carries from f883450 via the patch-equivalent #5668 rebase — range-diff all-27-
=)Claimed parse impact
Review dispositions (#5672 Gemini comment pass — head f883450)
All 5 bot comments addressed (ACCEPT + fixed), independently review-impl'd clean at the current head and CI-confirmed:
ability_scan.rs—Effect::Clashreclassified randomness-bearing (moved to thetruearm ofeffect_is_randomness_bearing; the shortcut gate fail-closes inengine.rs, so a clash-bodied recast loop is never offered). CR corrected: the bot's701.23ais Search; the real cites are CR 701.30a (reveal top of library) + CR 701.30d (winner by revealed mana value) + CR 732.2a (a shortcut can't include conditional actions whose outcome determines the next action). Discriminating revert-probe assertion added.LoopShortcutModal.tsx— removed the dead?? []guard (pointsis a non-optionalDecisionPoint[]).decision_template.rs—PinnedDecision/ConcreteDecisiontake/paybool→ typedMayChoiceOption{Take,Decline}/UnlessPaymentOption{Pay,Decline}(project rule: no bool fields).game_state.rs—RecastContext.uses_buybackbool→BuybackUsage{Used,NotUsed}+pays()accessor. Declined the bot's speculativeOptionalCostKindgeneralization as YAGNI (buyback is the only optional cost tracked today).server-corepayload-guard test was migrated to the new enum (workspace-build correctness).Combo-detector series
ResourceVector+ modulo-resource loop equalityResourceVectordetect_loop→LoopCertificate+ corpus harnesscargo combo-verifyCLI over the corpus∞unbounded-resource display (wholeResourceAxisclass; engine-ownedDerivedViews)DecisionTemplate+ object-growth detection + combo-declaration UILoopCertificate→ top line)Predecessor: PR-6.75 — #5072 — #5072 — delivered the C0-full + C1 read/write conflict-profile module (
ability_rw.rs) that madegroup_is_order_independentprovably sound (latent CR 603.3b fix). PR-7 builds the live loop-shortcut offer, the opponent accept/shorten window, and the combo-declaration UI on top of that detection foundation.Combo-declaration UI scope
The Declare + RespondToShortcut modals cover every reachable offer today (choice-free drains + object-growth convoke loops). Human pin-capture (specifying targets/taps for conditionally-infinite combos) is intentionally deferred to the gated >2p targeted-offer work (tracked follow-up) and will be assembled engine-side so the frontend stays a pure display layer. This is complete for what can occur today, not a cut — the pinning WRITE-side (
validate_pinsfirewall + drive-and-measure injector) already ships here, forward-covered by synthetic tests.Known-minor (tracked follow-up)
DeclareShortcut(no decline-and-continue path; onlyConcedebypasses the prompt). CR 732.2a makes proposing a shortcut a MAY, so this is a rules-completeness gap. Tracked as a standalone engine follow-up (add a decline action that returns to priority and suppresses immediate re-offer); not a blocker for this PR.Game-changing feature toggle (#4603)
This is a game-changing feature and ships behind the user-controllable
LoopDetectionModetoggle, default OFF. With OFF,samples()==false, neither the loop-shortcut nor the response-window offer is ever produced, and behavior is byte-identical to pre-featureupstream/main. Players opt in explicitly.