fix(engine): infinity-badge/offer correctness + per-entry token occurrence index (combo-fb phases 3-4, chain 2) - #6839
Conversation
📝 WalkthroughWalkthroughThe PR adds accepted loop-materialization bounds and scheduled-axis tracking, updates collapse prompts and derived views, routes ETB-sensitive life growth through replay, and centralizes token and incubator zone-change bookkeeping. It also adds replay and integration coverage for these paths. ChangesLoop materialization and resource views
Token entry recording and replay
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Controller
participant GameState
participant CollapsePrompt
participant DerivedViews
Controller->>GameState: accept Fixed(n) shortcut
GameState->>CollapsePrompt: set collapse maximum to accepted bound
GameState->>DerivedViews: expose scheduled collapse axes
DerivedViews-->>Controller: hide scheduled unbounded projections
sequenceDiagram
participant TokenEffect
participant RecordZoneChange
participant GameState
participant TriggerPipeline
TokenEffect->>RecordZoneChange: submit token entry snapshot
RecordZoneChange->>GameState: assign zone-change index and record entry
RecordZoneChange->>TriggerPipeline: emit ZoneChanged event
TriggerPipeline-->>GameState: update ETB trigger state
Possibly related PRs
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
github-actions
Bot
added
the
needs-maintainer
2278049 to
7cf16a4
Compare
Parse changes introduced by this PR✓ No card-parse changes detected. |
|
🤖 AI text below 🤖 Rebased onto upstream/main |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer hold — current draft head 7cf16a49c9b23aee72cf15babad2d00c137587e8.
This chained PR explicitly remains draft until #6838 merges and rebases; it cannot be approved or enqueued on this head. I reviewed the current own arc and the full external-feedback surface: parse-diff is correctly no_changes, CodeRabbit skipped review because the PR is draft, and no review threads are open.
The required Rust suite is also failing/pending on the included predecessor. Re-open this for a fresh independent current-head review after #6838 is resolved, the branch rebases to its phases 3–4-only diff, the PR is marked ready, and required CI is clean.
7cf16a4 to
61d6adc
Compare
|
🤖 AI text below 🤖 Understood on the maintainer hold — this stays draft until #6838 merges and this branch rebases to its phases 3–4-only diff, then goes ready for a fresh independent current-head review. Head update only: the Chain invariant preserved — phase-0-2 ( Full response to the review findings is on #6838. |
|
Maintainer hold — fresh head The new delta from the prior held head is only the inherited rustfmt import-list rewrap in No approval or merge-queue action is appropriate yet. Please rebase to the promised phases-3–4-only range after #6838 merges, mark the PR ready for review, and wait for the fresh required checks; that new ready, green head needs an independent review before enqueue. |
61d6adc to
fe81c97
Compare
|
Maintainer hold on current head #6838 is formally changes-requested on head Exact next step: after #6838 lands, rebase this branch to the promised phases-3–4-only diff, then present a fresh non-draft head with current CI for review. |
…o infinity badge (CR 732.2c) Accepting a loop shortcut schedules the growth to materialize in one batch at the CR 500.5 boundary rather than replaying it per cycle. Between the accept and that boundary the board does not yet hold the growth, so the wire kept advertising an unbounded Life/Tokens/Counters axis beside a finite life total, a finite token pile, and finite counters -- an internally inconsistent HUD. Filter the PROJECTION, never the STORE: `scheduled_collapse_axes` becomes the single authority for "which axes will collapse", exhaustive over `PersistentAxisMaterialization` with no wildcard so a new variant breaks the build rather than silently rendering a stale infinity. Its three consumers -- the badge rows, the token pile, and the counter pills -- share that one derivation, so the surfaces can never disagree. The store keeps its marks, because the boundary collapse and `refill_infinite_mana` both read it. A `DriveSequence` is the exception the class rule has to carve out: its `collapsed_axes` is the loop's whole `proposal.unbounded`, not a deferral, and a `Mana(_)` among them is live NOW -- `refill_infinite_mana` re-tops the pool off the store after every action. Hiding it would show no infinity beside a pool that keeps refilling: the same inconsistency, inverted. Mana is today's only already-materialized axis; the census of production readers is recorded beside the filter, along with the condition a future axis must meet to join it. CR 732.2c also bounds the boundary prompt: the shortcut is taken at the count every player accepted, so the collapse may not exceed it. That bound is recorded only where a stash exists to bound -- gated on a measured stash-length delta, because a route that registers nothing would otherwise strand a bound that nothing clears and silently cap a later, larger, fully-agreed shortcut. Two accepts in one phase bind to the minimum, under-delivering rather than materializing growth the table never agreed to. Assisted-by: ClaudeCode:claude-opus-4.8
…s at full multiplayer (CR 732.2a) The object-growth veto treated every life-growing loop as unbounded, so a token-minting ETB lifegain engine (Soul Warden / Prosperous Innkeeper / Bogwater Lumaret, and the lifelink-ETB shape) never reached the CR 732.2a shortcut offer in a real multiplayer game. Route on an axis-shaped conjunct instead of an effect-shaped one: a life loop takes the ETB-sourced route only when it grows life, mints tokens, and the board actually carries a functioning enters-the-battlefield trigger. Each conjunct is independently load-bearing, so a mana-only loop with a board ETB trigger stays on the batched Life route. `board_has_functioning_etb_trigger` is shape-agnostic (any functioning ETB trigger, not a named effect), and the CR 113.6 zone gate it shares with the sibling firewall is extracted into `functioning_board_trigger_defs` so both walk the board through one authority. The low3 board-trigger axis is a typed `Low3BoardEtbTrigger` rather than a bool, per parameterize-don't-proliferate. Verification (measured, this commit alone on a clean base): cargo check --workspace --all-targets exit 0 combo_infinite_pile:: 27 passed / 0 failed (27 = the bucket's 28 minus the combined-gate test, which belongs to the follow-up commit; measured in a throwaway worktree on the clean base, not inferred from the full-tree run) Assisted-by: ClaudeCode:claude-opus-4.8
…rrence index Token entries were emitted with `turn_zone_change_index` hardcoded to 0, so every token entry in a turn aliased onto occurrence #0. The CR 603.2c batched replay guard dedups on `(definition_ref, turn_zone_change_index)`, so a second same-turn token batch collided with the first and its batched trigger fire was swallowed. Route token births through `restrictions::record_zone_change`, the single authority that assigns the index and performs the CR 403.3 battlefield-entry bookkeeping. The three co-located `record_battlefield_entry` calls are deleted in the same change, since the recorder does it and a second call double-counts. This exposed a live/replay desync in the CR 733 resolved-commands journal: the live path now pushes a ledger record for a token birth, but `apply_resolved_token_creation` did not, so a journaled token birth followed by a same-turn zone change failed closed with `TurnRecordIndexMismatch`. The replayer now reconstructs the entry record and records it through the same authority, which also closes a pre-existing gap where replay skipped the CR 403.3 bookkeeping entirely. Reconstruction rather than a new command field, so the serialized journal surface is unchanged. The same applier also skipped token ability injection. It now dispatches on the body variant, mirroring the live seam: a Copy body gets `finalize_copied_token` + the predefined-only injector (CR 707.2 + CR 702.33a strip cast-only characteristics off a token copy); a Spec body gets the catalog injector. Known ceiling, documented at the seam: the live journal point precedes entry counters, `attach_to`, and injection, so a replayed record can still differ from live for counter-carrying and Role/Aura token births. Closing those requires moving the live record point and is out of scope here. Note: `apply_resolved_token_creation` has no production caller yet — this is replay fidelity ahead of wiring, exercised by the cr733 suites. Verification (measured): cargo test -p engine 21975 passed / 0 failed / 15 ignored revert-probe: deleting the body dispatch fails 2 tests; replacing it with a blanket injector fails exactly 1, so both arms are separately load-bearing. Assisted-by: ClaudeCode:claude-opus-4.8
fe81c97 to
81a020f
Compare
|
🤖 AI text below 🤖 Ready for review, per your note: rebased onto merged #6838 landed as squash
Binary fixture change, disclosed —
Verified after construction: Local verification on CI is running on this head; I'll report here if anything comes back red. |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/engine/src/game/effects/token.rs (1)
1701-1722: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftThe CR 403.3 entry record now depends on event emission, so pause routes lose it. Deleting the unconditional
record_battlefield_entryfrom the liminal finalize tail moved the entry record insidepush_committed_token_entry_events, which is gated onTokenEntryEventEmission::Emit. On theSuppressroute the record is owed tohandle_copy_target_choice, and that emit is skipped on four pause returns fromfinish_copy_target_choice_entry: thecurrent_self_enter_replacement_choicepause, thereplay_deferred_entry_eventspause, the batch-delivery drain, andEnteringAuraAttachment::NeedsChoice. On those routes the token has nobattlefield_entries_this_turnrecord and nozone_changes_this_turnslot.
crates/engine/src/game/effects/token.rs#L1701-L1722: record the zone change unconditionally in this tail, and letTokenEntryEventEmissiongate only theZoneChangedandTokenCreatedpushes. Split the record step out ofpush_committed_token_entry_eventsso both modes reach it.crates/engine/src/game/engine_replacement.rs#L1777-L1805: once the record is unconditional, keeppaused_entry_emitfor the emit only, and correct the comment claim that the abandonment is pre-existing — it is pre-existing for theBecomeCopychain and the copy continuation, not for the entry record.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/effects/token.rs` around lines 1701 - 1722, The token finalization tail in crates/engine/src/game/effects/token.rs:1701-1722 must record the zone change unconditionally, while TokenEntryEventEmission gates only the ZoneChanged and TokenCreated event pushes; split that record step out of push_committed_token_entry_events so both emission modes reach it. In crates/engine/src/game/engine_replacement.rs:1777-1805, retain paused_entry_emit for event emission and update the comment to state abandonment is pre-existing only for the BecomeCopy chain and copy continuation, not the entry record.
🧹 Nitpick comments (3)
crates/engine/tests/integration/token_zone_change_index.rs (2)
318-333: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd the empty-ledger precondition that the sibling twin already asserts.
This test pins exact indices (
[0, 1]then[2]).mixed_group_sibling_then_token_each_fire_the_batched_triggerguards the same arithmetic with an explicitzone_changes_this_turn.len() == 0assertion. Without it, any scenario change that puts a zone change on the ledger before the batch makes this fail on the index values rather than on the precondition.♻️ Proposed precondition
let life_start = life_of_p0(runner.state()); + // The index arithmetic below is only legible if the per-turn ledger starts empty. + assert_eq!( + runner.state().zone_changes_this_turn.len(), + 0, + "the CR 400.7 per-turn zone-change ledger starts empty" + ); let tokens = mint_token_batch(runner.state_mut(), host, 2);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/token_zone_change_index.rs` around lines 318 - 333, Add an explicit assertion before minting the token batch in the relevant test that runner.state().zone_changes_this_turn.len() == 0, matching mixed_group_sibling_then_token_each_fire_the_batched_trigger. Keep the existing life delta and exact token index assertions unchanged.
613-621: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winLink the recorded follow-up to a tracked issue.
This comment records a measured, unfixed gap: no board ETB trigger fires on the copy-target-choice resume route, with
batched: trueand withbatched: false. The comment is the only record of it. A future reader cannot tell whether the gap is still open.Do you want me to open an issue that captures the measurement and reference it here?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/token_zone_change_index.rs` around lines 613 - 621, The comment documents an unfixed copy-target-choice resume gap without a trackable reference. Create or identify a tracked issue containing the measured behavior for both batched modes, then update the comment near the copy-target-choice resume assertions to include that issue’s reference while preserving the existing scope and details.crates/engine/src/ai_support/candidates.rs (1)
3030-3057: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winAdd a
max: 0test case for the new clamp.
(*max).min(1)exists specifically to keep this candidate legal whenmaxis0(a shortcut accepted atFixed(0)), per the added comment. The existingpay_amount_loop_collapse_offers_only_default_onetest only coversmax: 1000. Add a sibling case withmax: 0asserting the candidate submitsamount: 0, so a future edit to this clamp cannot silently regress the zero-bound scenario without a test failing.✅ Suggested additional test
#[test] fn pay_amount_loop_collapse_clamps_to_zero_when_max_is_zero() { let state = GameState { waiting_for: WaitingFor::PayAmountChoice { player: PlayerId(0), resource: PayableResource::LoopCollapse { axis: LoopCollapseAxis::Tokens, }, min: 0, max: 0, accumulated: 0, source_id: crate::types::identifiers::ObjectId(0), pending_mana_ability: None, }, ..GameState::new_two_player(42) }; let pay_amounts: Vec<u32> = candidate_actions(&state) .iter() .filter_map(|a| match a.action { GameAction::SubmitPayAmount { amount } => Some(amount), _ => None, }) .collect(); assert_eq!( pay_amounts, vec![0], "a shortcut accepted at Fixed(0) must offer amount 0, not an out-of-range 1" ); }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/candidates.rs` around lines 3030 - 3057, Add a sibling test next to pay_amount_loop_collapse_offers_only_default_one covering WaitingFor::PayAmountChoice with LoopCollapse and max: 0, then collect SubmitPayAmount candidates and assert the sole offered amount is 0. Keep the test focused on preserving the (*max).min(1) zero-bound behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@crates/engine/src/game/effects/token.rs`:
- Around line 1701-1722: The token finalization tail in
crates/engine/src/game/effects/token.rs:1701-1722 must record the zone change
unconditionally, while TokenEntryEventEmission gates only the ZoneChanged and
TokenCreated event pushes; split that record step out of
push_committed_token_entry_events so both emission modes reach it. In
crates/engine/src/game/engine_replacement.rs:1777-1805, retain paused_entry_emit
for event emission and update the comment to state abandonment is pre-existing
only for the BecomeCopy chain and copy continuation, not the entry record.
---
Nitpick comments:
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 3030-3057: Add a sibling test next to
pay_amount_loop_collapse_offers_only_default_one covering
WaitingFor::PayAmountChoice with LoopCollapse and max: 0, then collect
SubmitPayAmount candidates and assert the sole offered amount is 0. Keep the
test focused on preserving the (*max).min(1) zero-bound behavior.
In `@crates/engine/tests/integration/token_zone_change_index.rs`:
- Around line 318-333: Add an explicit assertion before minting the token batch
in the relevant test that runner.state().zone_changes_this_turn.len() == 0,
matching mixed_group_sibling_then_token_each_fire_the_batched_trigger. Keep the
existing life delta and exact token index assertions unchanged.
- Around line 613-621: The comment documents an unfixed copy-target-choice
resume gap without a trackable reference. Create or identify a tracked issue
containing the measured behavior for both batched modes, then update the comment
near the copy-target-choice resume assertions to include that issue’s reference
while preserving the existing scope and details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 812e1d5d-b9bc-4371-b2c3-345295356507
⛔ Files ignored due to path filters (2)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (19)
crates/engine/src/ai_support/candidates.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/effects/incubate.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/effects/token_copy.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_replacement.rscrates/engine/src/game/restrictions.rscrates/engine/src/game/turns.rscrates/engine/src/types/game_state.rscrates/engine/tests/integration/combo_infinite_pile.rscrates/engine/tests/integration/cr733_resolved_copy_token_creation.rscrates/engine/tests/integration/cr733_resolved_token_creation.rscrates/engine/tests/integration/kilo_live_offer_from_real_dump.rscrates/engine/tests/integration/loop_shortcut.rscrates/engine/tests/integration/loop_shortcut_mana_engine.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/token_zone_change_index.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Current-head review complete — awaiting the aggregate required check.
I reviewed 81a020fb36dfba0f736cfbc2332f475ef3af4a8a as the promised independent phases-3–4 range. Current parse evidence is no_changes; Rust lint, both test shards, card-data, frontend, WASM, Tauri, and security are green. The required aggregate Rust (fmt, clippy, test, coverage-gate) is still queued, so this is not ready for approval or enqueue yet.
I also traced the outside-diff token pause concern: the Suppress copy-entry path now stashes EmitCommittedCopyTokenEntry, and the pending-counter resolver calls push_committed_token_entry_events, which is the same recorder/emitter authority used by the unpaused tail. I found no remaining current-head blocker from that report. I will re-evaluate merge action once the aggregate check settles.
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 81a020fb36dfba0f736cfbc2332f475ef3af4a8a.
Evidence: the independent phases-3–4 diff is clean/mergeable; current parse evidence is no_changes; all required checks, including the aggregate Rust gate, are successful. I also verified the paused Suppress copy-token route reaches the shared recorder/emitter through EmitCommittedCopyTokenEntry, so the external pause-path concern is covered by the current implementation. Approval only; this sweep does not enqueue the PR.
|
🤖 AI text below 🤖 On the pause-path concern you cleared — I measured it and believe it is narrower than "covered". Your call on what to do about it; I have not touched the approved head. You wrote that the paused Exhaustive measurement in
There are five Since Reachability — stated honestly. Route 3 ( A fix is implemented and seam-tested locally — not pushed. It follows CodeRabbit's shape:
Your choice, both ready:
I did not push, because you had already given a disposition on exactly this concern and it is |
|
🤖 AI text below 🤖 Merged before you picked a route, so per the offer above I took the second one: the CR 403.3 It also carries two defects that an independent review found in my fix after I posted here —
Each fix carries a revert-probe with exactly one test flipping. Full drift gate green on the |
…date set The bounded-`UntilLethal` gate from the previous commit changes what the generator emits at the real F4 offer, and two §5 U6 rows pinned the pre-gate answer. They are this PR's own rows (added in 59afd02, "pin the bounded-offer behaviour it actually has"), so what they pinned was the behaviour the maintainer's Medium 5 identifies as wrong; re-pinning them is the discriminating test for that fix, not an accommodation to it. U6 (i) `..._is_untillethal_and_decline_only` => `..._is_decline_only`. Measured: `legal_actions` at the F4 offer is now `[DeclineShortcut]` alone. Both declare candidates are excluded, each by a different conjunct — `UntilLethal` by `!schema.is_bounded()` (new), `Fixed(max_iterations)` by `schema.points.is_empty()` (F4 publishes one point) — so the row carries one REVERT-PROBE per exclusion. A single probe would leave the other exclusion holding the assertion up and report a false pass. Both were RUN: * drop `!schema.is_bounded()` => `[DeclareShortcut { UntilLethal, None }, DeclineShortcut]` vs `[DeclineShortcut]` => FLIPPED, rc=101; * drop `schema.points.is_empty() &&` => `[DeclareShortcut { Fixed(35), None }, DeclineShortcut]` vs `[DeclineShortcut]` => FLIPPED, rc=101. Both restored byte-identical (`cmp`), and the F4 suite is 11/11 green with both gates in place. The row's third reach-guard claimed `predicted_winner == None` supplied REACHABILITY for `phase_ai::policies::loop_shortcut`'s `(None, UntilLethal)` reject arm. That claim is now FALSE — the generator can no longer put that pair in front of the policy from a bounded offer — so it is restated as a measured property of the board, with the policy's own row named as the arm's coverage. Leaving it as written would have been a false claim in a test. U6 (ii) `u6_the_ais_only_declare_candidate_is_refused_...` => `u6_no_declaration_the_generator_can_emit_opens_the_window_...`. The AI has no declare candidate at this node any more, so the old name and its doc table row ("the AI's own candidate") overclaimed. Its four one-axis `apply()` drives are unchanged and still measure the ENGINE guard — which is exactly the fact the generator gate depends on, so they are the reason the gate is correct rather than collateral. Its `for action in legal_actions(..)` loop now iterates a single element and is therefore WEAK on its own; that is said in the comment, and an `is_empty()` positive control is added so an empty candidate set cannot satisfy it vacuously. Also stales-out an upstream comment this PR broke: `candidates.rs:3040` (from phase-rs#6839) said the `LoopShortcut` arm "only ever proposes `UntilLethal`", hence no AI-declared shortcut could produce the `PayableResource::LoopCollapse` prompt. This PR's own 7d51e35 added the `Fixed(max_iterations)` candidate, and only a `Fixed` count routes through `materialize_fixed_shortcut` — the single path that registers the stash `turns.rs:455` turns into that prompt (traced, not assumed). The prompt is AI-reachable now; the comment says so. Assisted-by: ClaudeCode:claude-opus-5
…date set The bounded-`UntilLethal` gate from the previous commit changes what the generator emits at the real F4 offer, and two §5 U6 rows pinned the pre-gate answer. They are this PR's own rows (added in 59afd02, "pin the bounded-offer behaviour it actually has"), so what they pinned was the behaviour the maintainer's Medium 5 identifies as wrong; re-pinning them is the discriminating test for that fix, not an accommodation to it. U6 (i) `..._is_untillethal_and_decline_only` => `..._is_decline_only`. Measured: `legal_actions` at the F4 offer is now `[DeclineShortcut]` alone. Both declare candidates are excluded, each by a different conjunct — `UntilLethal` by `!schema.is_bounded()` (new), `Fixed(max_iterations)` by `schema.points.is_empty()` (F4 publishes one point) — so the row carries one REVERT-PROBE per exclusion. A single probe would leave the other exclusion holding the assertion up and report a false pass. Both were RUN: * drop `!schema.is_bounded()` => `[DeclareShortcut { UntilLethal, None }, DeclineShortcut]` vs `[DeclineShortcut]` => FLIPPED, rc=101; * drop `schema.points.is_empty() &&` => `[DeclareShortcut { Fixed(35), None }, DeclineShortcut]` vs `[DeclineShortcut]` => FLIPPED, rc=101. Both restored byte-identical (`cmp`), and the F4 suite is 11/11 green with both gates in place. The row's third reach-guard claimed `predicted_winner == None` supplied REACHABILITY for `phase_ai::policies::loop_shortcut`'s `(None, UntilLethal)` reject arm. That claim is now FALSE — the generator can no longer put that pair in front of the policy from a bounded offer — so it is restated as a measured property of the board, with the policy's own row named as the arm's coverage. Leaving it as written would have been a false claim in a test. U6 (ii) `u6_the_ais_only_declare_candidate_is_refused_...` => `u6_no_declaration_the_generator_can_emit_opens_the_window_...`. The AI has no declare candidate at this node any more, so the old name and its doc table row ("the AI's own candidate") overclaimed. Its four one-axis `apply()` drives are unchanged and still measure the ENGINE guard — which is exactly the fact the generator gate depends on, so they are the reason the gate is correct rather than collateral. Its `for action in legal_actions(..)` loop now iterates a single element and is therefore WEAK on its own; that is said in the comment, and an `is_empty()` positive control is added so an empty candidate set cannot satisfy it vacuously. Also stales-out an upstream comment this PR broke: `candidates.rs:3040` (from phase-rs#6839) said the `LoopShortcut` arm "only ever proposes `UntilLethal`", hence no AI-declared shortcut could produce the `PayableResource::LoopCollapse` prompt. This PR's own 7d51e35 added the `Fixed(max_iterations)` candidate, and only a `Fixed` count routes through `materialize_fixed_shortcut` — the single path that registers the stash `turns.rs:455` turns into that prompt (traced, not assumed). The prompt is AI-reachable now; the comment says so. Assisted-by: ClaudeCode:claude-opus-5
…date set The bounded-`UntilLethal` gate from the previous commit changes what the generator emits at the real F4 offer, and two §5 U6 rows pinned the pre-gate answer. They are this PR's own rows (added in 59afd02, "pin the bounded-offer behaviour it actually has"), so what they pinned was the behaviour the maintainer's Medium 5 identifies as wrong; re-pinning them is the discriminating test for that fix, not an accommodation to it. U6 (i) `..._is_untillethal_and_decline_only` => `..._is_decline_only`. Measured: `legal_actions` at the F4 offer is now `[DeclineShortcut]` alone. Both declare candidates are excluded, each by a different conjunct — `UntilLethal` by `!schema.is_bounded()` (new), `Fixed(max_iterations)` by `schema.points.is_empty()` (F4 publishes one point) — so the row carries one REVERT-PROBE per exclusion. A single probe would leave the other exclusion holding the assertion up and report a false pass. Both were RUN: * drop `!schema.is_bounded()` => `[DeclareShortcut { UntilLethal, None }, DeclineShortcut]` vs `[DeclineShortcut]` => FLIPPED, rc=101; * drop `schema.points.is_empty() &&` => `[DeclareShortcut { Fixed(35), None }, DeclineShortcut]` vs `[DeclineShortcut]` => FLIPPED, rc=101. Both restored byte-identical (`cmp`), and the F4 suite is 11/11 green with both gates in place. The row's third reach-guard claimed `predicted_winner == None` supplied REACHABILITY for `phase_ai::policies::loop_shortcut`'s `(None, UntilLethal)` reject arm. That claim is now FALSE — the generator can no longer put that pair in front of the policy from a bounded offer — so it is restated as a measured property of the board, with the policy's own row named as the arm's coverage. Leaving it as written would have been a false claim in a test. U6 (ii) `u6_the_ais_only_declare_candidate_is_refused_...` => `u6_no_declaration_the_generator_can_emit_opens_the_window_...`. The AI has no declare candidate at this node any more, so the old name and its doc table row ("the AI's own candidate") overclaimed. Its four one-axis `apply()` drives are unchanged and still measure the ENGINE guard — which is exactly the fact the generator gate depends on, so they are the reason the gate is correct rather than collateral. Its `for action in legal_actions(..)` loop now iterates a single element and is therefore WEAK on its own; that is said in the comment, and an `is_empty()` positive control is added so an empty candidate set cannot satisfy it vacuously. Also stales-out an upstream comment this PR broke: `candidates.rs:3040` (from phase-rs#6839) said the `LoopShortcut` arm "only ever proposes `UntilLethal`", hence no AI-declared shortcut could produce the `PayableResource::LoopCollapse` prompt. This PR's own 7d51e35 added the `Fixed(max_iterations)` candidate, and only a `Fixed` count routes through `materialize_fixed_shortcut` — the single path that registers the stash `turns.rs:455` turns into that prompt (traced, not assumed). The prompt is AI-reachable now; the comment says so. Assisted-by: ClaudeCode:claude-opus-5
2 participants
🤖 AI text below 🤖
DRAFT until chain link 1 merges — this diff currently includes chain-1 commits; after chain 1 lands this branch rebases, the diff reduces to phases 3–4 only, and the PR flips to non-draft. Chain management is handled by the author.
Summary
Chain 2 of the combo-detector player-feedback epic (phases 3–4): a resource whose collapse is already scheduled renders no ∞ badge (CR 732.2c); infinite-combo shortcuts are offered for ETB-sourced life loops at full multiplayer (CR 732.2a); and every token battlefield entry gets its own CR 603.2c occurrence index, fixing same-turn token batches aliasing onto occurrence #0 (which swallowed the second batch's batched trigger — exactly the workload combo loops generate).
Files changed
crates/engine/src/analysis/resource.rs,crates/engine/src/game/derived_views.rs— ∞-badge/collapse schedulingcrates/engine/src/ai_support/candidates.rs,crates/engine/src/game/engine.rs,game/turns.rs— ETB-life-loop offer at full multiplayercrates/engine/src/game/effects/{token,token_copy,incubate}.rs,game/engine_replacement.rs,game/restrictions.rs,crates/engine/src/types/game_state.rs— per-entry token occurrence index routed through therecord_zone_changesingle authoritycrates/engine/tests/fixtures/cr733/authority_matrix.json.gz,crates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gz— fixturescrates/engine/tests/integration/{combo_infinite_pile,cr733_resolved_copy_token_creation,cr733_resolved_token_creation,kilo_live_offer_from_real_dump,loop_shortcut,loop_shortcut_mana_engine,token_zone_change_index,main}.rs— coveragecrates/engine/src/game/triggers.rs— inherited via the chain-1 prefix: astyle(engine)commit (471484867) rewrapping the ability import list to rustfmt canonical form (one hunk, no semantic change), fixing thecargo fmt --all -- --checkCI failureTrack
Developer
LLM
Model: claude-fable-5
Tier: Frontier
Thinking: high
Implementation method (required)
Method: /engine-implementer
CR references
CR 732.2c (shortcut ends the loop — no ∞ badge for a scheduled collapse), CR 732.2a (shortcut offers), CR 603.2c (one trigger per event occurrence — per-entry token occurrence index), CR 733 (resolved-copy token creation coverage).
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head. (CI-owned alternative stated below, per author-directed remote-CI reliance for chain submissions.)
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
Branch-tip battery at
04b6bb600(contains this range, rebased on73fd7f6de):cargo check --workspace— 0;cargo clippy --workspace --all-targets -- -D warnings— 0;cargo test -p phase-enginelib — 18124 passed / 0 failed; integration — 4304 passed / 0 failed;cargo test -p phase-ai— 1930 passed / 0 failed.FORGE_TEST_FULL_DB=1 ordering_parity_sweeptwo-arm (merge-base vs tip, identical corpus) —unexplained=18both arms, byte-identical lists, delta-0.scripts/ai-perf-gate.shtwo-arm — 30-row tables byte-identical, delta-0.Phases 3–4 closed through the full
/engine-implementerpipeline with review-impl to zero findings (lane journal; phase-close SHAs19bb0f416phase 3,2c34a9759/36e9668d1phase 4, pre-rebase).Gate A
Deferred to the CI-owned parse-diff check for this submission (author-directed remote-CI reliance for the PR chain). head=61d6adcd1 base=02760f588. No
parser//oracle*paths in this range — expected parse delta is zero.Anchored on
crates/engine/src/game/restrictions.rs(record_zone_change) — the existing single zone-change authority; the token fix routes token births through it and deletes three co-locatedrecord_battlefield_entrycalls that would double-countcrates/engine/src/game/engine_replacement.rs— the existing CR 603.2c batched-replay dedup seam (dedups on(definition_ref, turn_zone_change_index)) that the per-entry index makes correctFinal review-impl
Final review-impl PASS at each phase close (zero findings; lane journal), re-verified at branch tip
04b6bb600by the full boundary battery above.Claimed parse impact
None.
Scope Expansion
None. Note: the token occurrence-index commit (
61d6adcd1) is a prerequisite engine fix with standalone value and the widest non-combo blast radius in the chain — it can be split into its own PR on request.Validation Failures
None.
CI Failures
None. (Rebased onto upstream/main
02760f5882026-07-31; chain rebased as a unit — phase-0-2 remains a strict prefix of phase-3-4. Conflicts: none — the one predicted overlap,crates/engine/tests/integration/main.rs, merged cleanly (additivemodlines on both sides); post-rebase that file is upstream's version plus exactly one added line,mod token_zone_change_index;, at its sorted position.git range-diff 73fd7f6de..22780493c 02760f588..61d6adcd1reports 8/8=on the pre-existing commits (patch-identical, zero content drift), plus one commit added after that rebase:471484867(the rustfmt rewrap answering review item 4).)Chain & series context
This PR is chain link 2 — predecessor: chain 1 (linked in first comment); successor: chain 3 (phase 5, 5a–5d bounded fast-forward + pinned optional links) opens when phase 5d completes.
Predecessor series (combo-detector): #4119 · #4480 · #4493/#4534 · #4547 · #4603 · #5672 · #6238 · #6259.
Summary by CodeRabbit