Skip to content

feat(engine): drive & collapse accepted CR 732.2a infinite-loop shortcuts at the phase boundary - #6238

Merged
matthewevans merged 25 commits into
phase-rs:mainfrom
lgray:feat/combo-detector-live
Jul 21, 2026
Merged

feat(engine): drive & collapse accepted CR 732.2a infinite-loop shortcuts at the phase boundary#6238
matthewevans merged 25 commits into
phase-rs:mainfrom
lgray:feat/combo-detector-live

Conversation

@lgray

@lgray lgray commented Jul 20, 2026

Copy link
Copy Markdown
Contributor

I've fully playtested this branch end to end and confirmed it correctly offers CR 732.2a loop shortcuts to the user in a live 4-player game: the Witherbloom / Sprout Swarm object-growth loop and the Kilo, Apogee Mind + Freed from the Real + Relic of Legends + Pentad Prism counter loop both surface the shortcut prompt and resolve to the correct board state (naming ~500 counters resolves quickly). The next tasks are to widen the scope of combos that we trigger on and accept.

🤖 AI text below 🤖

Summary

Delivers the acceptance + materialization half of the CR 732.2a loop-shortcut feature. When a player accepts an offered infinite loop at the phase boundary (the offer / opponent response window landed in PR-7 #5672), the engine now drives the loop to a player-named finite count and collapses the unbounded growth axis — copy-token piles, +1/+1 (and other) counters, life, and mana — into the correct terminal board state, rendering on the affected resource. Covers object-growth loops (tapped token piles) and persistent-axis loops (counters / life / mana) with APNAP accept + interruptibility, a zone-gated object-growth census firewall (CR 603.4 / 603.6a / 113.6), and multi-action loop-shortcut driving for infinite-mana combos.

Multi-commit epic (24 commits) developed on the fork; each component was built through /engine-implementer (plan → review-engine-plan → implement → review-impl).

Implementation method (required)

Method: /engine-implementer

CR references

Core: CR 732.2a / CR 732.2b / CR 732.6 (loop shortcuts + response window), CR 500.4 / CR 500.5 (phase-boundary auto-advance / SBA), CR 603.4 / CR 603.6a / CR 113.6 (zone-gated loop-firewall observer scans), CR 704.5a / CR 704.5f / CR 704.5j (mandatory-loop winner / state-based actions), CR 608.2bCR 608.2d (per-iteration re-check + predictability). Plus supporting annotations for counters (CR 122.1), life (CR 119), and keyword/interaction rules — ~89 unique CR annotations in the diff, each grepped against docs/MagicCompRules.txt.

Verification

  • Required checks ran clean locally, 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 (recent components; note below).
  • Both anchors cite existing analogous code at the same seam.

Rebased onto upstream/main (v0.31.0, f4287548f) — clean, no conflicts; branch is 0 behind / 24 ahead. Measured on the rebased head 90ad9da0d:

  • Live 4-player playtest (ground truth) — both combos offer the shortcut and resolve to the correct board state; ~500-counter collapse is fast. Confirmed by the maintainer (on the pre-rebase content-equivalent tip; the rebase pulled only upstream parser/card-data/CI changes, which don't touch the loop-shortcut path).
  • cargo clippy -p engine --all-targetsclean (0 warnings/errors) post-rebase — the semantic-drift check a clean 3-way merge can hide.
  • cargo test -p engine --test integration combo_infinite_pile19 passed; 0 failed post-rebase (both real-4p fixture tests ok, including the regenerated Witherbloom untapped-precast dump).
  • scripts/build-wasm.sh assert_wasm_stackPASS: engine_wasm_bg.wasm initial memory min = 365 pages (~22 MiB) — the 16 MiB WASM shadow-stack fix; guards against the accept-time loop-replay stack overflow that dropped the browser engine ("connection lost").
  • scripts/check-parser-combinators.sh — Gate A + Gate G PASS (below).
  • cargo fmt --all --check — clean.

Full clippy --all-targets --workspace / test / frontend / card-data-coverage / FORGE run on this PR.

Gate A

Gate A PASS head=90ad9da0da8974f468c233e1ab61e1db9388bd45 base=836ff312ae2073c99af28d286b0c4915faa8a458

Anchored on

  • crates/engine/src/game/engine_resolution_choices.rs — the SubmitPayAmount LoopCollapse materialization mirrors the pre-existing copy-token batch mint (drive_copy_token_batches) for the counter / life / mana axes.
  • crates/engine/src/analysis/resource.rs (+ DerivedViews) — the render extends PR-6's (feat(engine): ∞ unbounded-resource display for confirmed infinite loops (combo PR-6) #4603) engine-owned unbounded_resources projection over the ResourceAxis class.

Final review-impl

Final review-impl PASS head=90ad9da0da8974f468c233e1ab61e1db9388bd45

(The two most recent components — the WASM shadow-stack fix and the axis-labelled LoopCollapse prompt — ran /review-impl to clean this session; earlier epic components were reviewed through the same pipeline across prior sessions.)

Claimed parse impact

None — engine game-logic (loop-shortcut acceptance + boundary collapse), not parser card-coverage.

Combo-detector series

Pos PR Delivers
PR-0 #4092 ResourceVector + modulo-resource loop equality.
PR-1 #4097 Analysis sim harness feeding ResourceVector.
PR-2 #4119 Net-progress detect_loopLoopCertificate + corpus harness.
PR-3 #4480 Live mandatory-loop winner shortcut (drain-cascade, CR 704.5a).
PR-4a #4493 Engine B static ability-graph extractor.
PR-4b #4534 Engine B effect/trigger breadth + life-symmetry cost.
PR-5 #4547 cargo combo-verify CLI over the corpus.
PR-6 #4603 unbounded-resource display (engine-owned DerivedViews projection).
PR-6.75 #5072 C0-full + C1 read/write conflict-profile precision pass.
PR-7 #5672 Loop shortcut offer + APNAP accept-or-shorten response window (CR 732.2a–c).
this PR (this PR) Loop-shortcut acceptance + phase-boundary collapse/materialization — drive an accepted loop to a finite N and collapse the unbounded axis (tokens / counters / life / mana) to the correct terminal board, rendering .

Predecessor: PR-7 — #5672#5672 — delivered the CR 732.2a offer + opponent accept-or-shorten window; this PR executes an accepted offer by driving the loop to the player-named count and materializing the collapsed result at the CR 500.5 boundary.

@lgray
lgray requested a review from matthewevans as a code owner July 20, 2026 18:37

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Blocked — protected workflow changes require explicit maintainer review; no implementation diff was reviewed.

🔴 Blocker

.github/workflows/deploy.yml: this PR modifies a protected deployment-workflow path. Repository policy treats workflow edits as a hard stop, so this sweep does not evaluate, approve, or enqueue the implementation while that path is present.

Recommendation: remove the protected workflow change into an explicitly maintainer-owned review, then resubmit the implementation-only scope for normal review.

@gemini-code-assist gemini-code-assist Bot left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Code Review

This pull request implements comprehensive support for CR 732.2a loop-collapse shortcuts and display updates, including rendering the infinity symbol (∞) for unbounded piles and counters on the frontend, and updating the loop-collapse prompt to dynamically name the collapsed axis (Tokens, Counters, Life, or Mixed). It unifies recast and activation loops under a new LoopAction enum, optimizes liminal copy-token creation to prevent stack overflows, and introduces a load migration to clean up stale transient loop sequences. A critical bug was identified in the iterative token-creation logic where created_ids is overwritten rather than accumulated across iterations, which should be resolved using the suggested extend fix.

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.

Comment on lines +596 to 605
if !super::token::commit_liminal_copy_token_entry(state, event, events) {
created_ids = state.last_created_token_ids.clone();
return CopyTokenApplyStatus {
created_ids,
completion: CopyTokenApplyCompletion::Paused,
};
}
// CR 707.2: this copy committed; iterate to the next token in the
// batch (O(1) stack). Terminal drain runs once after the loop.
created_ids = state.last_created_token_ids.clone();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

critical

There appears to be a bug in the iterative logic for creating liminal tokens. The created_ids vector is being overwritten in each iteration of the loop instead of accumulating the IDs of all created tokens.

Specifically, created_ids = state.last_created_token_ids.clone(); replaces the list of created IDs with a new list containing only the most recently created token's ID. Since commit_liminal_copy_token_entry clears state.last_created_token_ids on each call, this function will incorrectly report that only one token was created for the entire batch.

The fix is to append the new token ID to the created_ids vector in each iteration, for instance by using extend.

Suggested change
if !super::token::commit_liminal_copy_token_entry(state, event, events) {
created_ids = state.last_created_token_ids.clone();
return CopyTokenApplyStatus {
created_ids,
completion: CopyTokenApplyCompletion::Paused,
};
}
// CR 707.2: this copy committed; iterate to the next token in the
// batch (O(1) stack). Terminal drain runs once after the loop.
created_ids = state.last_created_token_ids.clone();
if !super::token::commit_liminal_copy_token_entry(state, event, events) {
created_ids.extend(state.last_created_token_ids.iter().copied());
return CopyTokenApplyStatus {
created_ids,
completion: CopyTokenApplyCompletion::Paused,
};
}
// CR 707.2: this copy committed; iterate to the next token in the
// batch (O(1) stack). Terminal drain runs once after the loop.
created_ids.extend(state.last_created_token_ids.iter().copied());

@github-actions

Copy link
Copy Markdown

Parse changes introduced by this PR

✓ No card-parse changes detected.

lgray added 24 commits July 20, 2026 14:23
…ts (CR 732.2a)

CORE Unit-1 (P0+P2) of the loop/combo detector. Introduces
ScanMode {Conservative, LoopFirewall} in ability_scan so the CR 732.2a
loop-shortcut firewall reads token/mana ability bodies precisely
(LoopFirewall) while the CR 603.3b trigger-ordering gate keeps its
conservative answer (Conservative). Off is byte-identical (phase-rs#4603): all
existing pub(crate) entries pass Conservative, unchanged.

P0: ScanMode enum + minimal mode thread (6 fns on the path to the
Effect::Token/Mana leaves) + LoopFirewall _for_loop entries.
P2: Token/Mana descend under LoopFirewall; new exhaustive scanners
scan_continuous_modification (53), scan_keyword (198, reuses
keyword_cost_reads_growing_class), scan_mana_production (15, board
aggregates self-assert sibling), scan_pt_value. resource.rs firewall
descends per-modification on sibling||projected (M9, both axes).

14 discriminating tests (P0-1..4, P2-1..10), 13/14 revert-probe-proven;
independent review re-ran 3 probes + audited the full descent for
under-reads (none). clippy -D warnings clean; 16762 passed / 0 failed.

Assisted-by: ClaudeCode:claude-opus-4.8
…offers

Extend the loop-shortcut detector to capture repeated activated abilities alongside recasts, unifying both under a flat LoopAction {Recast, Activate} carried by LoopActionContext (renamed from RecastContext, with #[serde(from)] back-compat so pre-rename combo saves still load). This is the REACH milestone: activated-ability loops now capture, drive, and sustain; the offer path itself is exercised by an #[ignore]d canary pending the P3 firewall-precision change (Presence of Gond + Intruder Alarm).

- game/engine.rs: Activate capture arm + action-agnostic drive/normalize/offer hook
- types/game_state.rs: LoopAction + LoopActionContext + LoopActionContextRepr serde migration + discriminating round-trip test
- game/casting_costs.rs: action-agnostic loop-context setter
- analysis/resource.rs: object-growth cover conjuncts over the renamed context
- tests/integration/loop_shortcut_activation.rs: capture/sustain canary + sustain-failure negative (offer test #[ignore]d, P3-load-bearing)

Assisted-by: ClaudeCode:claude-opus-4.8
…ortcut detector (P3-B)

The loop-shortcut detector certifies a game loop as no-net-progress so it can be
shortcut (CR 732.2a). A mass-battlefield read — a resolver that enumerates the
battlefield and applies to every matching object, scaling with board growth — can
escalate each iteration, so relaxing its census read yields a false combo
certificate. Add the census firewall: two exhaustive no-wildcard classifier
oracles (effect_target_ctx → LiveBoardCensus, effect_census_role → Census) tag the
28 mass-population effects (incl. 8 dual-mode resolvers: PhaseOut/PhaseIn,
GainActivatedAbilitiesOfTarget, BecomeCopy, TurnFaceUp/Down, MultiplyCounter,
CopyTokenBlockingAttacker); guard#3 + census_partition pin the 28-set; a durable
read_dir forward-guard asserts the mass-scan-idiom file set == a curated 14-file
classification, each census file tied to the oracle. Canary coverage (Sprout
Swarm, Intruder Alarm, token-storm) confirms offer-when-sound / veto-when-growing.

CR 732.2a, 702.26, 611.2c, 707.2, 708.2/708.2a, 701.10, 508.1.

Assisted-by: ClaudeCode:claude-opus-4.8
…bilitiesFromSource in the CR 732.2a census firewall

Rebase adaptation onto upstream/main. Two new upstream Effect/ContinuousModification
variants broke the loop-shortcut census firewall's exhaustive no-wildcard matches
(the design catching drift at compile):

- Effect::ArrangePlanarDeckTop (phase-rs#6070, Susan Foreman) reorders the planar deck top
  (Planechase), not a battlefield population, so it is not a live-board census:
  classified relax (SnapshotOrEvent / CensusRole::Relax) in both effect_target_ctx
  and effect_census_role, keeping their Census sets byte-identical
  (census_partition invariant holds). Not a mass-scan-idiom resolver, so guard#3's
  census tag-set and the durable CLASSIFIED const are unchanged.
- ContinuousModification::RetainAllOtherAbilitiesFromSource (phase-rs#6009, Sakashima) is a
  copy-layer ability-retention mod with no inner walker, same class as
  RetainPrinted{Trigger,Ability}FromSource: Axes::CONSERVATIVE (fail-closed).

CR 732.2a.

Assisted-by: ClaudeCode:claude-opus-4.8
…bos (CR 732.2a)

Generalize the CR 732.2a loop-shortcut detector from single-action recast/
activate loops to multi-action sequences, so real two-activation mana engines
(Basalt Monolith + Power Artifact) are detected and OFFERED as an
Advantage-status shortcut.

- game_state: last_loop_action_context (Option) -> last_loop_action_sequence
  (Vec<LoopActionContext>); empty = unarmed. Adds LoopAction::is_voluntarily_
  repeatable (CR 601.2a/602.2/605.3a) and a serde shim migrating
  null/single-object/array to Vec (aliases last_recast_context /
  last_loop_action_context).
- engine: accumulate_loop_action_step (controller-reset + 16-step cap); mana
  and else arms seed/accumulate/clear; drive_loop_sequence_iteration loops the
  existing single-action drive; STEP-D voluntary-repeatability gate at the
  offer site; materialize drives N real cycles (per-cycle break-on-err
  defensive floor, offered-finite-loop class measured empty).
- casting_costs: recast capture writes a 1-element Vec gated
  !in_simulation_probe() so detection/materialize clone-drives never pollute
  the compared sequence (cover-compare invariant).
- resource: last_loop_action_sequence excluded from GameState PartialEq but
  explicitly compared where recurrence identity requires it.
- tests: Basalt+Power acceptance, accumulation, materialize Fixed(1)/Fixed(5),
  interruptibility pass-vs-respond, Off byte-identity, serde migration, and the
  cond-A non-targeted-opponent depletion no-op probe (12 new tests + field
  rename adaptations across existing loop_shortcut suites).

Assisted-by: ClaudeCode:claude-opus-4.8
…vation loop shortcuts (CR 732.2a)

Add matched discriminating pairs proving the CR 732.2a loop-shortcut offer is
genuinely interruptible for two more banked combos: an opponent holding a real
defuse GRANTS the shortcut when it passes and gets NO grant beyond the stack
when it responds -- the pass-vs-respond is the sole delta and flips the outcome.

- object-growth (Witherbloom, the Balancer + Sprout Swarm): opponent holds
  Murder; passing while Sprout is on the stack (CR 601.2i / CR 117.3c) offers
  the shortcut, while Murdering Witherbloom in that window removes granted
  affinity so the convoke-only {4}{G} recast is unpayable -> no offer
  (arm_murder + sprout_swarm_scenario_with_murder, n_fodder pinned to the
  no-affinity arithmetic).
- activation (Presence of Gond + Intruder Alarm): opponent holds Disenchant;
  passing offers, while Disenchanting Intruder Alarm on the {T} stack window
  (CR 602.2a) leaves the host tapped so the 2nd activation is illegal -> no
  offer (ported arm_disenchant / place_in_hand + intruder_alarm helper).

Each defused arm carries reach-guards (enabler destroyed AND the loop's own
action still occurred: a Saproling / one Elf made) so the no-offer is the
enabler-removal break, not a vacuous upstream failure. Non-vacuity is
revert-probed: swapping the opponent's defuse cast for a pass flips the
terminal state back to LoopShortcut{proposer:P0}.

Assisted-by: ClaudeCode:claude-opus-4.8
…p-window CR fix

CR 732.2a loop-shortcut interruptibility matched pair for the Vito + Sanguine
Bond + Bloodthirsty Conqueror drain combo. Undefused: opponent holds Murder but
passes -> loop settles -> LoopShortcut{proposer:P0} offered (win_kind=LethalDamage,
mandatory=false). Defused: opponent responds Murder->Bloodthirsty Conqueror (the
single load-bearing closer; Vito/Sanguine are redundant drainers) at the pre-offer
drain-on-stack priority window -> closer destroyed -> the two in-flight drains
resolve for exactly -2 (no re-gain) -> empty stack -> no offer. The opponent's
pass-vs-respond is the sole delta and flips offer<->no-offer (revert-probe measured
by the executor, independently re-measured byte-identical by review-impl).

Also corrects the untap-on-stack window citation in the mana-engine pair:
CR 605.3b (mana-ability-off-stack, the wrong rule for a non-mana ability) ->
CR 602.2a (activated ability created on the stack). CR 605.3b retained for the
mana beat's no-window contrast; line-613 cost-fuel 602.2b left untouched.

Test-only; no engine logic changed. HELD on the fork branch (not for push/PR).

Assisted-by: ClaudeCode:claude-opus-4.8
… CR 113.6)

The CR 732.2a loop-shortcut detector's board-recurrence firewalls scanned
abilities in ALL zones when deciding whether a live observer reads the growing /
projected class. A permanent trigger / static / replacement on a card in the
LIBRARY (an inert deck card) cannot function (CR 603.4 / CR 113.6) yet was
treated as a live observer, so the recurrence cover was rejected and the
interactive combo shortcut was never offered.

Reproduced from a real 4-player game: Witherbloom, the Balancer + Sprout Swarm
(infinite Saprolings) failed to prompt because Kodama of the East Tree sitting in
the player's LIBRARY was scanned as an observer of the growing creature class.

Fix: gate every observer scan on zone-of-function, reusing canonical predicates:
- triggers     -> triggers::trigger_definition_functions_in_zone (now pub(crate))
- statics      -> functioning_abilities::static_functions_in_zone
- replacements -> [Battlefield, Command] (find_applicable_replacements scope)

Sites fixed in analysis/resource.rs:
- fire_time_conditions_read_growing_class: (1) triggers, (3) replacements, (4) statics
- fire_time_conditions_read_projected_resource: (i) triggers, (ii) replacements, (iii) statics
- life_event_replacements_may_prompt (drain-cover life-prompt firewall)
- cost_surface_references_growing_class: skip Library (never a cost source) and
  zone-gate the static cost-mod sub-scan; the HAND surface stays (the loop's own
  recast spell rides there)

Adds discriminating regression test object_growth_library_observer_does_not_
suppress_offer; revert-probe verified (disabling the block-(1) gate flips it to
no-offer -> "got Priority").

Verified: analysis units 270 pass, loop_shortcut 66 pass, combo-verify corpus
13 confirmed / 4 gated / 37 deferred / 0 failed, engine lib clippy clean.

Assisted-by: ClaudeCode:claude-opus-4.8
…ng N iterations

CR 732.2a: accepting an unbounded object-growth (fodder/token) or mana-engine loop
shortcut now marks the certificate's unbounded axes via the shared mark_unbounded_loop
writer — the same path the reconcile/determinate crown uses — rather than replaying N
discrete iterations. The old drive was O(N) (~0.4s per token; 212s for 500 Saprolings)
and capped the 'infinite' at N; the frontend echoed the schema's Fixed(1) verbatim, so
accepting only ever minted ONE token.

Now accept APPLIES the infinite status: unbounded_resources gains the certified axes,
so the infinity HUD badge projects and (for mana) refill_infinite_mana holds the pool
at INFINITE_MANA_PER_TYPE. Zero objects are minted at accept; the finite count is named
later at the CR 500.5 phase/step boundary (follow-up: boundary finite-resolution prompt).

Measured on the reported 4-player state: accept marks {P0: {TokensCreated}}, +0
Saprolings (was +1..+500), 1.48s vs 212s. Mana engine: pool topped to 100,
count-independent (+98 for Fixed(1) and Fixed(5)).

Part 1 of the infinite-status lifecycle (mark on accept). Part 2 (CR 500.5 boundary
finite-resolution: prompt finite N for persistent axes, empty transient mana) follows.

Assisted-by: ClaudeCode:claude-opus-4.8
…efield

DESIGN step 4 of the CR 732.2a combo detector. When a player accepts an
object-growth loop shortcut, the engine already marked the per-player ∞
status + HUD badge, but the per-object battlefield pile was never rendered —
so tapped fodder tokens (e.g. Saprolings) showed ×N instead of ∞ in a live
game. This builds the missing rendering.

Engine:
- Snapshot the winning controller's tapped fodder-class members into a new
  `unbounded_loop_pile` map at loop-accept, re-derived via a one-period drive
  on a throwaway clone (live state untouched — shared-borrow signature).
- Excluded from PartialEq / normalize_for_loop / loop_fingerprint so display
  state cannot perturb CR 104.4b / CR 732.2a loop-detection equality.
- Project to `DerivedViews.unbounded_pile` (filtered by battlefield membership,
  CR 110.1), threaded through the multiplayer `filter_state_for_viewer` path.

Frontend (display-only): render `∞` count-independently via pure Set-membership
on `derived.unbounded_pile` across all three identical-permanent-group surfaces —
main board, opponent board-peek popover, and combat attack-target picker.

Tests: real-4p load-dump + build-fresh acceptance tests assert the pile equals
the controller's tapped fodder members (non-circular check; register revert-probe
flips both); frontend specs render ∞ vs ×N discriminatingly on every surface.

Known gap (deferred to a separate unit, disclosed in-code at select_convoke_taps):
the object-growth detection replay's convoke tap-selection (lowest-ObjectId-per-
color) can tap an untapped green cost-reducer instead of fodder, suppressing the
offer when the reducer is untapped at cast time.

Assisted-by: ClaudeCode:claude-opus-4.8
Basalt Monolith + Power Artifact makes infinite COLORLESS mana, but a real
4-player game showed the pool flooded with 100 units of every color. The
combo detector had correctly recorded `unbounded_resources = [Mana(Colorless)]`;
the bug was downstream in `refill_infinite_mana`, which flagged on any
`Mana(_)` axis and then unconditionally topped up all six `INFINITE_MANA_TYPES`
— fabricating W/U/B/R/G that no ability in the loop ever produced, and
illegally enabling colored-pip payment from a colorless-only engine.

CR 106.1b (six distinct mana types; colorless is its own type) + CR 106.4
(only mana an effect actually adds enters the pool): refill now tops up only
the mana colors present in that player's recorded axes. The debug
`SetInfiniteMana` toggle stores all six axes, so its output is byte-identical;
a color-specific loop (colorless) now refills colorless only.

Tests (real-4p load-dump + unit): the real Basalt dump refills colorless-only
with zero fabricated colors; a subset axis refills only its color; the all-six
debug axis still refills all six (over-narrowing guard). Revert-probe confirmed:
the pre-fix all-six body fails the first two and passes the third.

Assisted-by: ClaudeCode:claude-opus-4.8
The CR 732.2a object-growth loop detector replays the loop and checks net
board growth. Its convoke tap-selection (`select_convoke_taps`) picked the
lowest ObjectId per color, so an untapped green cost-reducer with a lower id
than the fodder tokens (e.g. Witherbloom, the Balancer below the Saprolings)
got convoke-tapped instead of fodder. That tapped the stable engine in the
replay, drifting the board-cover check, so the loop offer never surfaced —
observed live: with Witherbloom untapped, casting Sprout Swarm produced no
offer; tapping Witherbloom first made it fire.

CR 702.51a (convoke lets a player choose which creatures to tap) + CR 732.2a
(a legal, predictable loop sequence): a sustaining loop taps its fodder, not
its engine. Add `ConvokeTapOrder{Canonical, DetectionFodderFirst}`; the sole
production caller `resolve_pin(ConvokeTaps)` passes `DetectionFodderFirst`
(tokens/fodder first, then lowest id). The order is local to resolve_pin and
does not thread through `resolve()` — `select_convoke_taps` has exactly one
caller and live/AI/human convoke (the `TapForConvoke` path) never touches it,
so live gameplay is byte-unchanged. Fodder-first is preference-with-fallback:
the picker still taps the engine if fodder cannot cover a colored pip.

Tests: the real untapped-Witherbloom 4p playtest dump now surfaces the
LoopShortcut offer; two selector discriminators distinguish the modes (mixed
board: fodder-first taps the token, Canonical the nontoken); the four existing
unit tests keep asserting lowest-id under Canonical. Revert-probe: forcing
Canonical, or neutering the fodder-first sort, flips the real-dump offer to
no-offer.

Assisted-by: ClaudeCode:claude-opus-4.8
…kens at the boundary

Part 2 of the CR 732.2a combo detector. Part 1 marks the infinite status and
renders the pile but mints zero objects. This adds the payoff: when a player has
accepted an object-growth loop shortcut, the next phase/step boundary prompts the
loop controller for a finite count N (CR 500.5 boundary, reusing the
PayAmountChoice machinery), mints N concrete tapped tokens, and ends the infinite
status.

Engine:
- New `PayableResource::LoopCollapse` (unit variant) drives the boundary prompt;
  the mint arm early-returns and mints N tapped copies of the captured fodder
  profile via the copy-token path (CR 111.10 + CR 707.2 — the token is a copy of
  the loop fodder's copiable values).
- Capture the fodder's `CopiableValues` at loop-accept (where the fodder class is
  already derived and the loop sequence is still intact) into a new
  `pending_unbounded_materialization` stash — excluded from the loop-equality
  family like the rest of the unbounded_* state.
- A second collapse pass in `drain_pending_phase_transition_progress` runs after
  the CR 500.5 mana-empty APNAP drain, in APNAP order, leaving the empty phase
  progress intact so the post-mint re-drain restores priority in one action.
- `clear_unbounded_token_loop` clears only the `TokensCreated` axis (+ stash +
  token pile), preserving any coexisting axis (e.g. a debug infinite-mana axis)
  and the loop enablers — distinct from the whole-player `clear_unbounded_loop`.
- AI names N=1 (a conservative default that bounds search against the 1000-cycle
  MAX_SHORTCUT_CYCLES cap).

Frontend (display-only): render the LoopCollapse prompt through the existing
PayAmountChoice UI (types union + switch arm + i18n across all 7 locales).

Tests (real-4p load-dump + building-block, each revert-probed):
- T1 drives the real Sprout Swarm 4p dump accept -> boundary -> prompt ->
  SubmitPayAmount{5} -> 5 tapped Saprolings -> infinite cleared -> no re-prompt
  (revert-probe: no collapse pass -> no prompt).
- T2 the real Basalt mana dump does NOT prompt at the boundary (mana writes no
  stash; the collapse is token-only, the mana boundary-empty is deferred).
- T5 the axis-scoped clear preserves a coexisting mana axis (revert-probe: the
  whole-player clear wrongly wipes it).
- An AI-seam test asserts N=1 (revert-probe: the default explodes to 1001).

The CR 500.5 boundary mana-empty for realized-infinite mana is deferred (it
entangles with the debug infinite-mana toggle) and documented as a follow-up.

Assisted-by: ClaudeCode:claude-opus-4.8
Basalt Monolith + Power Artifact (and every infinite-mana combo) produced a
realized-infinite mana pool that persisted across step/phase boundaries: the
pool kept refilling because `keep_for_infinite_mana` retained mana for ANY
player carrying an `unbounded_resources` `Mana(_)` axis, which cannot tell the
developer `SetInfiniteMana` debug toggle apart from a detected/accepted loop
(their footprints are identical). So a loop the engine correctly marked infinite
also stayed infinite forever, contrary to CR 500.5 (unspent mana empties as a
step or phase ends).

Add a provenance discriminator and drain the loop-backed case:

- New `GameState.debug_infinite_mana: BTreeSet<PlayerId>` marks the players
  whose `Mana(_)` axes come from the `SetInfiniteMana` debug toggle. Written
  only by that debug handler (insert on enable, remove on disable). Excluded
  from `PartialEq` / `normalize_for_loop` / `loop_fingerprint` like the rest of
  the `unbounded_*` display state; `#[serde(default)]` keeps old dumps loading.
- Scope the keep-gate (turns.rs) to the debug marker instead of "has a Mana
  axis": a loop-backed pool now drains at the boundary (CR 500.5); the debug
  toggle still persists.
- New axis-scoped `clear_unbounded_mana_loop` de-realizes the drained Mana axis
  at the boundary so `refill_infinite_mana` cannot re-seed it, dropping the
  player's `unbounded_loop_enablers` in lockstep iff the axis set empties
  (CR 104.4b / CR 110.1) — a coexisting non-Mana axis (a Path-C `{Mana, Counter}`
  cover) keeps its enablers. Mirrors `clear_unbounded_token_loop`; mana carries
  no pile/stash (a mana engine reproduces no fodder).
- The boundary clear runs BEFORE the token-collapse pause so a player holding
  both a mana loop and a token loop has its mana axis cleared before the
  `SubmitPayAmount` re-drain, preventing a refill re-seed.

The debug `SetInfiniteMana` toggle is unchanged and still yields persistent
all-color infinite mana for playtesting.

Tests (real-4p load-dump + building-block, each revert-probed):
- Loop-backed mana drains + de-realizes at the boundary on the real Basalt dump
  (colorless 100 -> 0, Mana axis removed); reverting the axis-clear lets refill
  re-seed -> flips.
- The debug toggle persists (multi-authority: a player both debug-toggled and
  loop-backed keeps mana -- debug dominates).
- The axis-scoped clear preserves a coexisting Counter axis + enablers
  (lockstep-iff-empty) and drops both when only the Mana axis remains.
- A coexisting mana+token boundary drains the mana and still collapses the token
  loop; moving the clear after the token check re-seeds the pool -> flips.
- The `SetInfiniteMana` handler records/removes the marker (guards the sole
  production writer).

Assisted-by: ClaudeCode:claude-opus-4.8
…prompt

Bug B (stack overflow): the liminal-immediate copy-token batch drove one
mutually-recursive frame per token (commit_liminal_..._and_continue_copy_batch
-> continue_liminal_copy_token_batch -> apply_copy_token_after_replacement),
each frame a large im::HashMap COW insert, so minting large N overflowed the
stack (~200 tokens in WASM's smaller stack). Make the batch iterative (O(1)
stack) like the sibling non-liminal branch, preserving every pause/resume path
(counter-pause, post-replacement-drain, NeedsChoice, Prevented, terminal drain).
The terminal step COMPUTES Completed-vs-Paused instead of hardcoding Completed,
so multi-source copy effects don't double-mint when a later batch pauses on a
replacement choice.

Bug A (wording): the LoopCollapse token-count prompt no longer frames the
choice as 'pay N tokens'; it reads as choosing how many tokens to create from
infinite (display-only, all 7 locales).

CR 603.7 + CR 701.36a (created-token id ledger), CR 707.2 (copy), CR 616.1 (replacement choice), CR 732.2a (loop shortcut collapse).

Assisted-by: ClaudeCode:claude-opus-4.8
… non-fodder creature (CR 732.2a)

When the Sprout Swarm + Witherbloom, the Balancer convoke loop is demonstrated by
tapping Witherbloom (a non-fodder creature) instead of a Saproling, the CR 732.2a
object-growth shortcut was granted but no infinite tapped Saprolings materialized —
the infinite pile was built from live tapped fodder, which is empty when convoke
tapped Witherbloom rather than a token.

Fix: at accept, when the certified period actually taps a fodder creature each cycle
(period.taps_fodder) and the live board has no tapped fodder yet, seed a tapped
representative Saproling (the infinite-pile anchor, CR 111.1 + CR 110.5b) and an
untapped representative (the +1 remainder left by the final non-convoke-paid cast,
CR 702.51a). taps_fodder is measured on the same clone-drive the cover check already
runs, so it discriminates a convoke/tap-cost growth loop (seed) from a pure
untapped-partition growth loop (no seed) — closing the over-fire the cover's `>=`
admits. register_unbounded_loop_pile/register_pending_materialization stay ungated.

Board after accept: 6 untapped Saprolings + a tapped infinite pile + Witherbloom
tapped, cashing out to N tapped + 6 untapped at the loop boundary (CR 707.2 — tapped
status is set explicitly, not copied). Boundary handler byte-unchanged.

Assisted-by: ClaudeCode:claude-opus-4.8
…cycle decisions (CR 732.2a)

The live loop-shortcut detector failed to offer the infinite-charge shortcut for the
Kilo, Apogee Mind + Freed from the Real + Relic of Legends + Pentad Prism proliferate
combo — a mana-neutral, +1-charge/cycle unbounded loop (WinKind::Advantage, CR 104.4b).
Three fixes make it fire from a real game:

- FIX-1: record & replay the three fixed in-cycle choices (tap-target, mana-color,
  proliferate-target) via LoopActionContext.pins and a new PinnedDecision::ManaColor
  (CR 608.2d — choices announced while applying an effect), threaded through the
  decision-template schema, drive beat arms, viewer redaction, trigger ordering, the
  server-core payload guard, and the frontend DecisionPointKind union.
- FIX-2: wire loop_states_cover_modulo_counter_growth into the object-growth shortcut's
  empty-cover arm so the +1-charge/cycle growth certifies as an unbounded loop.
- FIX-3: conditional load-migration (GameState::migrate_transient_loop_sequence) drops a
  loaded save's stale pinless loop-history unless waiting_for is a shortcut window
  ({LoopShortcut, RespondToShortcut}), so a pre-fix save fires the offer promptly on
  reload without corrupting an offer-save's pinned sequence.

Acceptance is driven from the real 4-player playtest dump through the production
into_game_state chokepoint (the "combo fires in a real game" criterion), with an
interruptibility matched pair (undefused -> grant / Freed removed -> no grant) plus
identity-binding and mana-color pin-replay hostile tests.

Assisted-by: ClaudeCode:claude-opus-4.8
(cherry picked from commit dc7cc134017f7392fc8b94b320d5f852dd06dbea)
…growth loop is accepted (CR 732.2a)

Accepting the Kilo, Apogee Mind + Freed from the Real + Relic of Legends + Pentad
Prism proliferate loop (a +1-charge/cycle counter-growth loop, CR 701.34a) granted
the infinity HUD badge but left Pentad Prism's charge counter rendering its literal
value. Counter-growth marked only the per-player `unbounded_resources` axis, with no
per-object projection (object-growth has the `unbounded_pile` channel that renders
infinity on the token objects, but the counter axis `Counter(Other, Other)` is
object-agnostic — the object id is not recoverable from it).

Add a display-only per-object unbounded-counter channel mirroring `unbounded_pile`:
- `GameState.unbounded_counter_targets` (BTreeMap<PlayerId, BTreeSet<(ObjectId,
  CounterType)>>) — display state, EXCLUDED from loop-equality/normalize/fingerprint
  exactly like `unbounded_loop_pile`, guarded by a revert-probed exclusion test.
- Populated at `materialize_object_growth_shortcut` accept by RE-DERIVING the grown
  (object, counter_type) pairs: driving one period on a throwaway clone and diffing
  Generic counters (`grown_generic_counter_targets`, sharing the single-source
  `generic_counter_is_growable` with `classify_generic_counter_growth`). General over
  the class (One Ring burden, etc.), not proliferate-only.
- Projected to `DerivedViews.unbounded_counters` (mirrors the per-object
  `battlefield_keyword_badges` channel) -> `PermanentCard` renders the infinity glyph
  in place of the count on the matching counter pill.

Pentad's real counter count is never mutated (rules-correct; the infinity is
display-only, CR 122.1). Driven from the real 4-player dump through the production
accept path; the shared-drive extraction from `current_period_fodder` is
byte-preserving, so the Sprout convoke-fix tests stay green.

Assisted-by: ClaudeCode:claude-opus-4.8
…h loop firewall (CR 603.6a)

The CR 732.2a object-growth loop-shortcut offer was suppressed in realistic
multiplayer games whenever an opponent's Eminence commander (e.g. Inalla,
Archmage Ritualist) sat in the command zone. The observer firewall
fire_time_conditions_read_growing_class scans ETB observers in all zones and
vetoed on Inalla's CopyTokenOf body — even though its entry matcher ('another
nontoken Wizard you control', controller = the opponent) can never match the
loop's Saproling fodder (CR 603.6a checks the entering permanent against the
matcher).

Gate block(1) to skip an ETB observer whose entry matcher PROVABLY excludes the
growing fodder class, via new game::triggers::etb_observer_provably_excludes_class
(composing the same trigger_matchers::valid_card_matches used at fire time).
Fail-closed: a broad (no valid_card), disjunctive (zone_change_clauses),
non-battlefield-destination, or genuinely-matching observer still vetoes.
Soundness rests on the cover gate (board_covers_modulo_fodder, all-zones
content-equality) preceding the firewall, so the fodder is the only per-cycle
battlefield entrant.

Real 4p driven test (live cast via apply(), not load-then-inspect): the Sprout
Swarm + Witherbloom loop now OFFERS despite the opponent's command-zone Inalla;
a matched negative proves a broad matching observer still vetoes.

Assisted-by: ClaudeCode:claude-opus-4.8
…ted counter-growth loops (CR 732.2a)

The art-crop battlefield display mode (ArtCropCard, battlefieldCardDisplay ==
"art_crop") rendered the raw finite counter count with no unbounded-counter
subscription, so an accepted CR 732.2a counter-growth loop showed the finite
count (e.g. "2" charge) instead of the infinity glyph the full-card
PermanentCard mode already renders from derived.unbounded_counters. Subscribe
to derived.unbounded_counters here too and render infinity for a counter the
engine marks unbounded. Matched-pair component test guards the render flip
(marked => infinity, unmarked => count) so a future missed render site fails.

Assisted-by: ClaudeCode:claude-opus-4.8
…hase boundary (CR 732.2a)

When a counter / life / token growth loop is accepted as unbounded (CR 732.2a), defer its
persistent axes to the next CR 500.5 phase/step boundary, where the loop controller is
prompted (PayAmountChoice { LoopCollapse }) to name a finite N and the axes resolve to
exactly that many. Fixes the reported bug where an accepted ∞-counter loop was never
prompted to a finite number on moving to combat.

- Persistent-axis materialization stash (PersistentAxisMaterialization: Tokens / Counters /
  Life / DriveSequence). Unobserved axes batch an O(1) N×δ apply; observed axes replay the
  captured period N times through real apply() so per-cycle observers fire (CR 701.34a
  proliferate, replacement doublers). Transient mana drains at the boundary.
- Per-axis observation firewall (counter_growth_is_observed / life_growth_is_observed): a
  coarse axis-agnostic firewall mis-routed on a real 4p board carrying an incidental life
  observer. Re-checked at submit so an observer that drifts in during the accept→boundary
  window declines only its own axis (apply_counter_addition bypasses replacements; CR 732.2b
  never forces a shortcut).
- O(1) iterative mint replaces the recursive O(N)-depth copy path that overflowed the WASM
  stack at ~N=200.
- Regression tests drive the real accept-time registration end-to-end on real 4p dumps,
  including kilo_accept_collapses_at_boundary_to_exactly_n_counters (Kilo proliferate: accept
  ∞ → prompted at the boundary → SubmitPayAmount{5} → exactly +5 charge, pill cleared; a
  revert-probe on the DriveSequence registration FLIPS the boundary prompt).

Assisted-by: ClaudeCode:claude-opus-4.8
Mechanical/semantic adaptations to upstream API changes surfaced by cargo check
after replaying the combo-detector series onto upstream/main:
- resource.rs board_has_event_observer: adopt upstream ActiveTriggerDefinition
  iterator form (active.definition) — third active_trigger_definitions caller;
  CR 603.4/113.6 zone-gate preserved.
- ability_scan.rs: thread ScanMode/FilterReadContext through scan_player_filter
  and scan_target_filter; add ContinuousModification::SetTextName axes arm
  (CR 612.8/613.1c, sibling of SetChosenName — reads no board aggregate).
- triggers.rs etb_observer_provably_excludes_class: project the live functioning
  source via trigger_source_context_for_latch to match upstream's LKI-by-
  incarnation valid_card_matches source-context refactor; fail-closed.

Assisted-by: ClaudeCode:claude-opus-4.8
…th axis (CR 732.2a)

The finite-count prompt for an accepted CR 732.2a object-growth loop always read "tokens" even for counter/life loops. Add a display-only `LoopCollapseAxis { Tokens, Counters, Life, Mixed }` field on `PayableResource::LoopCollapse`, derived at the CR 500.5 phase/step boundary from the controller's pending-materialization stash via `LoopCollapseAxis::from_materializations` (exhaustive over all 17 `ResourceAxis` arms, no wildcard; the flagship Kilo combo's observed-growth `DriveSequence` counter axis is mapped).

The frontend `PayAmountChoiceUI` and all 7 locales now select axis-correct title/button keys. Counter/life labels are iteration-framed ("x{{value}}"), never a raw resource count: N is the loop's cycle count and each cycle applies per_cycle_delta, so N tokens but Nxdelta counters/life. The axis is a pure display descriptor -- the submit handler ignores it and resolves growth from the typed stash, so a stale label can never mis-resolve.

Tests: discriminating T1-T4 (counter/token/life/mixed) + a `from_materializations` unit test + a frontend render test + the flagship Kilo real-dump end-to-end `axis == Counters` assertion, each revert-probed.

add-engine-variant scope: display-only descriptor on an existing variant (no new sibling); cross-CR unification lives only at the display layer, allowed.

Assisted-by: ClaudeCode:claude-opus-4.8
…eplay can't overflow

Executing the Kilo, Apogee Mind + Relic of Legends + Freed from the Real +
Pentad Prism proliferate/charge loop dropped the engine ("connection lost").
Root cause: the accept-time loop-replay recursion (drive_persistent_axis_collapse
-> drive_loop_sequence_iteration -> apply) has a huge by-value GameState/
ActionResult frame; one loop period's replay needs 1-2 MiB of release-optimized
stack (measured: SIGABRTs a 1 MiB thread, survives 2 MiB), but the shipped WASM
had only wasm-ld's default 1 MiB shadow stack (17 memory pages) while the engine
assumes native's 16 MiB (RUST_MIN_STACK). It overflowed on replay cycle 1 in the
browser but never natively.

Give wasm32-unknown-unknown a matching 16 MiB shadow stack via a wasm-ld
-z stack-size link-arg in .cargo/config.toml. Proven in V8/node: the runtime
shadow stack scales exactly with the flag. No download-size change; cost is a
one-time ~15 MiB linear-memory reservation per module.

Guard: scripts/build-wasm.sh asserts the shipped engine_wasm_bg.wasm declares
initial memory min >= 200 pages, so a rebase silently dropping the config line
fails the build loudly (with flag: 365 pages; without: ~17). deploy.yml's
build-wasm job gains an explicit setup-node for the guard's node parser.

The throwaway native crash-repro is removed: a native test runs at
RUST_MIN_STACK=16 MiB and ignores -z stack-size, so it cannot guard a WASM
link-arg; the build-side memory-min assertion is the discriminating guard.

Assisted-by: ClaudeCode:claude-opus-4.8
@lgray
lgray force-pushed the feat/combo-detector-live branch from 3da49e8 to 90ad9da Compare July 20, 2026 19:40
The Paired-seed and Decision-cost quick gates ran with timeout-minutes: 30,
which flaked out on slow hosted runners. Measured budget on a card-data cache
miss: the "Generate card data" step alone is ~6.5m (5m `tool` build + gen),
then a cold debug build, then the debug run — the paired gate drives 30 games
(~34m at slow-runner ~1.1m/game) and the perf gate runs 5 cold child processes
x 3 scenarios x 3000 actions (~21m). Hosted-runner speed varies ~2x, so a fast
runner finishes under 30m while a slow one overruns and the job is cancelled.

Raise both quick gates to 60m. Coverage and the fixed perf workload are
unchanged; the higher ceiling only bills the failure case (same rationale the
nightly drift monitor already documents). The per-counter median assertion
(phase-rs#4878) stays variance-robust; only the wall-clock ceiling changes.

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

lgray commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

🤖 AI text below 🤖

CI timeout fix pushed (04d289c69) — and the two AI gates are a ceiling issue, not a feat regression

Both Paired-seed AI gate and Decision-cost perf gate failed on run 29772981579 by hitting the job timeout-minutes: 30 (both cancelled at 30m2x — a wall-clock overrun, not an assertion failure).

Root cause (measured)

The 30m ceiling is too tight for the gates' own debug workload on a slow hosted runner — this PR just triggers the workflow (via .cargo/config.toml), it does not slow the gates. Budget from the failing log:

  • Generate card data step alone = ~6.5m on a cache miss (5m03s tool-profile build + 1m27s gen).
  • Cold debug build of the gate binary.
  • The run itself: paired gate drives 30 games (--games 10 × 3 scenarios) ≈ 34m at the slow-runner ~1.1m/game seen on this run (red-mirror alone was 11.4m); perf gate runs 5 cold child processes × 3 scenarios × 3000 actions ≈ 21m in debug.

Hosted-runner speed varies ~2× (recent passing runs ranged 8m41s–16m06s for the same fixed workload), so a fast runner finishes under 30m while a slow one overruns.

This PR does not regress AI decision cost (measured)

  • git diff v0.31.0..HEAD touches zero AI-cost files: crates/phase-ai/** = 0 lines, data/card-data.json unchanged, perf-baseline.json unchanged.
  • The engine changes are all loop_detection.samples()-gated → Off in GameState::new_two_player (the gate's constructor), so the CR 732.2a detector never runs in these games.
  • Ran cargo ai-perf-gate locally against the committed baseline: 0 FAIL, 28 PASS, 0 NEW, 0 REMOVED — every per-counter median is within tolerance (all deltas ≤ 0).

The fix

04d289c69 raises both PR quick gates timeout-minutes: 30 → 60, with a comment documenting the budget. Coverage (--games 10) and the fixed perf workload are unchanged; the higher ceiling only bills the failure case — same rationale the nightly drift monitor already carries. Nightly variants (300m / 30m) untouched.

On the protected-workflow-path block

Understood that .github/workflows/deploy.yml (and now .github/workflows/ai-gate.yml) are protected paths you gate on. For context on why they're in the branch:

  • deploy.yml (+7, actions/setup-node@v4 node-22) is load-bearing for the crash fix: scripts/build-wasm.sh's assert_wasm_stack guard parses the shipped wasm's memory section with node to verify the 16 MiB shadow stack that stops the accept-time loop-replay overflow. release.yml already sets up node for its build-wasm job; this mirrors it for deploy.
  • ai-gate.yml (+14) is the timeout raise above — the minimal change to stop the flaky 30m overrun.

These are yours to apply/approve. If you'd prefer the workflow edits land through a maintainer-owned change instead of this feature PR, say the word and I'll drop them here and hand you the exact diffs (deploy.yml node setup + the two timeout-minutes: 60 bumps) to apply separately — the non-workflow parts of the crash fix (.cargo/config.toml stack size, build-wasm.sh guard) would need your workflow change to land first so deploy stays green.

@matthewevans matthewevans left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Maintainer review complete — no actionable finding on the current head.

✅ Clean

  • The protected deployment edit is a bounded prerequisite: .github/workflows/deploy.yml:310-315 installs Node 22 immediately before scripts/build-wasm.sh release, and scripts/build-wasm.sh:39-70 runs the new memory assertion after the engine artifact has passed through wasm-bindgen and optional wasm-opt.
  • .github/workflows/ai-gate.yml:26-32,106-112 changes only the two PR-job wall-clock ceilings. The gate commands, game count, perf sample count, and assertion logic are unchanged.
  • Gemini's open token_copy.rs:605 concern does not reproduce on this head. Each liminal entry carries the accumulated created_ids into finalization (token.rs:1343); finalization appends the just-committed object and writes the complete ledger (token.rs:1425-1426) before the loop reloads it at token_copy.rs:605. That is accumulation, not overwrite.

Recommendation: approve this head; do not enqueue it from this review.

@matthewevans matthewevans self-assigned this Jul 20, 2026
@matthewevans matthewevans added the feature Larger-scoped feature label Jul 20, 2026
@matthewevans matthewevans removed their assignment Jul 20, 2026
@matthewevans

Copy link
Copy Markdown
Member

Independent review-impl — head 04d289c69f

Reviewed the full diff (63 files, +13.4k/−1.2k) in a worktree via five parallel deep-reads (game-state threading + migration + serde; loop-drive/phase-boundary; LoopCollapse materialization; the ability_scan.rs census firewall; and the projection + frontend), plus personal verification of every finding at MED or above. Findings-only; silence = LGTM on a surface.

Confirmed clean (with evidence)

  • Gemini's token_copy.rs:605 CRITICAL is refuted — independently, and agreeing with your maintainer review. The batch ledger is carried through LiminalEntry.created_ids (seeded with the running accumulation at token_copy.rs:542) and rebuilt as entry.created_ids + [object_id] in finalization (token.rs:1425-1426), so line 605 reloads the full accumulated list, not a single ID. commit_liminal_copy_token_entry does not clear the vector — Gemini's premise is false.
  • New-field / rename threading + serde migration: clean. The RecastContext → LoopActionContext rename is compiler-enforced total (partition destructure at game_state.rs:806); migrate_transient_loop_sequence funnels through every restore path (WASM decode_restored_game_state, both wire decoders); new GameState fields are all #[serde(default, skip_serializing_if)] with back-compat aliases, so old saves deserialize. adapter/types.ts matches the Rust serialized shape for all four new surfaces (LoopCollapseAxis, PayableResource::LoopCollapse, DecisionPointKind::ManaColor, unbounded_counters).
  • Census firewall (ability_scan.rs, +3250): clean. No wildcard match arms (fail-closed to CONSERVATIVE); classification is structural, not card-tuned (card names live only in doc-comment exemplars); zone-gating is sound; ~40 CR base numbers all resolve and describe their code; linear AST walk, no O(bf²); the AI candidate arm correctly bounds a 1000-wide prompt to one N=1 candidate.
  • CR foundation verified — 732.2a/b/c, 500.4/500.5, 603.4/603.6a/113.6 all grep-clean against docs/MagicCompRules.txt and describe the annotated behavior.
  • wire contract: clean. Represented as membership sets (unbounded_pile: Vec<ObjectId>, unbounded_counters: HashMap<ObjectId, Vec<CounterType>>), never a numeric sentinel; the frontend renders engine membership only and does no arithmetic on (no NaN/overflow path). Display-only architecture rule holds.
  • Tests are non-vacuous — the new integration tests drive through apply() / GameRunner / real GameAction::{DeclareShortcut,RespondToShortcut} and carry documented revert-flip reach-guards (combo_infinite_pile.rs §non-vacuity note; kilo_interruptibility_tests len()==2 anchors before negatives).
  • Parse-diff sticky comment: "No card-parse changes detected" — consistent with the engine-only claimed scope; no parser blast radius.

Findings

[MED] The Tokens boundary-mint arm ignores a possible pause from drive_copy_token_batches, then unconditionally drains the phase transition. Evidence: crates/engine/src/game/engine_resolution_choices.rs:2196-2225 (mints count: amount, discards the drive result, collapsed.push) followed by the unconditional drain_pending_phase_transition_progress at :2290, whereas the copy resolver can set pending_copy_token_resolution = Some(..) and a replacement waiting_for mid-batch and return. The sibling Counters/Life/DriveSequence arms are observer-checked or pause-safe; only Tokens has no guard. Why it matters: if the mint ever pauses (a token/ETB replacement requiring a choice), the immediate drain advances the phase and overwrites waiting_for = Priority, corrupting state with a half-minted batch and an orphaned replacement choice. Reachability is gated by the offer-time firewall (a CR 732.2a shortcut can't contain a choice action), so this is latent today — but the seam is undefended if that invariant ever weakens. Suggested fix: after the mint, early-return WaitingFor when state.pending_copy_token_resolution.is_some() (or waiting_for is a replacement choice), mirroring the pause-aware returns elsewhere.

[LOW] Accept→boundary observer-drift orphans a declined axis's marks. Evidence: engine_resolution_choices.rs:2168 takes the whole stash, :2228/:2255 continue a now-observed Counters/Life item (never pushed to collapsed), and :2287 clear_collapsed_materializations clears only collapsed axes (game_state.rs:15556) — so the declined axis's entry in unbounded_resources / unbounded_counter_targets survives with its stash gone. The code comment (:2181) states leaving it for manual play is intentional (CR 732.2b never forces a shortcut) and game totals stay correct, so this is cosmetic — but I couldn't find a path that ever reclaims that badge or re-offers the declined axis. Please confirm the intended lifecycle: does the detector re-offer it, or is the badge stuck until clear_unbounded_loop? If stuck, either clear the declined axis marks here or re-route it to a DriveSequence replay.

[LOW] The Tokens axis assumes exactly one fodder token per loop period. Evidence: game_state.rs:3383 (Tokens(Box<CopiableValues>) carries no per-cycle count) minted as count: amount at engine_resolution_choices.rs:2202. A certified period whose net token production is k>1 per cycle collapses to N tokens instead of k·N. (Token-doubler replacements are handled correctly via replace_event on the batch; this is only the intrinsic "create two tokens per cycle" case.) Suggested fix: capture the per-period fodder count and mint count · amount, or assert the certified period is single-fodder so the limitation is explicit rather than a silent undercount.

[LOW] No end-to-end test exercises accept-time production routing for a real counter- or life-growth loop. Evidence: combo_infinite_pile.rs:1621-1635 (and :1753, :1883, :1907) graft the Counters/Life stash via register_pending_materialization onto a real token loop; the consumption path is production-driven and well-probed, but the accept-time batched-vs-DriveSequence decision at engine.rs:~2429 for a genuine counter/life loop is never reached. A misroute in that branch selection would not be caught. Suggested fix: add one scenario that accepts a real counter- (or life-) growth loop and asserts the resulting stash shape/axis.

[LOW] The counter tooltip leaks the finite count while the pill shows . Evidence: client/src/components/board/PermanentCard.tsx:918-933 and client/src/components/card/ArtCropCard.tsx:214-226 wrap the pill in <CounterTooltip count={count}>, and formatCounterTooltip (client/src/viewmodel/cardProps.ts:178-184) interpolates the raw finite count. Hovering an unbounded counter shows e.g. "…: 4", contradicting the pill — and the PR's own tests deliberately hide that count elsewhere (not.toContain("x4")). Suggested fix: thread isUnbounded into CounterTooltip and render /"unbounded" in the summary instead of the finite count.

[LOW] Hardcoded plural renders "Create 1 tokens" at value=1 in all 7 locales. Evidence: client/src/i18n/locales/en/game.json loopCollapseAmountTokens: "Create {{value}} tokens" (+ parallel de/es/fr/it/pl/pt). The stepper min can be low (tests use min: 0), so value=1 is reachable. Suggested fix: use i18next plural keys (loopCollapseAmountTokens_one/_other with {{count}}). (The ×N unit-framed variants are unaffected.)

Notes

  • Browser verification of the glyph rendering / badge positioning / z-index was not performed — UI correctness there is unverifiable from code alone; the vitest coverage is strong (matched marked/unmarked pairs at every display site + the count=1 singleton trap).
  • One weaker signal not filed as a finding: broadening per-activation last_loop_action_sequence recording (engine.rs) makes the empty-stack offer-drive hook fire after ordinary mana taps — but it's gated behind the samples() opt-in that is off in production, so no prod perf regression. Worth a glance if that gate ever defaults on.

Net: the architecture (threading, migration, census firewall, CR grounding, wire contract, test rigor) is solid; residual risk is concentrated in the newest boundary-materialization seam. Only the MED is worth a code change before merge (defense-in-depth guard); the rest are LOW polish/robustness/coverage items.

@lgray

lgray commented Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

I'll create a followup PR that takes care of the medium and five lows. Let's go ahead and get this merged. @matthewevans

@matthewevans
matthewevans merged commit 5495825 into phase-rs:main Jul 21, 2026
16 checks passed
lgray added a commit to lgray/phase that referenced this pull request Jul 21, 2026
Follow-up to phase-rs#6238 (merged). Addresses the 1 MED + 5 LOW findings from the
maintainer's independent review-impl:

- [MED] engine_resolution_choices: early-return the boundary Tokens mint when the
  copy-token batch pauses (pending_copy_token_resolution), so a paused mint can't
  cash out the Tokens infinity axis or advance the phase. Defense-in-depth: the
  CR 732.2a offer firewall (drive_loop_action_iteration's exhaustive _ => RecastAbort)
  makes this unreachable today. + discriminating test (single-optional pausing fodder).
- [LOW-1] Document the declined-axis infinity lifecycle (CR 732.2b capability marker;
  live retirement = re-detection re-collapse or debug toggle; enabler-departure is
  inert for object-growth marks pending a separate follow-up) + re-offer regression test.
- [LOW-2] Annotate the k==1 single-fodder invariant (derived_fodder_class's
  single-new-object gate makes k>1 unregisterable) + in-crate unit test.
- [LOW-3] New E2E driving a REAL accept through the accept-time batched-vs-DriveSequence
  routing for a genuine production loop (not a grafted stash).
- [LOW-4] Thread engine-provided isUnbounded into the counter tooltip so an infinity
  pill's tooltip shows infinity, not the finite count (+ summaryUnbounded in all 7 locales).
- [LOW-5] i18next _one/_other plural for the token loop-collapse label (fixes
  'Create 1 tokens' at value=1) across all 7 locales.

Gemini's token_copy.rs:605 'critical' is a refuted false premise (accumulation via
LiminalEntry.created_ids) - no change. The object-growth enabler-registration gap is
pre-existing and broader - deferred to a separate follow-up.

Assisted-by: ClaudeCode:claude-opus-4.8
matthewevans added a commit to andriypolanski/phase that referenced this pull request Jul 21, 2026
phase-rs#6259)

* fix(engine,client): address review findings on CR 732.2a loop-collapse

Follow-up to phase-rs#6238 (merged). Addresses the 1 MED + 5 LOW findings from the
maintainer's independent review-impl:

- [MED] engine_resolution_choices: early-return the boundary Tokens mint when the
  copy-token batch pauses (pending_copy_token_resolution), so a paused mint can't
  cash out the Tokens infinity axis or advance the phase. Defense-in-depth: the
  CR 732.2a offer firewall (drive_loop_action_iteration's exhaustive _ => RecastAbort)
  makes this unreachable today. + discriminating test (single-optional pausing fodder).
- [LOW-1] Document the declined-axis infinity lifecycle (CR 732.2b capability marker;
  live retirement = re-detection re-collapse or debug toggle; enabler-departure is
  inert for object-growth marks pending a separate follow-up) + re-offer regression test.
- [LOW-2] Annotate the k==1 single-fodder invariant (derived_fodder_class's
  single-new-object gate makes k>1 unregisterable) + in-crate unit test.
- [LOW-3] New E2E driving a REAL accept through the accept-time batched-vs-DriveSequence
  routing for a genuine production loop (not a grafted stash).
- [LOW-4] Thread engine-provided isUnbounded into the counter tooltip so an infinity
  pill's tooltip shows infinity, not the finite count (+ summaryUnbounded in all 7 locales).
- [LOW-5] i18next _one/_other plural for the token loop-collapse label (fixes
  'Create 1 tokens' at value=1) across all 7 locales.

Gemini's token_copy.rs:605 'critical' is a refuted false premise (accumulation via
LiminalEntry.created_ids) - no change. The object-growth enabler-registration gap is
pre-existing and broader - deferred to a separate follow-up.

Assisted-by: ClaudeCode:claude-opus-4.8

* fix(engine): make loop-collapse submit pause-safe for mixed-axis stashes

The LoopCollapse submit arm applied stashed persistent-axis materializations
in registration order, so a mixed-axis stash (Guide of Souls + Sprout Swarm =
tokens+life; Witherbloom, the Balancer + Sprout Swarm = tokens+counters) could
leave a finite-applied Counters/Life axis with a stale infinity mark if the
Tokens mint paused for a replacement choice (the guard early-returned before
the post-loop cash-out).

Process the only pause-prone axis (Tokens) last via a stable sort, and clear
the already-applied axes before the pause early-return, so the finite
non-token effect commits exactly once and only the still-paused Tokens axis
stays unbounded (CR 732.2a boundary collapse; CR 732.2b never forces a
shortcut, so the paused axis is left for manual play).

Adds med_mixed_counter_tokens_pause_commits_finite_counter_and_keeps_only_tokens_unbounded,
a mixed Counters+Tokens pause regression with both discriminators measured:
remove the sort -> counter never applied; remove the clear -> stale infinity.

Assisted-by: ClaudeCode:claude-opus-4.8

---------

Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
@lgray
lgray deleted the feat/combo-detector-live branch July 21, 2026 16:47
andriypolanski pushed a commit to andriypolanski/phase that referenced this pull request Jul 21, 2026
…to phase-server (phase-rs#6292)

Two distinct causes broke the first executed deploy since phase-rs#6238/phase-rs#6282:

1. setup-rust-toolchain exports RUSTFLAGS='-D warnings' by default, and an
   env RUSTFLAGS overrides ALL .cargo/config.toml rustflags — silently
   dropping the [target.wasm32-unknown-unknown] 16 MiB shadow-stack
   link-arg phase-rs#6238 added, tripping build-wasm.sh's assert_wasm_stack guard.
   Pass rustflags: '' on every wasm-building job (deploy build-wasm,
   release wasm + broker-wasm) so config.toml stays authoritative.

2. phase-rs#6282 gave phase-server a rustls-only reqwest, but the CI builds run
   'cargo build --bin phase-server' unscoped from the workspace root, so
   feature unification folds feed-scraper's native-tls reqwest features
   in, dragging openssl-sys into the musl cross-compile (no OpenSSL →
   build failure) and dynamic OpenSSL into the Docker image (runtime has
   no libssl). Scope every server build with -p phase-server: deploy,
   release (linux + matrix legs), Dockerfile compile stage, Tiltfile.

Verified: cargo tree -p phase-server -i openssl-sys --target
x86_64-unknown-linux-musl finds no path post-fix (present unscoped);
rustflags input semantics confirmed against the action's action.yml.

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

Labels

feature Larger-scoped feature

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants