feat(engine): loop-shortcut bounded-offer core, pin injection, owner firewall, and measured 4p rows (combo-fb phases 5a-5d, chain 3) - #6886
Conversation
github-actions
Bot
added
the
needs-maintainer
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR adds bounded loop-shortcut offers and replay validation, moves resolution-choice classification into board-aware probing, centralizes player-choice eligibility, updates loop-state persistence, adds integration coverage, and introduces fixture migration and stamping tools. ChangesBounded loop shortcuts and probing
Choice eligibility
Fixture tooling and client labels
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 17
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/choose_from_zone.rs (1)
103-132: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
choosable_opponentsin the fallback.When fewer than two candidates exist,
resolve_chooserusesplayers::opponents, which includes phased-out players. A phased-out opponent can therefore receive the choice instead of the only legal opponent.🤖 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/choose_from_zone.rs` around lines 103 - 132, Update the fallback chooser flow after the candidate-count check so it does not select phased-out opponents through resolve_chooser. When fewer than two choosable opponents exist, reuse the eligible opponent list from players::choosable_opponents or otherwise resolve the chooser against that filtered set, while preserving the existing targeted-opponent behavior.
🧹 Nitpick comments (8)
scripts/lib/trigger-firing.jq (1)
82-86: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winGuard the install-command probe against non-object
commandvalues.
select(.command.DelayedTriggerInstall)indexes.commandwith a key. Serde serializes an externally tagged unit variant as a bare JSON string. If any journal entry carries such a command, jq aborts withCannot index string with "DelayedTriggerInstall". The file states that undetermined cases abort by name; a jq type error is not a named abort, and the operator cannot tell a shape problem from a real install root.Filter to objects first so the probe stays total.
♻️ Proposed fix
([ (.gameState.resolved_rules_journal.entries // [])[] - | select(.command.DelayedTriggerInstall) ] | length) as $installs + | select((.command | objects | has("DelayedTriggerInstall")) // false) ] | length) as $installs🤖 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 `@scripts/lib/trigger-firing.jq` around lines 82 - 86, Update the install-command probe in the resolved-rules journal calculation to inspect DelayedTriggerInstall only when .command is an object, preventing jq type errors for serialized string commands. Preserve the existing install count and named undetermined error behavior for valid object-shaped commands.scripts/stamp-fixture-firing.sh (1)
79-82: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winArm 2 compares aggregate carrier sums, so two opposite errors can cancel.
NEEDcollapses the pending, stack, and resolving carrier counts into one integer.GOTcollapses the same three classes into one integer. Arm 2 then compares only the totals.A missing carrier in one class and a surplus carrier in another produce equal totals and a green arm. The current derivation in
scripts/lib/trigger-firing.jqcannot produce that combination, because_firingaborts rather than skipping a record. The weakness is therefore latent today. It becomes live as soon as the derivation gains any non-aborting path, or when a committed fixture already carries a stalestack_trigger_firingsentry.Compare the three classes separately.
♻️ Proposed fix
Emit a per-class shape from both sides and compare those.
- NEED="$(gzip -dc "$FIX" | jq -c -f <(printf '%s\ntrigger_carrier_count\n' "$(cat "$LIB")"))" - GOT="$(gzip -dc "$TMP" | jq -c '((if .gameState.pending_trigger_firing then 1 else 0 end) - + (.gameState.stack_trigger_firings // {} | length) - + (if .gameState.resolving_trigger_firing then 1 else 0 end))')" + NEED_SHAPE="$(gzip -dc "$FIX" | jq -S -c ' + {pending: (if (.gameState.pending_trigger // null) != null then 1 else 0 end), + stack: ([ (.gameState.stack // [])[] | select(.kind.type == "TriggeredAbility") ] | length), + resolving: (if (((.gameState.resolving_stack_entry // .gameState.resolving_trigger).kind.type? // "") + == "TriggeredAbility") then 1 else 0 end)}')" + GOT_SHAPE="$(gzip -dc "$TMP" | jq -S -c ' + {pending: (if .gameState.pending_trigger_firing then 1 else 0 end), + stack: (.gameState.stack_trigger_firings // {} | length), + resolving: (if .gameState.resolving_trigger_firing then 1 else 0 end)}')" + NEED="$(printf '%s' "$NEED_SHAPE" | jq -c 'add')"Then key arm 2 on the shapes:
- if [ "$GOT" -eq "$NEED" ]; then ARM2=true; else ARM2=false; fi + if [ "$GOT_SHAPE" = "$NEED_SHAPE" ]; then ARM2=true; else ARM2=false; fi
trigger_carrier_countinscripts/lib/trigger-firing.jqcan expose the per-class object and keep the scalar astrigger_carrier_count | add, so the single-definition property holds.Also applies to: 105-108
🤖 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 `@scripts/stamp-fixture-firing.sh` around lines 79 - 82, Update the NEED and GOT derivations in the arm 2 comparison to emit matching per-class objects for pending, stack, and resolving trigger-carrier counts instead of only aggregate totals. Preserve trigger_carrier_count as the scalar sum by deriving it from the per-class object, and compare the complete shapes in both arm 2 locations so mismatches between classes cannot cancel out.crates/engine/src/analysis/resource.rs (3)
12195-12227: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
drive_one_beatduplicatesdump_drive_one_beatfrom the same module.
dump_drive_one_beatis defined at the top of thismod testsand is in scope here. The nesteddrive_one_beatis the same policy with the same body: pass atPriority, otherwise take the first legal non-terminal action, excludingConcedeandDebug.The doc comment justifies copying the policy from
tests/integration/loop_shortcut.rs, which is a separate crate and cannot be imported. That justification does not apply to a second copy inside the module that already has one. Both copies encode the drive policy the measurements in this file depend on, so a change to one silently invalidates the other's recorded beat counts.Call the module-level helper instead.
♻️ Proposed change
- /// One beat of the shared dump drive policy (`tests/integration/loop_shortcut.rs`'s - /// `dump_drive_one_beat`): at `Priority` always pass — the mandatory triggers resolve - /// and re-trigger, which IS the loop when there is one — and otherwise take the first - /// legal non-terminal action. - fn drive_one_beat(state: &mut GameState) -> Result<(), String> { - // ... duplicated body ... - } -Then replace the two call sites in this test with
dump_drive_one_beat(&mut state).🤖 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/analysis/resource.rs` around lines 12195 - 12227, Remove the nested drive_one_beat implementation and reuse the existing module-level dump_drive_one_beat helper. Update both call sites in this test to pass &mut state to dump_drive_one_beat, preserving the current drive policy and error behavior.
57-73: 🚀 Performance & Scalability | 🔵 TrivialConsider emitting a metric when the probe budget denies a charge.
PROBE_BUDGETis derived from a single measured offering beat (13 charges) on the current corpus, doubled. The PR body already records one consequence: dellian does not offer under this budget. Exhaustion is fail-closed, so correctness is preserved, but a starved acceptance in production is silent —MintMetercarriesdenied, and nothing surfaces it outside tests.A counter or structured log on
denied() == truewould let you see whether real tables hit the cap before a user reports a missing offer. That measurement is also what would justify the next re-derivation of the constant.🤖 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/analysis/resource.rs` around lines 57 - 73, Emit an operational metric or structured log whenever MintMeter reports denied() == true during probe charging, so probe-budget exhaustion is observable outside tests. Locate the charge-denial handling that consumes MintMeter and record the event there, including enough context to identify the affected classification or run; preserve the existing fail-closed behavior and PROBE_BUDGET semantics.
594-613: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winState and enforce the
frames_per_period >= 1invariant.
frames_per_perioddelimits a committed cycle ingame::engine::drive_one_shortcut_cycleand, per the doc above, supplies the per-period magnitude the CR 704 count bound divides by. A value of0is meaningless for both roles: as a delimiter it commits a cycle after no frames, as a divisor it is a panic.
PeriodicDeltaderivesDefault, soframes_per_period: 0is constructible in-crate today. Both current producers yield at least 1 (ring_delta_signaturesearcheskfrom1, and the direct-recurrence basis derives from a ring index), so this is a defence against a future producer rather than a live defect. Deserialization is already safe — the field carries no#[serde(default)], so an omitted key is a hard error.Make the invariant load-bearing rather than incidental: add a checked constructor, or have the consumer treat
0as a refusal.♻️ Proposed constructor
pub struct PeriodicDelta { pub frames_per_period: u32, pub delta: ResourceVector, pub victim_slot: Vec<(DecisionSlot, i64)>, } + +impl PeriodicDelta { + /// CR 732.2a: a period spans at least one retained ring frame. A zero + /// period is neither a cycle delimiter nor a divisor, so it is refused + /// here rather than at the consumer. + pub(crate) fn new( + frames_per_period: u32, + delta: ResourceVector, + victim_slot: Vec<(DecisionSlot, i64)>, + ) -> Option<Self> { + (frames_per_period >= 1).then_some(Self { + frames_per_period, + delta, + victim_slot, + }) + } +}🤖 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/analysis/resource.rs` around lines 594 - 613, Make the frames_per_period >= 1 invariant explicit for PeriodicDelta: add a checked constructor that rejects zero and update producers/deserialization-related creation paths to use it, or ensure game::engine::drive_one_shortcut_cycle refuses PeriodicDelta values with frames_per_period == 0 before using the value as a cycle delimiter or divisor. Preserve valid nonzero periods and avoid relying on the derived Default to create executable PeriodicDelta values.crates/engine/src/types/game_state.rs (1)
18954-18974: 🚀 Performance & Scalability | 🔵 Trivial
record_loop_detect_samplenow clones the whole game state twice per call.Previously this function produced one normalized snapshot via
normalize_for_loop. It now produces two full snapshots:normalized(vianormalize_for_loop, which itself starts withself.clone()) andlive(via the newloop_detect_live_sample, which also doesself.clone()). Both clones are rooted in the same unmodifiedself, so this is correct, but it doubles the clone cost ofGameStateon what the surrounding documentation describes as a per-resolution hot path (the post-pipeline priority frame). With the ring capped at 16 entries, this can retain up to 32GameState-equivalent payloads instead of 16.Many
GameStatefields are plainVec/HashMap/HashSet(notim-backed), so this cost is not fully amortized by structural sharing. Confirm this was profiled on a representative long game (many resolutions, large board) to be an acceptable trade-off, since there is no way to reduce this to one clone without losing either the raw or the normalized view.🤖 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/types/game_state.rs` around lines 18954 - 18974, Profile the per-resolution loop-detection path around record_loop_detect_sample on representative long games with large boards, measuring clone time and memory retention for the normalized and live snapshots. Confirm that retaining both GameState snapshots is an acceptable trade-off, and document the profiling result or adjust the design if the added cost is not acceptable.crates/engine/src/game/engine.rs (1)
13886-14021: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚖️ Poor tradeoffKey the source-census tests to symbols, not to line numbers.
Three rows assert facts about this file's own text:
the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_event,arc_as_ptr_beat_identity_is_the_sample_not_one_of_its_halves, andthe_period_touch_window_is_carried_by_the_live_half.The first pins exact
file:linecoordinates for all five producers. Its own doc records three re-baselines inside this one PR — U4, the 5d LOW-fix, and the rebase onto#6842— and each time the producer set was unchanged and only line numbers moved. The test therefore fires on unrelated edits above a producer, which trains a maintainer to re-baseline the number rather than to adjudicate the set. That defeats the row's stated purpose.Keep the invariant, drop the coordinate. Assert the sorted list of producer FILE PATHS plus the per-file producer count, and keep the total/partition assertion. A sixth producer still fails the row; a comment added above an existing one does not.
The other two rows already use
engine_fn_extentto anchor by signature, which is the right shape. Reuse that anchoring for the census row.Also applies to: 15966-15996, 16011-16066
🤖 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/engine.rs` around lines 13886 - 14021, The source-census test the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_event currently pins producer line numbers, causing unrelated edits to fail. Reuse engine_fn_extent or equivalent symbol-based anchoring to assert the sorted producer file paths and per-file counts, while preserving the total and producers/readers/in_test partition checks; remove coordinate-specific expectations and keep the existing symbol-based approach used by arc_as_ptr_beat_identity_is_the_sample_not_one_of_its_halves and the_period_touch_window_is_carried_by_the_live_half.Source: Path instructions
crates/phase-ai/src/policies/loop_shortcut.rs (1)
508-543: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the
n == max_iterationsboundary case.The row tests
Fixed(4)andFixed(11)againstmax_iterations == 10. It never testsFixed(10)— the exact countai_support::candidatesemits for a bounded offer.A policy that used
>=instead of>would reject the AI's own generated candidate, and every assertion in this file would still pass. The engine-side rowai_bounded_declare_candidate_is_generated_legal_and_drivescovers the boundary at the engine, not at this policy.Add a third arm at the bound.
💚 Proposed boundary arm
assert_eq!(kind_of(&outside), "loop_shortcut_bounded_declare_over_bound"); + + // (iii) AT the bound — the exact count `ai_support::candidates` emits for a bounded + // offer. A `>=` comparison in the policy would reject the AI's own candidate and no + // other assertion in this file would notice. + let at_bound = verdict_for(&state, &declare(IterationCount::Fixed(10))); + assert!( + matches!(at_bound, PolicyVerdict::Score { .. }), + "`max_iterations` itself is WITHIN the offered bound, got {at_bound:?}" + ); + assert_eq!(kind_of(&at_bound), "loop_shortcut_bounded_declare_progress"); }🤖 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/phase-ai/src/policies/loop_shortcut.rs` around lines 508 - 543, Add a third boundary-case arm to the test loop_shortcut_bounded_declare_scores_and_rejects_over_bound using Fixed(10), the state’s max_iterations value. Assert that this exact-bound declaration receives the same scoring verdict and bounded-progress kind as the within-bound case, with the critical-band delta check preserved, while keeping the existing over-bound rejection assertions unchanged.
🤖 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.
Inline comments:
In `@crates/engine/src/ai_support/candidates.rs`:
- Around line 3238-3253: Update the candidate emission condition around
schema.points.is_empty() to also require !schema.is_bounded(), preventing the
UntilLethal DeclareShortcut candidate from being generated for bounded schemas
while preserving it for unbounded schemas.
In `@crates/engine/src/analysis/loop_check.rs`:
- Around line 183-187: Update the `ShortcutProposal.per_cycle` serialization
path so populated `PeriodicDelta` values with `PlayerId`-keyed maps can be
serialized and deserialized through JSON, including persisted
`WaitingFor::RespondToShortcut` and the WASM `to_js` path. Add or reuse a
wire-safe map representation or serde adapter while preserving `None` omission
and the existing `PeriodicDelta` semantics.
In `@crates/engine/src/analysis/resource.rs`:
- Around line 4237-4243: Move the probe-budget charge to precede state cloning
in stack_entry_resolution_choice_freedom, returning MayPrompt when charging is
refused; apply the same charge-before-clone ordering in
optional_cleared_classification. Update
crates/engine/src/analysis/resource.rs:4237-4243 and :1636-1642 so spent()
counts every probe attempt and exhausted budgets prevent further board
allocations.
- Around line 276-312: Bind FrameIx to its owning container rather than
representing it as a reusable bare index: update the FrameIx definition and
frame_ix to mint a branded key, then make verdict validate ownership and return
None for foreign or invalid keys instead of indexing or computing a verdict.
Update every verdict consumer to propagate this refusal as the existing frame_ix
failure path, while preserving successful same-container memoization and verdict
computation.
- Around line 2099-2115: Make the empty-input guard in the
ResolutionChoiceFreedom::FreeUnlessReplacements branch load-bearing in release
builds by returning false when events is empty, while retaining the existing
debug_assert! for diagnostics. Ensure the any-based replacement check runs only
for non-empty events.
- Around line 1281-1286: Correct the explanatory comment beside the zero-delta
check in the candidate-period search, without changing the existing early return
behavior. Remove the incorrect claim that larger periods are multiples of the
smallest period, and state only that returning on a zero validated per-period
delta is an intentional fail-closed choice that may miss later candidates
because each period is checked independently.
In `@crates/engine/src/game/resolution_prompt.rs`:
- Around line 613-624: Update the choice-freedom traversal around
effect_resolution_choice_freedom so the chain root is probed only once: stop
passing the full ResolvedAbility a through effect_resolution_choice_freedom and
its allow-listed branches when that probe already resolves sub_ability and
else_ability. Retain recursion only for ability-level gates handled by the
checks before this block, avoiding repeated chain resolution, duplicated events,
and extra ProbeBudget charges.
- Around line 119-123: Update the prompt guard in probe_resolution to reject
whenever work.waiting_for is any non-priority variant, rather than comparing its
discriminant with state.waiting_for. Preserve the existing
ResolutionProbe::Prompted result and allow only the priority/absence case on the
incoming resolution board.
- Around line 84-96: Introduce a private resolution-board type produced by
stack::bind_resolution_scope, then change probe_resolution and
ability_resolution_choice_freedom to accept that type instead of &GameState.
Update production callers to pass the bound board returned by
bind_resolution_scope, preserving the existing resolution-scope binding and
preventing raw GameState values from reaching either API.
In `@crates/engine/src/game/targeting.rs`:
- Around line 3651-3653: The regression test
find_legal_targets_excludes_eliminated_player needs positive reach guards before
its exclusion checks. Assert that the eligible player is present in the Player
and Any results, and add an alive opponent before asserting the eliminated
player is absent from the ControllerRef::Opponent results.
In `@crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs`:
- Around line 11-14: Update the stale test-site count in the documentation for
the loop shortcut offer-writer census from 12 to 14, including the nearby
repeated “22/12” comment, so both prose references match the authoritative
assertion `(22, 14)`.
In `@crates/phase-ai/src/policies/loop_shortcut.rs`:
- Around line 227-247: Update the bounded-declaration match in the policy logic
around IterationCount::Fixed so Fixed(0) is handled by a separate guard inserted
after the over-bound rejection and before the scoring arm. Ensure zero-count
declarations do not receive the critical-band winning-declare score, while
preserving the existing scoring behavior for positive in-bound counts.
In `@scripts/lib/trigger-firing.jq`:
- Around line 98-112: Update the trigger-firing transformation to preserve
existing canonical carriers by binding the current stack trigger firings as
$sf_existing alongside $objs, then derive and assign pending_trigger_firing,
stack_trigger_firings, and resolving_trigger_firing only when their
corresponding values are absent. Retain existing values unchanged, including
delayed carriers, and avoid calling _firing for records that already have a
carrier.
- Around line 34-37: Update _defs to read serialized trigger_definitions
descriptions from each entry’s nested .definition.description, while continuing
to read base_trigger_definitions from their direct .description field. Preserve
the existing empty-array fallback and ensure granted or copied trigger
descriptions are retained without causing UNDETERMINED firing carrier failures.
In `@scripts/migrate-dump-fixture.sh`:
- Around line 138-140: Update the fixture-writing pipeline around the
destination assignment and its production call using $OUT so output is first
written to a mktemp file in the destination directory, then atomically moved to
$dest only after unzip, jq, and gzip complete successfully. Clean up the
temporary file on failure, preserving the existing destination when the jq
recipe aborts.
- Around line 181-188: Update Arm 2 in the migration validation flow to compare
only the canonical target_slots projection, specifically the effect_kind values,
between PATCHED and UNPATCHED; do not use whole-document equality because
stamp_trigger_firing and stamp_delayed_allocators introduce unrelated
differences. Report and fail the vacuous case when that projection is unchanged,
while preserving the existing success output for a real target_slots difference.
Add a separate validation arm for allocator and firing changes, equivalent to
arms 2 and 3 in stamp-fixture-firing.sh.
- Around line 125-136: Update the filter initialization and pristine processing
in the migration script to validate that the input envelope contains a non-null
gameState before applying the final {gameState:.gameState} projection. Reject
non-gameState envelopes explicitly, while preserving the existing patched-mode
transformations and valid gameState output.
---
Outside diff comments:
In `@crates/engine/src/game/effects/choose_from_zone.rs`:
- Around line 103-132: Update the fallback chooser flow after the
candidate-count check so it does not select phased-out opponents through
resolve_chooser. When fewer than two choosable opponents exist, reuse the
eligible opponent list from players::choosable_opponents or otherwise resolve
the chooser against that filtered set, while preserving the existing
targeted-opponent behavior.
---
Nitpick comments:
In `@crates/engine/src/analysis/resource.rs`:
- Around line 12195-12227: Remove the nested drive_one_beat implementation and
reuse the existing module-level dump_drive_one_beat helper. Update both call
sites in this test to pass &mut state to dump_drive_one_beat, preserving the
current drive policy and error behavior.
- Around line 57-73: Emit an operational metric or structured log whenever
MintMeter reports denied() == true during probe charging, so probe-budget
exhaustion is observable outside tests. Locate the charge-denial handling that
consumes MintMeter and record the event there, including enough context to
identify the affected classification or run; preserve the existing fail-closed
behavior and PROBE_BUDGET semantics.
- Around line 594-613: Make the frames_per_period >= 1 invariant explicit for
PeriodicDelta: add a checked constructor that rejects zero and update
producers/deserialization-related creation paths to use it, or ensure
game::engine::drive_one_shortcut_cycle refuses PeriodicDelta values with
frames_per_period == 0 before using the value as a cycle delimiter or divisor.
Preserve valid nonzero periods and avoid relying on the derived Default to
create executable PeriodicDelta values.
In `@crates/engine/src/game/engine.rs`:
- Around line 13886-14021: The source-census test
the_cr_603_5_prompt_census_is_pinned_so_a_sixth_producer_is_a_counted_event
currently pins producer line numbers, causing unrelated edits to fail. Reuse
engine_fn_extent or equivalent symbol-based anchoring to assert the sorted
producer file paths and per-file counts, while preserving the total and
producers/readers/in_test partition checks; remove coordinate-specific
expectations and keep the existing symbol-based approach used by
arc_as_ptr_beat_identity_is_the_sample_not_one_of_its_halves and
the_period_touch_window_is_carried_by_the_live_half.
In `@crates/engine/src/types/game_state.rs`:
- Around line 18954-18974: Profile the per-resolution loop-detection path around
record_loop_detect_sample on representative long games with large boards,
measuring clone time and memory retention for the normalized and live snapshots.
Confirm that retaining both GameState snapshots is an acceptable trade-off, and
document the profiling result or adjust the design if the added cost is not
acceptable.
In `@crates/phase-ai/src/policies/loop_shortcut.rs`:
- Around line 508-543: Add a third boundary-case arm to the test
loop_shortcut_bounded_declare_scores_and_rejects_over_bound using Fixed(10), the
state’s max_iterations value. Assert that this exact-bound declaration receives
the same scoring verdict and bounded-progress kind as the within-bound case,
with the critical-band delta check preserved, while keeping the existing
over-bound rejection assertions unchanged.
In `@scripts/lib/trigger-firing.jq`:
- Around line 82-86: Update the install-command probe in the resolved-rules
journal calculation to inspect DelayedTriggerInstall only when .command is an
object, preventing jq type errors for serialized string commands. Preserve the
existing install count and named undetermined error behavior for valid
object-shaped commands.
In `@scripts/stamp-fixture-firing.sh`:
- Around line 79-82: Update the NEED and GOT derivations in the arm 2 comparison
to emit matching per-class objects for pending, stack, and resolving
trigger-carrier counts instead of only aggregate totals. Preserve
trigger_carrier_count as the scalar sum by deriving it from the per-class
object, and compare the complete shapes in both arm 2 locations so mismatches
between classes cannot cancel out.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8c5ac462-43d1-44b8-8ee5-40efe02d18ca
⛔ Files ignored due to path filters (6)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (48)
crates/engine/src/ai_support/candidates.rscrates/engine/src/analysis/corpus_tests.rscrates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/effects/choose.rscrates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/clash.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/proliferate.rscrates/engine/src/game/effects/separate_piles.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine.rscrates/engine/src/game/filter.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mod.rscrates/engine/src/game/phasing.rscrates/engine/src/game/players.rscrates/engine/src/game/replacement.rscrates/engine/src/game/resolution_prompt.rscrates/engine/src/game/sba.rscrates/engine/src/game/stack.rscrates/engine/src/game/targeting.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/tests/integration/fantastic_four_bounded_loop.rscrates/engine/tests/integration/gift_recipient_phased_out_opponent.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.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/loop_shortcut_offer_writer_census.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/rules/battle.rscrates/engine/tests/integration/sprout_inalla_realistic_offer.rscrates/phase-ai/src/policies/loop_shortcut.rscrates/phase-ai/src/projection.rscrates/phase-ai/src/search.rsscripts/lib/trigger-firing.jqscripts/migrate-dump-fixture.shscripts/stamp-fixture-firing.sh
| /// CR 732.2a: copied verbatim off the confirmed certificate so the drive reads ONE | ||
| /// authority for what a conformant cycle looks like. `None` for every offer whose | ||
| /// producer states no per-period signature (see [`LoopCertificate::per_cycle`]). | ||
| #[serde(default, skip_serializing_if = "Option::is_none")] | ||
| pub per_cycle: Option<crate::analysis::resource::PeriodicDelta>, |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Locate ShortcutProposal, its derives, and every serialization site that carries it.
ast-grep run --pattern 'pub struct ShortcutProposal { $$$ }' --lang rust crates/engine/src
# Any serde round-trip test naming ShortcutProposal?
rg -nP --type=rust -C4 '\bShortcutProposal\b' crates/engine crates/engine-wasm | rg -n -C4 'serde_json|to_string|from_str|from_value|Serialize|Deserialize'
# DecisionTemplate is the other payload on ShortcutProposal — check its key types for map keys.
ast-grep run --pattern 'pub struct DecisionTemplate { $$$ }' --lang rust crates/engine/srcRepository: phase-rs/phase
Length of output: 5730
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- ShortcutProposal and surrounding declarations ---'
sed -n '145,195p' crates/engine/src/analysis/loop_check.rs
printf '%s\n' '--- DecisionTemplate and nested types ---'
sed -n '1,180p' crates/engine/src/analysis/decision_template.rs
rg -n -C3 'enum DecisionGroupKey|struct PinnedDecision|enum ReplayMode|type DecisionGroupKey' crates/engine/src/analysis
printf '%s\n' '--- Serialization and deserialization call sites ---'
rg -n -C4 --type rust 'ShortcutProposal|LoopCertificate|LoopShortcut|serde_json|to_string|from_str|from_value|Serialize|Deserialize' crates/engine crates/engine-wasm
printf '%s\n' '--- Explicit round-trip tests and panic/error handling ---'
rg -n -C5 --type rust 'round.?trip|periodic_delta_survives|serde_json|expect\(|unwrap\(|panic!' crates/engine crates/engine-wasmRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- loop_check.rs ---'
sed -n '60,195p' crates/engine/src/analysis/loop_check.rs
printf '%s\n' '--- decision_template.rs declarations ---'
rg -n -C5 '^(pub )?(struct|enum|type) (DecisionTemplate|PinnedDecision|DecisionGroupKey|ReplayMode)\b' crates/engine/src/analysis/decision_template.rs
printf '%s\n' '--- exact ShortcutProposal references ---'
rg -n -C3 --glob '*.rs' '\bShortcutProposal\b' crates/engine crates/engine-wasm
printf '%s\n' '--- exact serialization references near loop shortcut types ---'
rg -n -C3 --glob '*.rs' '(serde_json|to_string|from_str|from_value|serialize|deserialize)' crates/engine/src/analysis crates/engine-wasm/srcRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- persisted RespondToShortcut ingress test ---'
sed -n '10770,10855p' crates/engine/tests/integration/loop_shortcut.rs
printf '%s\n' '--- PeriodicDelta and ResourceVector serialization shape ---'
rg -n -C8 'struct PeriodicDelta|struct ResourceVector|counter_key_pairs|PlayerId' crates/engine/src/analysis/resource.rs crates/engine/src/types/player.rs
printf '%s\n' '--- GameState serde boundary and WASM restore path ---'
rg -n -C5 'serde_json|from_slice|from_str|from_value|to_vec|to_string|GameState|waiting_for' crates/engine-wasm/src crates/engine/src/types/game_state.rs | head -n 500
printf '%s\n' '--- round-trip tests for ShortcutProposal or RespondToShortcut ---'
rg -n -C6 'per_cycle|RespondToShortcut|ShortcutProposal.*serialize|serialize.*ShortcutProposal|invalid type: string' crates/engine/tests/integration/loop_shortcut.rs crates/engine/srcRepository: phase-rs/phase
Length of output: 50371
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- exact persisted-restore test block ---'
sed -n '10778,10848p' crates/engine/tests/integration/loop_shortcut.rs
printf '%s\n' '--- PeriodicDelta declarations and serde attributes ---'
sed -n '1,220p' crates/engine/src/analysis/resource.rs | rg -n -C10 'PeriodicDelta|ResourceVector|BTreeMap|HashMap'
rg -n -C8 'pub struct PeriodicDelta|pub struct ResourceVector|counter_key_pairs|life:' crates/engine/src/analysis/resource.rs
printf '%s\n' '--- bounded proposal construction ---'
sed -n '4335,4385p' crates/engine/src/game/engine.rs
sed -n '2128,2225p' crates/engine/src/game/engine.rs
printf '%s\n' '--- WASM JSON boundary ---'
rg -n -C8 --glob '*.rs' 'serde_json::(to|from)|from_slice|to_vec|expect\(' crates/engine-wasm/srcRepository: phase-rs/phase
Length of output: 50370
🏁 Script executed:
#!/bin/bash
set -e
printf '%s\n' '--- PlayerId serde implementation ---'
rg -n -C12 'struct PlayerId|impl Serialize for PlayerId|impl.*Deserialize.*PlayerId' crates/engine/src
printf '%s\n' '--- PeriodicDelta complete field list ---'
sed -n '586,665p' crates/engine/src/analysis/resource.rs
printf '%s\n' '--- existing wire test body ---'
sed -n '12335,12435p' crates/engine/src/analysis/resource.rs
printf '%s\n' '--- WASM state projection callers of to_js ---'
rg -n -C5 --glob '*.rs' '\bto_js\(' crates/engine-wasm/src/lib.rsRepository: phase-rs/phase
Length of output: 34236
🏁 Script executed:
#!/bin/bash
set -e
python3 - <<'PY'
from pathlib import Path
loop_check = Path("crates/engine/src/analysis/loop_check.rs").read_text()
resource = Path("crates/engine/src/analysis/resource.rs").read_text()
engine = Path("crates/engine/src/game/engine.rs").read_text()
player = Path("crates/engine/src/types/player.rs").read_text()
wasm = Path("crates/engine-wasm/src/lib.rs").read_text()
integration = Path("crates/engine/tests/integration/loop_shortcut.rs").read_text()
checks = {
"ShortcutProposal serializes per_cycle": (
"pub per_cycle: Option<crate::analysis::resource::PeriodicDelta>" in loop_check
),
"PeriodicDelta contains ResourceVector": (
"pub delta: ResourceVector" in resource
),
"ResourceVector has PlayerId-keyed life map": (
"pub life: BTreeMap<PlayerId, i64>" in resource
),
"PlayerId is transparent u8": (
"#[serde(transparent)]\npub struct PlayerId(pub u8);" in player
),
"bounded proposal copies certificate per_cycle": (
"per_cycle: offer.certificate.per_cycle.clone()" in engine
),
"WASM generic serializer panics on serialization errors": (
'unwrap_or_else(|e| panic!("serde_json serialization failed: {e}"))' in wasm
),
"integration source records the non-round-tripping restore case": (
'invalid type: string "0", expected' in integration
and "proposal.per_cycle = None;" in integration
),
}
for name, ok in checks.items():
print(f"{'PASS' if ok else 'FAIL'}: {name}")
if not all(checks.values()):
raise SystemExit(1)
PYRepository: phase-rs/phase
Length of output: 504
Make ShortcutProposal.per_cycle JSON-round-trippable.
A populated PeriodicDelta contains PlayerId-keyed maps. Persisted WaitingFor::RespondToShortcut then fails to deserialize, and the WASM to_js path panics when it serializes the proposal. Add a wire-safe map representation or serde adaptor.
🤖 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/analysis/loop_check.rs` around lines 183 - 187, Update the
`ShortcutProposal.per_cycle` serialization path so populated `PeriodicDelta`
values with `PlayerId`-keyed maps can be serialized and deserialized through
JSON, including persisted `WaitingFor::RespondToShortcut` and the WASM `to_js`
path. Add or reuse a wire-safe map representation or serde adapter while
preserving `None` omission and the existing `PeriodicDelta` semantics.
| pub(crate) fn frame_ix(&self, frame: &GameState) -> Option<FrameIx> { | ||
| self.frames | ||
| .iter() | ||
| .rposition(|f| std::ptr::eq(*f, frame)) | ||
| .map(FrameIx) | ||
| } | ||
|
|
||
| /// THE ONE DOOR. Computes on miss against `self.frames[f.0]` — the memo, | ||
| /// never the caller, converts `FrameIx` back to a board — charges the | ||
| /// OWNED budget, and memoizes. Total over minted keys: it returns a | ||
| /// value, never an `Option`, so there is no miss contract to get wrong. | ||
| pub(crate) fn verdict(&mut self, f: FrameIx, entry: &StackEntry) -> &EntryVerdict { | ||
| let key = (f, entry.id); | ||
| if !self.memo.contains_key(&key) { | ||
| let frame = self.frames[f.0]; | ||
| let published = self | ||
| .proposer | ||
| .and_then(|p| crate::game::engine::entry_publishes_pin_slots(frame, entry, p)); | ||
| let primary = | ||
| super::stack_entry_resolution_choice_freedom(frame, entry, &mut self.budget); | ||
| let residual = match published.as_ref().and_then(|p| p.may.as_ref()) { | ||
| Some(_) => { | ||
| super::optional_cleared_classification(frame, entry, &mut self.budget) | ||
| } | ||
| None => None, | ||
| }; | ||
| self.memo.insert( | ||
| key, | ||
| EntryVerdict { | ||
| published, | ||
| primary, | ||
| residual, | ||
| }, | ||
| ); | ||
| } | ||
| &self.memo[&key] | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
FrameIx is not bound to the container that minted it.
FrameIx is pub(crate) and Copy, and its only correctness property is that frame_ix resolved it against self.frames. Nothing in the type prevents a FrameIx minted by container A from being passed to container B's verdict. Two failure modes follow:
- If B's
framesis shorter,self.frames[f.0]panics on an index out of bounds. - If B's
framesis at least as long, the memo silently computes and caches a verdict for the wrong frame — the exact class the doc comment claims is "unconstructible rather than merely unobserved".
The doc comment covers forging (E0603) but not cross-container reuse. Every call site currently re-mints per container by convention only; r22_conjunct4_the_effective_key_carries_the_containers_proposer relies on that convention in prose rather than in the type.
Bind the index to its container so a mismatch is a refusal, not a panic or a wrong verdict.
🛡️ Proposed fix: brand `FrameIx` with the owning container
+ /// A process-unique id per container, so a `FrameIx` cannot be spent
+ /// against a container that did not mint it.
+ static NEXT_CONTAINER_ID: std::sync::atomic::AtomicU64 = std::sync::atomic::AtomicU64::new(0);
+
#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)]
- pub(crate) struct FrameIx(usize);
+ pub(crate) struct FrameIx {
+ container: u64,
+ index: usize,
+ } pub(crate) fn frame_ix(&self, frame: &GameState) -> Option<FrameIx> {
self.frames
.iter()
.rposition(|f| std::ptr::eq(*f, frame))
- .map(FrameIx)
+ .map(|index| FrameIx {
+ container: self.container_id,
+ index,
+ })
}verdict then refuses a foreign index rather than indexing with it. That requires verdict to answer for a miss; the fail-closed reading matching every other seam in this module is to return None and have each consumer treat it as a refusal, the same way they already treat frame_ix returning None.
Run the following to confirm no call site already crosses containers:
#!/bin/bash
# Every frame_ix mint and every verdict consumption, with enough context to pair them.
rg -nP --type=rust -C6 '\bframe_ix\s*\(' crates/engine/src
echo '--- verdict consumers ---'
rg -nP --type=rust -C6 '\.verdict\s*\(' crates/engine/src🤖 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/analysis/resource.rs` around lines 276 - 312, Bind FrameIx
to its owning container rather than representing it as a reusable bare index:
update the FrameIx definition and frame_ix to mint a branded key, then make
verdict validate ownership and return None for foreign or invalid keys instead
of indexing or computing a verdict. Update every verdict consumer to propagate
this refusal as the existing frame_ix failure path, while preserving successful
same-container memoization and verdict computation.
| let per_period = ResourceVector::delta(&snaps[frames - 1 - k], &snaps[frames - 1]); | ||
| // Smallest repeating period, so every longer one is a whole number of copies of | ||
| // this one — a zero here cannot become non-zero at a larger `k`. | ||
| if per_period == ResourceVector::default() { | ||
| return None; | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The zero-delta early return is fail-closed, but its stated justification is wrong.
The comment claims "Smallest repeating period, so every longer one is a whole number of copies of this one — a zero here cannot become non-zero at a larger k." The repetition test only inspects the most recent 2k deltas, so it does not establish that the whole ring is periodic with period k. A larger candidate k' is therefore not necessarily a multiple of k, and its per-period delta can be non-zero.
Counter-example over 8 frames (7 deltas d1..d7, oldest first). At k = 1 the last two deltas are equal and zero, so this returns None. At k' = 3 the test compares [d1, d2, d3] against [d4, d5, d6]; d5 = d6 = 0 forces d2 = d3 = 0, but d4 is unconstrained and equals d1. The k' = 3 period is then d4 + d5 + d6 = d4, which can be non-zero.
The behaviour is the safe direction — a missed offer, never a wrong one — so this is a comment defect rather than a soundness defect. Correct it, because a future reader can use the false claim to justify keeping the early return while widening the search, or to replace the search with a single-k probe.
If you want the search to continue past a zero-delta candidate instead, continue is sound here for the same reason the current return is safe: each candidate k is validated independently.
🐛 Proposed correction
- // Smallest repeating period, so every longer one is a whole number of copies of
- // this one — a zero here cannot become non-zero at a larger `k`.
+ // A cycle that moves no resource states no CR 704 threshold to bound, so it
+ // cannot supply a per-period magnitude and is refused. This is deliberately a
+ // WHOLE-SEARCH refusal, not a per-candidate one: the repetition test above
+ // inspects only the most recent `2k` deltas, so a larger `k` is NOT necessarily
+ // a multiple of this one and could carry a non-zero period. Refusing outright
+ // is the fail-closed direction (a missed offer, never a wrong one). Switch this
+ // to `continue` if that missed class ever needs to certify.
if per_period == ResourceVector::default() {
return None;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let per_period = ResourceVector::delta(&snaps[frames - 1 - k], &snaps[frames - 1]); | |
| // Smallest repeating period, so every longer one is a whole number of copies of | |
| // this one — a zero here cannot become non-zero at a larger `k`. | |
| if per_period == ResourceVector::default() { | |
| return None; | |
| } | |
| let per_period = ResourceVector::delta(&snaps[frames - 1 - k], &snaps[frames - 1]); | |
| // A cycle that moves no resource states no CR 704 threshold to bound, so it | |
| // cannot supply a per-period magnitude and is refused. This is deliberately a | |
| // WHOLE-SEARCH refusal, not a per-candidate one: the repetition test above | |
| // inspects only the most recent `2k` deltas, so a larger `k` is NOT necessarily | |
| // a multiple of this one and could carry a non-zero period. Refusing outright | |
| // is the fail-closed direction (a missed offer, never a wrong one). Switch this | |
| // to `continue` if that missed class ever needs to certify. | |
| if per_period == ResourceVector::default() { | |
| return None; | |
| } |
🤖 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/analysis/resource.rs` around lines 1281 - 1286, Correct the
explanatory comment beside the zero-delta check in the candidate-period search,
without changing the existing early return behavior. Remove the incorrect claim
that larger periods are multiples of the smallest period, and state only that
returning on a zero validated per-period delta is an intentional fail-closed
choice that may miss later candidates because each period is checked
independently.
| ResolutionChoiceFreedom::FreeUnlessReplacements(events) => { | ||
| // `events` came from the RESOLVER, never from a per-arm list, and is | ||
| // non-empty by construction — `probe_resolution` returns `Prompted` | ||
| // on an empty derivation, so `any()` can never discharge vacuously. | ||
| debug_assert!( | ||
| !events.is_empty(), | ||
| "empty derivations are MayPrompt, never FreeUnlessReplacements" | ||
| ); | ||
| !events.iter().any(|ev| { | ||
| !crate::game::replacement::proposed_event_prompt_cause( | ||
| board, | ||
| ev, | ||
| crate::game::replacement::replacement_registry(), | ||
| ) | ||
| .is_empty() | ||
| }) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
An empty events vector discharges vacuously in release builds.
FreeUnlessReplacements(events) is discharged with !events.iter().any(...), which is true for an empty vector. The non-empty invariant is held only by a debug_assert!, which is compiled out of release. If probe_resolution ever returns FreeUnlessReplacements(vec![]) — its contract says it returns Prompted instead, but that contract lives in another module and is not enforced here — the release build certifies the entry with no CR 616.1 check at all.
This is the fail-open direction, and it is the one direction this predicate exists to prevent. Make the emptiness check load-bearing in every build; keep the debug_assert! so a violation is still loud in tests.
🛡️ Proposed fix
debug_assert!(
!events.is_empty(),
"empty derivations are MayPrompt, never FreeUnlessReplacements"
);
- !events.iter().any(|ev| {
+ // Fail-closed in EVERY build: an empty derivation would make the `any()`
+ // below vacuously discharge, which is the one direction this predicate
+ // exists to prevent. The `debug_assert!` keeps the contract violation
+ // loud in tests; this keeps it safe in release.
+ !events.is_empty()
+ && !events.iter().any(|ev| {
!crate::game::replacement::proposed_event_prompt_cause(
board,
ev,
crate::game::replacement::replacement_registry(),
)
.is_empty()
})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| ResolutionChoiceFreedom::FreeUnlessReplacements(events) => { | |
| // `events` came from the RESOLVER, never from a per-arm list, and is | |
| // non-empty by construction — `probe_resolution` returns `Prompted` | |
| // on an empty derivation, so `any()` can never discharge vacuously. | |
| debug_assert!( | |
| !events.is_empty(), | |
| "empty derivations are MayPrompt, never FreeUnlessReplacements" | |
| ); | |
| !events.iter().any(|ev| { | |
| !crate::game::replacement::proposed_event_prompt_cause( | |
| board, | |
| ev, | |
| crate::game::replacement::replacement_registry(), | |
| ) | |
| .is_empty() | |
| }) | |
| } | |
| ResolutionChoiceFreedom::FreeUnlessReplacements(events) => { | |
| // `events` came from the RESOLVER, never from a per-arm list, and is | |
| // non-empty by construction — `probe_resolution` returns `Prompted` | |
| // on an empty derivation, so `any()` can never discharge vacuously. | |
| debug_assert!( | |
| !events.is_empty(), | |
| "empty derivations are MayPrompt, never FreeUnlessReplacements" | |
| ); | |
| // Fail-closed in EVERY build: an empty derivation would make the `any()` | |
| // below vacuously discharge, which is the one direction this predicate | |
| // exists to prevent. The `debug_assert!` keeps the contract violation | |
| // loud in tests; this keeps it safe in release. | |
| !events.is_empty() | |
| && !events.iter().any(|ev| { | |
| !crate::game::replacement::proposed_event_prompt_cause( | |
| board, | |
| ev, | |
| crate::game::replacement::replacement_registry(), | |
| ) | |
| .is_empty() | |
| }) | |
| } |
🤖 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/analysis/resource.rs` around lines 2099 - 2115, Make the
empty-input guard in the ResolutionChoiceFreedom::FreeUnlessReplacements branch
load-bearing in release builds by returning false when events is empty, while
retaining the existing debug_assert! for diagnostics. Ensure the any-based
replacement check runs only for non-empty events.
|
Generated for head Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — several serialization, prompt-state, analysis, and tooling paths remain unsafe on the current head.
Critical — loop proposal JSON cannot round-trip
loop_check.rs:160-187 derives serde for ShortcutProposal with per_cycle: PeriodicDelta, while resource.rs:481-502 and :594-621 retain BTreeMap<PlayerId, ...> values without serialization adapters. The proposal therefore cannot reliably survive a JSON round trip. Add the appropriate adapters or a transport representation and cover an actual JSON round trip.
Critical — speculative resolution can miss a re-parked prompt
resolution_prompt.rs:109-123 compares only WaitingFor discriminants after speculative resolution. A newly re-parked prompt with the same variant is therefore treated as unchanged. Compare the meaningful prompt state/identity instead, with a regression for a same-variant re-park.
Medium — trigger firing evidence reads the wrong field
scripts/lib/trigger-firing.jq:34-42 reads .description, but TriggerEntry owns the description under its nested definition (ability.rs:20703-20705). Use the definition description and add a fixture proving the firing evidence remains populated.
Medium — fixture migration is non-atomic and corrupts pass-through fixtures
migrate-dump-fixture.sh:124-140 writes directly to its destination before jq succeeds, and wraps a non-gameState fixture as {gameState:.gameState} despite its stated pass-through behavior. Write atomically only after jq succeeds and preserve non-gameState inputs unchanged; add fixtures for both failure and pass-through cases.
Medium — bounded choices still expose UntilLethal
candidates.rs:3238-3264 always offers UntilLethal even when the action is bounded, then additionally supplies Fixed. Do not expose the unbounded choice for a bounded action; represent only legal quantity choices and add a bounded-action regression.
Medium — empty resolution-event proof fails open
resource.rs:2092-2115 uses a debug assertion to claim FreeUnlessReplacements events are nonempty, then uses !events.iter().any(...). In release an empty vector returns true and incorrectly discharges the proof. Make empty events fail closed and add coverage.
Recommendation: address these substantive runtime and data-contract defects, restore the canceled/failing Rust verification, and then resubmit.
lgray
changed the title
…proofs Addresses maintainer Critical 1 and Medium 6 on phase-rs#6886, plus CodeRabbit 3699361027 / 3699361052 / 3699361032, all in `analysis/resource.rs`. 1. CRITICAL — `ShortcutProposal.per_cycle` could not survive the PRODUCTION persistence path. Adds a serde adaptor riding the four `PlayerId`-keyed `ResourceVector` maps as pair SEQUENCES, generalizing the existing `counter_key_pairs` into `map_key_pairs` so one definition covers the tuple key and the player keys. MEASURED MECHANISM (the old doc comment asserted the opposite, and was wrong): a bare `BTreeMap<PlayerId, i64>` is fine through `from_str` and through `from_value` IN ISOLATION — which is why the existing `periodic_delta_survives_the_serde_json_wire` arm passed and gave false confidence. It breaks only under the ENCLOSING shape: `WaitingFor` is `#[serde(tag, content)]`, so its payload is buffered through serde's `Content`, which stringifies map keys, and `PlayerId` is `#[serde(transparent)]` over `u8`. `PersistedGameState::deserialize` routes EVERY decode through `serde_json::Value` + `from_value`, including the WASM restore at `engine-wasm/src/lib.rs`'s `from_str::<PersistedGameState>` — so `from_str` at the boundary does not save it. Measured on serde_json 1.0.149; the failure text is `invalid type: string "0", expected u8`, exactly what `tests/integration/loop_shortcut.rs` had already recorded as a standing limitation. `generic_triggers` keeps its bare map: `TriggerKind` is a unit-variant enum, measured Ok through the same path. NEW ROW `a_populated_per_cycle_proposal_survives_the_production_persistence_ boundary` drives `from_value`, the `PersistedGameState` boundary, and the WASM bridge's own `from_str::<PersistedGameState>`, with all four `PlayerId`-keyed maps populated behind a reach-guard. REVERT-PROBE, run: dropping `#[serde(with = "map_key_pairs")]` from `life` ⇒ FAILS with that exact error text; restored byte-identical. 2. MEDIUM — an EMPTY `FreeUnlessReplacements` derivation discharged the CR 616.1 obligation vacuously. `!events.iter().any(..)` is `true` for an empty slice, and the only thing preventing it was a `debug_assert!`, which compiles out of release — so the fail-open case was live in the build that ships. Now a first-class refusal in every build. A refusal rather than a panic on purpose: it matches every other seam in the module, and a `debug_assert!` could not be covered at all, since it aborts the build tests run in. REVERT-PROBE, run: deleting the arm ⇒ the empty case flips to `true` and the new row FAILS while the non-empty arms stay green; restored byte-identical. 3. Comment defect (CodeRabbit 3699361032): the zero-delta early return claimed "every longer period is a whole number of copies of this one". The repetition test inspects only the most recent `2k` deltas, so a larger `k'` need not be a multiple of `k`. Behaviour is unchanged and still fail-closed; the false justification is corrected in place, with the counter-example, because it is the kind of claim a later reader would lean on to widen the search while keeping the early return. Assisted-by: ClaudeCode:claude-opus-5
Maintainer Critical 2 on phase-rs#6886, and CodeRabbit 3699361064. `probe_resolution` compared only the `WaitingFor` DISCRIMINANT of the probed clone against the incoming board. When the incoming board already carries a non-priority variant, a resolution that re-parks the SAME variant leaves the two discriminants equal, so the probe reported the resolution CHOICE-FREE while an unanswered choice sat on the board. That is fail-open in the one direction this function exists to close, and no comparison against the incoming variant can see it — the incoming variant is exactly what masks it. Now keyed on "is there a prompt at all": a non-`Priority` `waiting_for` on the probed board is itself a refusal. The incoming board is a RESOLUTION BOARD by this function's own documented contract, so a standing prompt is a reason to refuse rather than a baseline to compare against. The discriminant test is kept alongside it, so the guard is STRICTLY STRONGER on every input than the struck form: it can only ever cost coverage (a missed offer), never soundness — the same direction as the budget-exceeded and empty-derivation arms beside it. NEW ROW `a_prompt_standing_on_the_incoming_board_refuses_the_probe`, a MATCHED PAIR over all six allow-listed arms: each arm must still reach `Events` from a priority board (positive control — without it a probe that refused everything would pass), and must return `Prompted` when the same board carries a standing `ReplacementChoice`. The row asserts the resolution does NOT clear that prompt, so the discriminants really are equal and the struck guard could not have caught it. REVERT-PROBE, run: restoring the bare discriminant comparison ⇒ the negative arm FLIPS TO FAIL while the positive control stays green; restored byte-identical. Assisted-by: ClaudeCode:claude-opus-5
Maintainer Medium 5 on phase-rs#6886, and CodeRabbit 3699361023 / 3699361081. The candidate generator emitted `IterationCount::UntilLethal` unconditionally for every `WaitingFor::LoopShortcut` node, including bounded offers. `handle_declare_shortcut` rejects that combination outright (`IterationCount::UntilLethal if offer.schema.is_bounded()` => `reject_shortcut_declaration`, `crates/engine/src/game/engine.rs`), and that reject is a SUCCESSFUL fail-closed handback — `Ok(result)`, not an `Err`. So the candidate was not merely a wasted search node: the simulation layer was handed an action the engine accepts and then silently discards, i.e. an illegal quantity choice wearing the shape of a legal one, which the policy layer then had to know to score away. `UntilLethal` is now gated on `!schema.is_bounded()`. A bounded offer still gets `Fixed(max_iterations)` where its pin set permits a `template: None` declaration; where neither applies, `DeclineShortcut` genuinely is the only legal answer at the node, and representing that honestly is the point. Paired AI-side guard: `LoopShortcutPolicy`'s final bounded arm matched every remaining `Fixed(n)` INCLUDING `n == 0`, so a zero-count declaration — legal and representable, per `a_zero_count_declaration_validates_over_an_empty_range_but_still_checks_ cardinality` — would have been scored into the CRITICAL band. It commits no cycles while spending the CR 732.2b response window, the same weak-domination shape the over-bound and `(None, UntilLethal)` arms already reject. Unreachable from today's generator, so no current ranking moves; the arm now states its own precondition instead of relying on an invariant maintained a crate away. Uses `PolicyVerdict::reject`, not a raw sentinel. R8 OFFER-WRITER CENSUS: unaffected, and checked rather than assumed. The census counts occurrences of the `WaitingFor::LoopShortcut {` token; this change edits the BODY of an existing match arm and adds or removes no such token, so the pinned (22, 14) pair and the per-file production multiset are untouched. Assisted-by: ClaudeCode:claude-opus-5
…g carriers Maintainer Medium 3 on phase-rs#6886, and CodeRabbit 3699361085 / 3699361087 plus the non-object `command` nitpick. Also folds in the promised doc correction. 1. `_defs` read `.description` across BOTH definition lists, but they do not serialize alike. `trigger_definitions` is `Definitions<TriggerEntry>` and `TriggerEntry` is `{occurrence, definition}`, so its text is at `.definition.description`; only `base_trigger_definitions` (`Vec<TriggerDefinition>`) exposes `.description` directly. MEASURED on the committed corpus, which is what the maintainer asked for and what the earlier "all 172 carriers matched non-null" claim did not establish: of the `trigger_definitions` entries, ZERO expose a direct `.description` and 100% nest it (145 / 165 / 132 on dellian / dina / witherbloom). The live list therefore contributed NOTHING — every entry collapsed to the `// ""` fallback — and all 172 carriers resolved through `base_trigger_definitions` alone. Total descriptions visible to the derivation across the corpus: 875 before, 1755 after. REACHABLE, not theoretical: `dellian_emblem_conqueror_4p` carries a GRANTED trigger ("When ~ dies, you gain 1 life.") present in the live list and absent from the base list. A firing whose description existed only there would have aborted the whole stamp on a classifiable fixture. BEHAVIOUR-PRESERVING on the corpus: 172 carriers resolve before AND after, all `Ordinary`; the pristine regeneration stays BYTE_IDENTICAL=true. 2. `stamp_trigger_firing` assigned all three carrier keys unconditionally without reading them, so `stamp-fixture-firing.sh`'s header claim that in-place stamping "cannot revert anything" was false for exactly those keys — and arm 1 structurally cannot catch it, because it deletes them from both sides before comparing. An already-canonical `Delayed` carrier would either abort the stamp or be silently rewritten to `Ordinary`, the CR 603.7a to CR 603.1 re-classification this library exists to refuse. Now derives only into an ABSENT slot, which also makes the stamp idempotent. Preservation is scoped to stack entries that are still on the stack, so a stale key cannot accumulate and inflate the carrier total past the number of triggered records — the one shape that could have let arm 2's aggregate comparison cancel a surplus against a deficit. 3. `select(.command.DelayedTriggerInstall)` indexes `.command` with a key, which aborts jq with a raw type error on a serde unit variant (a bare JSON string). This file's contract is that undetermined cases abort BY NAME; filtered to objects first so the probe stays total. TWO NEW PRE-FLIGHT CONTROL ARMS, both with negative controls, because no fixture-level arm can witness either property: arm 4 DEFINITION_SHAPES — `_defs` resolves a nested-only description AND a direct-only one, and still ABORTS on one present in neither. arm 5 CARRIER_PRESERVED — an existing canonical carrier survives, while an absent one is still derived. REVERT-PROBE, run: restoring the old `_defs` ⇒ arm 4 reports `nested=FAILED` and the script refuses to stamp (rc=1), while `direct` stays green and the negative control still aborts; restored byte-identical. DOC: the header said the stamped artifact minus "the three new keys"; the executable `del()` names FIVE (three carriers + two allocators). Corrected, as promised in the PR body. Assisted-by: ClaudeCode:claude-opus-5
Maintainer Medium 4 on phase-rs#6886, and CodeRabbit 3699361092 (Critical) / 3699361088 / 3699361096. 1. NON-ATOMIC WRITE, and the destination is the committed fixture. `regenerate` redirected the pipeline straight into `$dest`; the shell creates and TRUNCATES a redirection target before the first command in the pipeline runs, and the production path passes `$OUT`. This recipe aborts BY DESIGN — `_firing` raises `UNDETERMINED firing carrier`, `stamp_delayed_allocators` raises `UNDETERMINED delayed-trigger allocators` — so `set -e` / `pipefail` stopped the script only AFTER the fixture had been truncated and a partial gzip stream written over it. The failure mode of a fail-closed recipe was destruction of the artifact it was refusing to rewrite. Now stages to `mktemp` and `mv`s only on success, matching `stamp-fixture-firing.sh`. 2. PASS-THROUGH CORRUPTION. The final `{gameState:.gameState}` is a REWRITE, not a projection, for a dump that is not `gameState`-shaped: `.gameState` is null on those, so the document became `{"gameState":null}` — silently, since that is valid JSON. Several fixtures in this corpus really do use the other envelope (top level `turn_number`), which is why `lib/trigger-firing.jq` already passes them through. Keyed on `has("gameState")` rather than truthiness, so an explicitly-null `gameState` is not quietly normalised into the husk shape. Preserves the input unchanged, per the maintainer's disposition. 3. ARM 2 HAD GONE VACUOUS — it was reporting the opposite of its claim. It compared the patched and unpatched regenerations wholesale and required a difference, and inferred from that difference that the `effect_kind` filter had teeth. Stage 2b broke the inference: the patched filter also runs `stamp_trigger_firing` and `stamp_delayed_allocators`, and the allocator stage rewrites the two allocator keys on EVERY `gameState`-shaped dump in this corpus (measured: all six move to 1), while the unpatched filter runs neither. So the documents differed unconditionally, including on the dumps that carry no target prompt at all, and arm 2 printed `PATCHED_DIFFERS=true` while `effect_kind` had matched nothing. Now compares the `target_slots` projection specifically, and NAMES the no-prompt case instead of counting it as a pass. New arm 3 `STAGE_2B_LANDED` covers what arm 2 no longer claims. TWO PRE-FLIGHT SELF-TESTS, on synthetic inputs through the SAME `transform` the migration uses, because neither property can be witnessed by a corpus fixture — both are about inputs this corpus does not contain. Each has a paired positive control: ATOMIC_ON_FAILURE — a real abort leaves the destination byte-identical to a sentinel (and a SUCCEEDING run must still replace it, or the row would pass because nothing ever writes). ENVELOPE_PRESERVED — a `turn_number` dump survives unchanged (and a `gameState` dump must still be projected). VERIFIED NEUTRAL against the pre-change script, run side by side: dellian reproduces `BYTE_IDENTICAL=true` and the same `PATCHED_DIFFERS=true stamped=["LoseLife"]`. dina's `BYTE_IDENTICAL=false` is PRE-EXISTING and reproduces identically under the original script — it is the documented later-parser-state divergence, not a regression from this change. Assisted-by: ClaudeCode:claude-opus-5
dd986e4 to
34f5823
Compare
|
🤖 AI text below 🤖 Thanks — all six findings are addressed, each with a discriminating test whose revert-probe was The failing Rust verificationThe census row Findings → commits
Plus
Trigger-firing evidence. Confirmed on the committed fixtures before changing anything: of the On the fixture control you asked for: I did not add one, and I want to be straight about why rather Bounded Fixture migration. Writes now stage to a temp file and Empty events. Made a first-class refusal in every build rather than a louder assert — a Two CI jobs I could not fix, with the measurementThe Paired-seed AI gate and Decision-cost perf gate were cancelled, and this is CodeRabbitThirteen of the seventeen inline comments are fixed in the commits above (several coincide with Four are declined as follow-ups rather than landed in this round, each with its measurement in the One note on the base: this is rebased onto Local gate, run in full at
The tip Every commit in the range also compiles on its own ( Re-requesting review. |
There was a problem hiding this comment.
Actionable comments posted: 7
🧹 Nitpick comments (2)
crates/engine/src/game/resolution_prompt.rs (1)
53-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMake the
joinmatch wildcard-free, like the rest of this module.Line 62 uses
_. The rest of this file enforces classification at compile time (the wildcard-freeEffectmatch, the..-freeResolvedAbilitydestructure). A future thirdResolutionChoiceFreedomvariant would silently collapse intoMayPrompthere instead of failing to compile. The fallback direction is fail-closed, so this is a compile-time-enforcement gap, not a soundness gap.♻️ Proposed refactor
- _ => ResolutionChoiceFreedom::MayPrompt, + (ResolutionChoiceFreedom::MayPrompt, _) + | (_, ResolutionChoiceFreedom::MayPrompt) => ResolutionChoiceFreedom::MayPrompt,🤖 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/resolution_prompt.rs` around lines 53 - 64, Update ResolutionChoiceFreedom::join to explicitly match every remaining pair of variants instead of using the wildcard arm, returning MayPrompt for each non-FreeUnlessReplacements combination. Preserve the existing merge behavior for two FreeUnlessReplacements values while ensuring any future variant causes a compile-time exhaustiveness error.Source: Coding guidelines
crates/phase-ai/src/policies/loop_shortcut.rs (1)
525-560: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a row for the
Fixed(0)reject arm.The new zero-count arm at line 246 has no test. Deleting that arm flips nothing in this module:
loop_shortcut_bounded_declare_scores_and_rejects_over_boundasserts onlyFixed(4)andFixed(11), andloop_shortcut_unbounded_offer_keeps_fixed_neutralasserts onlyFixed(4). The arm therefore ships without a revert probe.Assert the reject kind on the bounded schema. If you also apply the arm reorder proposed above, assert the same kind on the unbounded schema in the same row, so the two shapes are pinned together.
💚 Proposed row
/// CR 732.2a — a zero-repetition declaration commits nothing while spending the /// CR 732.2b response window, so it is weakly dominated by declining. /// /// REVERT-PROBE: delete the `(_, IterationCount::Fixed(0))` arm ⇒ the bounded case takes /// the scoring arm and returns the CRITICAL band for a guaranteed no-op ⇒ this row FAILS. #[test] fn loop_shortcut_zero_count_declare_is_rejected() { let state = bounded_offer_state(10); let v = verdict_for(&state, &declare(IterationCount::Fixed(0))); assert!( matches!(v, PolicyVerdict::Reject { .. }), "a zero-count declaration commits no cycles and must be vetoed, got {v:?}" ); assert_eq!(kind_of(&v), "loop_shortcut_bounded_declare_zero_count"); }🤖 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/phase-ai/src/policies/loop_shortcut.rs` around lines 525 - 560, Add a regression test near loop_shortcut_bounded_declare_scores_and_rejects_over_bound named loop_shortcut_zero_count_declare_is_rejected. On a bounded_offer_state, evaluate declare(IterationCount::Fixed(0)), assert it returns PolicyVerdict::Reject, and verify kind_of returns loop_shortcut_bounded_declare_zero_count; if the related arm reorder is present, add the corresponding unbounded-schema assertion in the same test row.Source: Path instructions
🤖 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.
Inline comments:
In `@crates/engine/src/game/resolution_prompt.rs`:
- Line 562: The quantity prompt resolver must cover both currently unguarded
positions: in the ability-level handling around repeat_for, replace the
unconditional ignored binding with the quantity_offers_up_to_choice gate used by
the other count/amount fields, and in the Power arm recurse through base as well
as exponent, matching the Difference and Sum/Max handling. Apply these changes
at crates/engine/src/game/resolution_prompt.rs:562-562 and
crates/engine/src/game/resolution_prompt.rs:196-199.
- Around line 1279-1288: Update the attribution (i) assertion in the relevant
resolution test to cover both production guard legs: verify that probe_board()
starts with a non-Priority waiting_for or that the waiting_for discriminants
differ, matching the guard’s OR condition. Keep the existing assertion message
and surrounding event-accounting checks unchanged.
- Around line 1540-1560: Add an `optional_for` mutation entry to the mutation
table alongside `optional` and `optional_targeting`, assigning a value matching
the field’s actual type so the corresponding gate is exercised.
- Around line 371-374: Update the CR citation in the Effect::ChoosePermanent
match arm to CR 707.6, while preserving the existing explanation and behavior.
In `@crates/phase-ai/src/policies/loop_shortcut.rs`:
- Line 218: Reorder the `IterationCount::Fixed` match arms in the loop-shortcut
policy so the `Fixed(0)` rejection is evaluated before the
`!schema.is_bounded()` neutral result, covering unbounded offers as well. Remove
the later duplicate `Fixed(0)` arm while preserving the existing behavior for
nonzero fixed counts and other iteration variants.
In `@scripts/lib/trigger-firing.jq`:
- Around line 156-174: The stack carrier update in
scripts/lib/trigger-firing.jq:156-174 must compare $sf_existing + $sf with the
stored stack_trigger_firings map and assign the result whenever they differ,
including when the rebuilt map is empty, so stale departed-entry keys are
removed. Extend carrier_preservation_control in
scripts/stamp-fixture-firing.sh:121-145 with stack and resolving cases, plus a
stale-key case that verifies absent stack entries are dropped.
In `@scripts/migrate-dump-fixture.sh`:
- Around line 170-180: Update regenerate() to create the staged temporary file
in the destination directory rather than via system TMPDIR, ensuring the final
mv remains an atomic same-filesystem rename. Preserve the existing cleanup on
transformation failure and remove any staged file if temporary-file creation or
subsequent processing fails.
---
Nitpick comments:
In `@crates/engine/src/game/resolution_prompt.rs`:
- Around line 53-64: Update ResolutionChoiceFreedom::join to explicitly match
every remaining pair of variants instead of using the wildcard arm, returning
MayPrompt for each non-FreeUnlessReplacements combination. Preserve the existing
merge behavior for two FreeUnlessReplacements values while ensuring any future
variant causes a compile-time exhaustiveness error.
In `@crates/phase-ai/src/policies/loop_shortcut.rs`:
- Around line 525-560: Add a regression test near
loop_shortcut_bounded_declare_scores_and_rejects_over_bound named
loop_shortcut_zero_count_declare_is_rejected. On a bounded_offer_state, evaluate
declare(IterationCount::Fixed(0)), assert it returns PolicyVerdict::Reject, and
verify kind_of returns loop_shortcut_bounded_declare_zero_count; if the related
arm reorder is present, add the corresponding unbounded-schema assertion in the
same test row.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: ba672bf6-c69d-422b-8310-87fd2cfd21c5
⛔ Files ignored due to path filters (6)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (48)
crates/engine/src/ai_support/candidates.rscrates/engine/src/analysis/corpus_tests.rscrates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/effects/choose.rscrates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/clash.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/proliferate.rscrates/engine/src/game/effects/separate_piles.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine.rscrates/engine/src/game/filter.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mod.rscrates/engine/src/game/phasing.rscrates/engine/src/game/players.rscrates/engine/src/game/replacement.rscrates/engine/src/game/resolution_prompt.rscrates/engine/src/game/sba.rscrates/engine/src/game/stack.rscrates/engine/src/game/targeting.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/tests/integration/fantastic_four_bounded_loop.rscrates/engine/tests/integration/gift_recipient_phased_out_opponent.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.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/loop_shortcut_offer_writer_census.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/rules/battle.rscrates/engine/tests/integration/sprout_inalla_realistic_offer.rscrates/phase-ai/src/policies/loop_shortcut.rscrates/phase-ai/src/projection.rscrates/phase-ai/src/search.rsscripts/lib/trigger-firing.jqscripts/migrate-dump-fixture.shscripts/stamp-fixture-firing.sh
🚧 Files skipped from review as they are similar to previous changes (40)
- crates/engine/tests/integration/main.rs
- crates/engine/src/game/effects/proliferate.rs
- crates/phase-ai/src/search.rs
- crates/engine/src/game/casting.rs
- crates/engine/src/game/effects/clash.rs
- crates/engine/tests/integration/gift_recipient_phased_out_opponent.rs
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- crates/engine/tests/integration/sprout_inalla_realistic_offer.rs
- crates/engine/src/analysis/corpus_tests.rs
- crates/engine/src/game/ability_utils.rs
- crates/phase-ai/src/projection.rs
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/effects/choose.rs
- crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs
- crates/engine/src/game/casting_tests.rs
- crates/engine/src/types/mod.rs
- crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs
- crates/engine/src/game/phasing.rs
- crates/engine/src/analysis/loop_check.rs
- crates/engine/src/game/effects/token.rs
- crates/engine/src/game/mod.rs
- crates/engine/src/game/players.rs
- crates/engine/src/game/interaction.rs
- crates/engine/src/game/effects/separate_piles.rs
- crates/engine/src/game/effects/choose_from_zone.rs
- crates/engine/src/game/filter.rs
- crates/engine/tests/integration/rules/battle.rs
- crates/engine/src/game/replacement.rs
- crates/engine/tests/integration/interaction_contract.rs
- crates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/sba.rs
- crates/engine/src/game/targeting.rs
- crates/engine/src/analysis/decision_template.rs
- crates/engine/src/game/stack.rs
- crates/engine/src/game/casting_costs.rs
- crates/engine/src/game/zone_pipeline.rs
- crates/engine/tests/integration/loop_shortcut.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/analysis/resource.rs
| // The engine-side regression row that keeps this honest is | ||
| // `bounded_fixed_count_commits_exactly_n_periods`, which asserts zero eliminations | ||
| // and `committed == n × published δ` on all three fixtures. | ||
| (_, IterationCount::Fixed(_)) if !schema.is_bounded() => na(), |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
The zero-count guard does not cover an unbounded offer.
Arm ordering places !schema.is_bounded() => na() before the Fixed(0) reject. A Fixed(0) declaration against an unbounded offer therefore returns neutral instead of reject.
The stated justification for the zero arm — "it commits NO cycles while still spending the CR 732.2b response window" — does not depend on the bound. It holds for the unbounded offer as well. Neutral is not harmless at this seam: in the heuristic branch the class-bonus table ranks DeclareShortcut (0.5) above DeclineShortcut (0.4), as this file documents at line 594, so a neutral verdict lets the AI pick the no-op declaration.
The branch is unreachable from today's engine generator, which emits Fixed only for bounded schemas. Rate it by what happens when it is reached.
Move the zero arm above the unbounded arm so the precondition is stated once for the whole Fixed family.
🛡️ Proposed reorder of the `Fixed` arms
+ // CR 732.2a: a ZERO-repetition declaration commits no cycles while still spending
+ // the CR 732.2b response window, on a bounded offer and on an unbounded one alike.
+ // Stated BEFORE the unbounded branch so the precondition covers the whole `Fixed`
+ // family rather than one half of it.
+ (_, IterationCount::Fixed(0)) => PolicyVerdict::reject(PolicyReason::new(
+ "loop_shortcut_bounded_declare_zero_count",
+ )),
+
(_, IterationCount::Fixed(_)) if !schema.is_bounded() => na(),Then delete the later Fixed(0) arm.
Also applies to: 233-248
🤖 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/phase-ai/src/policies/loop_shortcut.rs` at line 218, Reorder the
`IterationCount::Fixed` match arms in the loop-shortcut policy so the `Fixed(0)`
rejection is evaluated before the `!schema.is_bounded()` neutral result,
covering unbounded offers as well. Remove the later duplicate `Fixed(0)` arm
while preserving the existing behavior for nonzero fixed counts and other
iteration variants.
Source: Path instructions
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the current head still has fail-open classifier coverage and unbounded probe-cost gaps.
[HIGH] UpTo resolution choices can still be certified as choice-free through repeat_for. Evidence: crates/engine/src/game/resolution_prompt.rs:562 deliberately ignores repeat_for: Option<QuantityExpr>, while quantity_offers_up_to_choice only guards the other quantity positions (:181-202); QuantityExpr::UpTo is a resolution-time player choice and generic quantity resolution transparently takes its maximum (crates/engine/src/types/ability.rs:7027-7050). Why it matters: an allow-listed repeated ability with an UpTo repeat count can be probed as free and admitted to a loop certificate despite a CR 608.2d choice. Suggested fix: route repeat_for.as_ref().is_some_and(quantity_offers_up_to_choice) to MayPrompt and add a reach-guarded regression; add the missing optional_for mutation row in the same classifier test.
[MED] The probe budget does not bound the dominant work, and chain traversal re-resolves subchains. Evidence: crates/engine/src/analysis/resource.rs:4237-4252 clones the full GameState before the classifier can charge its budget; crates/engine/src/game/resolution_prompt.rs:613-624 probes the full ResolvedAbility and then recursively probes its sub_ability/else_ability, although the full resolver already resolves those branches. Why it matters: the loop detector can perform repeated whole-board clones/resolutions per frame-entry and exhaust its logical budget after paying the work, producing unbounded hot-path cost and avoidable coverage loss. Suggested fix: make the budget (or an equally cheap binding precondition) precede cloning and probe a chain root once, retaining recursion only for ability-level choice gates.
[MED] Fixture migration can retain stale trigger carriers and its advertised atomic write is not guaranteed atomic. Evidence: scripts/lib/trigger-firing.jq:170-173 assigns stack_trigger_firings only when new carriers exist, so a pruned $sf_existing is not written when $sf is empty; scripts/migrate-dump-fixture.sh:176-185 stages with mktemp -t, which may be on a different filesystem from $dest before mv. Why it matters: persisted fixtures can retain departed stack-entry metadata, and interruption during a cross-filesystem move can corrupt the destination. Suggested fix: assign whenever the rebuilt map differs (including {}), cover stale stack/resolving cases, and create/clean the stage file in dirname "$dest".
The required Rust aggregate check is still failed because both Rust test shards are cancelled, and the branch is behind current main; this review does not treat the author’s prior local run as a replacement for a current required-check success. Please address or substantively refute the findings, rebase as appropriate, and rerun the current-head review and required checks.
…proofs Addresses maintainer Critical 1 and Medium 6 on phase-rs#6886, plus CodeRabbit 3699361027 / 3699361052 / 3699361032, all in `analysis/resource.rs`. 1. CRITICAL — `ShortcutProposal.per_cycle` could not survive the PRODUCTION persistence path. Adds a serde adaptor riding the four `PlayerId`-keyed `ResourceVector` maps as pair SEQUENCES, generalizing the existing `counter_key_pairs` into `map_key_pairs` so one definition covers the tuple key and the player keys. MEASURED MECHANISM (the old doc comment asserted the opposite, and was wrong): a bare `BTreeMap<PlayerId, i64>` is fine through `from_str` and through `from_value` IN ISOLATION — which is why the existing `periodic_delta_survives_the_serde_json_wire` arm passed and gave false confidence. It breaks only under the ENCLOSING shape: `WaitingFor` is `#[serde(tag, content)]`, so its payload is buffered through serde's `Content`, which stringifies map keys, and `PlayerId` is `#[serde(transparent)]` over `u8`. `PersistedGameState::deserialize` routes EVERY decode through `serde_json::Value` + `from_value`, including the WASM restore at `engine-wasm/src/lib.rs`'s `from_str::<PersistedGameState>` — so `from_str` at the boundary does not save it. Measured on serde_json 1.0.149; the failure text is `invalid type: string "0", expected u8`, exactly what `tests/integration/loop_shortcut.rs` had already recorded as a standing limitation. `generic_triggers` keeps its bare map: `TriggerKind` is a unit-variant enum, measured Ok through the same path. NEW ROW `a_populated_per_cycle_proposal_survives_the_production_persistence_ boundary` drives `from_value`, the `PersistedGameState` boundary, and the WASM bridge's own `from_str::<PersistedGameState>`, with all four `PlayerId`-keyed maps populated behind a reach-guard. REVERT-PROBE, run: dropping `#[serde(with = "map_key_pairs")]` from `life` ⇒ FAILS with that exact error text; restored byte-identical. 2. MEDIUM — an EMPTY `FreeUnlessReplacements` derivation discharged the CR 616.1 obligation vacuously. `!events.iter().any(..)` is `true` for an empty slice, and the only thing preventing it was a `debug_assert!`, which compiles out of release — so the fail-open case was live in the build that ships. Now a first-class refusal in every build. A refusal rather than a panic on purpose: it matches every other seam in the module, and a `debug_assert!` could not be covered at all, since it aborts the build tests run in. REVERT-PROBE, run: deleting the arm ⇒ the empty case flips to `true` and the new row FAILS while the non-empty arms stay green; restored byte-identical. 3. Comment defect (CodeRabbit 3699361032): the zero-delta early return claimed "every longer period is a whole number of copies of this one". The repetition test inspects only the most recent `2k` deltas, so a larger `k'` need not be a multiple of `k`. Behaviour is unchanged and still fail-closed; the false justification is corrected in place, with the counter-example, because it is the kind of claim a later reader would lean on to widen the search while keeping the early return. Assisted-by: ClaudeCode:claude-opus-5
Maintainer Critical 2 on phase-rs#6886, and CodeRabbit 3699361064. `probe_resolution` compared only the `WaitingFor` DISCRIMINANT of the probed clone against the incoming board. When the incoming board already carries a non-priority variant, a resolution that re-parks the SAME variant leaves the two discriminants equal, so the probe reported the resolution CHOICE-FREE while an unanswered choice sat on the board. That is fail-open in the one direction this function exists to close, and no comparison against the incoming variant can see it — the incoming variant is exactly what masks it. Now keyed on "is there a prompt at all": a non-`Priority` `waiting_for` on the probed board is itself a refusal. The incoming board is a RESOLUTION BOARD by this function's own documented contract, so a standing prompt is a reason to refuse rather than a baseline to compare against. The discriminant test is kept alongside it, so the guard is STRICTLY STRONGER on every input than the struck form: it can only ever cost coverage (a missed offer), never soundness — the same direction as the budget-exceeded and empty-derivation arms beside it. NEW ROW `a_prompt_standing_on_the_incoming_board_refuses_the_probe`, a MATCHED PAIR over all six allow-listed arms: each arm must still reach `Events` from a priority board (positive control — without it a probe that refused everything would pass), and must return `Prompted` when the same board carries a standing `ReplacementChoice`. The row asserts the resolution does NOT clear that prompt, so the discriminants really are equal and the struck guard could not have caught it. REVERT-PROBE, run: restoring the bare discriminant comparison ⇒ the negative arm FLIPS TO FAIL while the positive control stays green; restored byte-identical. Assisted-by: ClaudeCode:claude-opus-5
Maintainer Medium 5 on phase-rs#6886, and CodeRabbit 3699361023 / 3699361081. The candidate generator emitted `IterationCount::UntilLethal` unconditionally for every `WaitingFor::LoopShortcut` node, including bounded offers. `handle_declare_shortcut` rejects that combination outright (`IterationCount::UntilLethal if offer.schema.is_bounded()` => `reject_shortcut_declaration`, `crates/engine/src/game/engine.rs`), and that reject is a SUCCESSFUL fail-closed handback — `Ok(result)`, not an `Err`. So the candidate was not merely a wasted search node: the simulation layer was handed an action the engine accepts and then silently discards, i.e. an illegal quantity choice wearing the shape of a legal one, which the policy layer then had to know to score away. `UntilLethal` is now gated on `!schema.is_bounded()`. A bounded offer still gets `Fixed(max_iterations)` where its pin set permits a `template: None` declaration; where neither applies, `DeclineShortcut` genuinely is the only legal answer at the node, and representing that honestly is the point. Paired AI-side guard: `LoopShortcutPolicy`'s final bounded arm matched every remaining `Fixed(n)` INCLUDING `n == 0`, so a zero-count declaration — legal and representable, per `a_zero_count_declaration_validates_over_an_empty_range_but_still_checks_ cardinality` — would have been scored into the CRITICAL band. It commits no cycles while spending the CR 732.2b response window, the same weak-domination shape the over-bound and `(None, UntilLethal)` arms already reject. Unreachable from today's generator, so no current ranking moves; the arm now states its own precondition instead of relying on an invariant maintained a crate away. Uses `PolicyVerdict::reject`, not a raw sentinel. R8 OFFER-WRITER CENSUS: unaffected, and checked rather than assumed. The census counts occurrences of the `WaitingFor::LoopShortcut {` token; this change edits the BODY of an existing match arm and adds or removes no such token, so the pinned (22, 14) pair and the per-file production multiset are untouched. Assisted-by: ClaudeCode:claude-opus-5
…g carriers Maintainer Medium 3 on phase-rs#6886, and CodeRabbit 3699361085 / 3699361087 plus the non-object `command` nitpick. Also folds in the promised doc correction. 1. `_defs` read `.description` across BOTH definition lists, but they do not serialize alike. `trigger_definitions` is `Definitions<TriggerEntry>` and `TriggerEntry` is `{occurrence, definition}`, so its text is at `.definition.description`; only `base_trigger_definitions` (`Vec<TriggerDefinition>`) exposes `.description` directly. MEASURED on the committed corpus, which is what the maintainer asked for and what the earlier "all 172 carriers matched non-null" claim did not establish: of the `trigger_definitions` entries, ZERO expose a direct `.description` and 100% nest it (145 / 165 / 132 on dellian / dina / witherbloom). The live list therefore contributed NOTHING — every entry collapsed to the `// ""` fallback — and all 172 carriers resolved through `base_trigger_definitions` alone. Total descriptions visible to the derivation across the corpus: 875 before, 1755 after. REACHABLE, not theoretical: `dellian_emblem_conqueror_4p` carries a GRANTED trigger ("When ~ dies, you gain 1 life.") present in the live list and absent from the base list. A firing whose description existed only there would have aborted the whole stamp on a classifiable fixture. BEHAVIOUR-PRESERVING on the corpus: 172 carriers resolve before AND after, all `Ordinary`; the pristine regeneration stays BYTE_IDENTICAL=true. 2. `stamp_trigger_firing` assigned all three carrier keys unconditionally without reading them, so `stamp-fixture-firing.sh`'s header claim that in-place stamping "cannot revert anything" was false for exactly those keys — and arm 1 structurally cannot catch it, because it deletes them from both sides before comparing. An already-canonical `Delayed` carrier would either abort the stamp or be silently rewritten to `Ordinary`, the CR 603.7a to CR 603.1 re-classification this library exists to refuse. Now derives only into an ABSENT slot, which also makes the stamp idempotent. Preservation is scoped to stack entries that are still on the stack, so a stale key cannot accumulate and inflate the carrier total past the number of triggered records — the one shape that could have let arm 2's aggregate comparison cancel a surplus against a deficit. 3. `select(.command.DelayedTriggerInstall)` indexes `.command` with a key, which aborts jq with a raw type error on a serde unit variant (a bare JSON string). This file's contract is that undetermined cases abort BY NAME; filtered to objects first so the probe stays total. TWO NEW PRE-FLIGHT CONTROL ARMS, both with negative controls, because no fixture-level arm can witness either property: arm 4 DEFINITION_SHAPES — `_defs` resolves a nested-only description AND a direct-only one, and still ABORTS on one present in neither. arm 5 CARRIER_PRESERVED — an existing canonical carrier survives, while an absent one is still derived. REVERT-PROBE, run: restoring the old `_defs` ⇒ arm 4 reports `nested=FAILED` and the script refuses to stamp (rc=1), while `direct` stays green and the negative control still aborts; restored byte-identical. DOC: the header said the stamped artifact minus "the three new keys"; the executable `del()` names FIVE (three carriers + two allocators). Corrected, as promised in the PR body. Assisted-by: ClaudeCode:claude-opus-5
Maintainer Medium 4 on phase-rs#6886, and CodeRabbit 3699361092 (Critical) / 3699361088 / 3699361096. 1. NON-ATOMIC WRITE, and the destination is the committed fixture. `regenerate` redirected the pipeline straight into `$dest`; the shell creates and TRUNCATES a redirection target before the first command in the pipeline runs, and the production path passes `$OUT`. This recipe aborts BY DESIGN — `_firing` raises `UNDETERMINED firing carrier`, `stamp_delayed_allocators` raises `UNDETERMINED delayed-trigger allocators` — so `set -e` / `pipefail` stopped the script only AFTER the fixture had been truncated and a partial gzip stream written over it. The failure mode of a fail-closed recipe was destruction of the artifact it was refusing to rewrite. Now stages to `mktemp` and `mv`s only on success, matching `stamp-fixture-firing.sh`. 2. PASS-THROUGH CORRUPTION. The final `{gameState:.gameState}` is a REWRITE, not a projection, for a dump that is not `gameState`-shaped: `.gameState` is null on those, so the document became `{"gameState":null}` — silently, since that is valid JSON. Several fixtures in this corpus really do use the other envelope (top level `turn_number`), which is why `lib/trigger-firing.jq` already passes them through. Keyed on `has("gameState")` rather than truthiness, so an explicitly-null `gameState` is not quietly normalised into the husk shape. Preserves the input unchanged, per the maintainer's disposition. 3. ARM 2 HAD GONE VACUOUS — it was reporting the opposite of its claim. It compared the patched and unpatched regenerations wholesale and required a difference, and inferred from that difference that the `effect_kind` filter had teeth. Stage 2b broke the inference: the patched filter also runs `stamp_trigger_firing` and `stamp_delayed_allocators`, and the allocator stage rewrites the two allocator keys on EVERY `gameState`-shaped dump in this corpus (measured: all six move to 1), while the unpatched filter runs neither. So the documents differed unconditionally, including on the dumps that carry no target prompt at all, and arm 2 printed `PATCHED_DIFFERS=true` while `effect_kind` had matched nothing. Now compares the `target_slots` projection specifically, and NAMES the no-prompt case instead of counting it as a pass. New arm 3 `STAGE_2B_LANDED` covers what arm 2 no longer claims. TWO PRE-FLIGHT SELF-TESTS, on synthetic inputs through the SAME `transform` the migration uses, because neither property can be witnessed by a corpus fixture — both are about inputs this corpus does not contain. Each has a paired positive control: ATOMIC_ON_FAILURE — a real abort leaves the destination byte-identical to a sentinel (and a SUCCEEDING run must still replace it, or the row would pass because nothing ever writes). ENVELOPE_PRESERVED — a `turn_number` dump survives unchanged (and a `gameState` dump must still be projected). VERIFIED NEUTRAL against the pre-change script, run side by side: dellian reproduces `BYTE_IDENTICAL=true` and the same `PATCHED_DIFFERS=true stamped=["LoseLife"]`. dina's `BYTE_IDENTICAL=false` is PRE-EXISTING and reproduces identically under the original script — it is the documented later-parser-state divergence, not a regression from this change. Assisted-by: ClaudeCode:claude-opus-5
34f5823 to
94bb153
Compare
|
🤖 AI text below 🤖 All three findings are addressed, and probing one of them turned up a fourth defect you did not Findings → commits
HIGH —
|
| run | board | leg assert | accounting arm | result |
|---|---|---|---|---|
| M1 | as shipped | present | deleted | RED |
| M2 | as shipped | absent | deleted | RED |
| M3 | parked non-priority | absent | deleted | RED — but not for the reason claimed |
| M4 | parked non-priority | present | intact | RED |
M1 vs M2 is the pair that matters: deleting the accounting arm flips the row red with and without
the new assert, so the row was never passing for the wrong reason as it stood. And the scenario the
comment describes turns out not to be expressible on this fixture — M3 fails at the row's own
reach-guard, not at any arm attribution:
reach-guard: the derived set must actually carry the Unaccounted variant
([Draw { .. }, ZoneChange { .. }])
Parking waiting_for at ReplacementChoice changes what the chain proposes — the sub-ability's
Tap stops being derived — so the row dies before the two legs can diverge. So this is a
completeness guard against future drift, not a fix for a reachable defect, and the code comment says
exactly that rather than implying a bug was closed. Worth adding that leg one is not unguarded
overall: a_prompt_standing_on_the_incoming_board_refuses_the_probe covers it directly at the
production level by parking a prompt on the incoming board. What was missing was only its
attribution inside this row.
3699911169 [Minor] — taken, and it is the kind of error worth more than its severity label.
resolution_prompt.rs cited CR 707.2c for Effect::ChoosePermanent. Verified against
docs/MagicCompRules.txt: 707.2c is "If a static ability generates a continuous effect that's a
copy effect, the copiable values that effect grants are determined only at the time that effect
first starts to apply" — a copiable-values timing rule with nothing to say about resolution-time
choices. The correct rule is CR 707.6: "if an object enters the battlefield as a copy of
another permanent, the object's controller will get to make any 'as [this] enters the battlefield'
choices for it" — which is precisely the fresh choice that raises WaitingFor::CopyTargetChoice.
Corrected, with the rule's actual content in the annotation rather than a bare number. (Negative
control: 999.99z returns 0 hits, so the grep is discriminating and not matching everything.)
3699911175 [Major] — declined, refuted by the type. It asks that
quantity_offers_up_to_choice recurse into QuantityExpr::Power's base "matching the
Difference and Sum/Max arms". Power is declared { base: i32, exponent: Box<QuantityExpr> }
(types/ability.rs:7059-7060). base is a plain i32, structurally incapable of carrying an
UpTo, and the suggested quantity_offers_up_to_choice(base) would not compile — it would be a
type error, not a behaviour change. Its own fallback, "or document the invariant", is the applicable
half, and I have taken that: the arm now carries a one-line note saying why base: _ is not an
omission, so the asymmetry with Difference/Sum/Max stops reading like a bug to the next
reviewer. Worth noting the same comment's other bullet is your HIGH repeat_for finding, which is
real and fixed in 276265e93 — it found one true and one false in a single comment.
3699911189 [Minor] — deferred, with the trigger named. The claim is correct as stated: in
policies/loop_shortcut.rs the arm Fixed(_) if !schema.is_bounded() => na() precedes
Fixed(0) => reject, so a zero-count declaration against an unbounded offer scores neutral rather
than reject, and the class-bonus table ranks DeclareShortcut above DeclineShortcut. Two reasons
it is not in this PR. First, reachability: the arm's own comment already records that today's
generator emits Fixed only for bounded schemas, and the M5 fix in this PR gates that push on
schema.is_bounded() as well, so the branch is now unreachable through two independent conditions
rather than one. Second, cost: it is a phase-ai scoring change, which carries the cargo ai-gate
paired-seed obligation, and the AI gates are exactly the checks currently dying at the 60-minute
timeout described below — I would be buying a gate I cannot read in order to reorder an arm nothing
can reach. It goes in with the Fixed-candidate generator-gap follow-up, where the reachability and
the gate run land together.
The required Rust check — it is a timeout, not a failing test
Understood on not substituting a local run for a required check; that is why this is pushed and the
checks are running at head. But the red rows on the previous head were not test failures, and I
would rather you not go hunting for a panic that does not exist:
| check | conclusion | duration | failed steps |
|---|---|---|---|
Rust tests (shard 1/2) |
cancelled |
20m18s | 0 |
Rust tests (shard 2/2) |
cancelled |
20m16s | 0 |
Rust (fmt, clippy, test, coverage-gate) |
failure |
3s | — |
ci.yml:123 sets timeout-minutes: 20 on the shards, and GitHub records a timeout-minutes kill
as cancelled. Equal per-job durations are the timeout signature; a fail-fast cancel lands at one
wall-clock instant with unequal durations. The whole run's conclusion is completed/cancelled with
no panic in the logs. The aggregate row is downstream, not independent: it started at 17:56:59,
after both shards were killed at 17:56:52 and 17:56:55.
Whose fault, both facts:
| ref | shard 1 | shard 2 |
|---|---|---|
main run 30746439872 |
17m17s ✅ | 16m24s ✅ |
main run 30755708363 |
16m42s ✅ | 17m43s ✅ |
this PR @ dd986e404 (before this round) |
timeout ❌ | 19m40s ✅ — 20s of headroom |
this PR @ 34f582386 (after) |
timeout ❌ | timeout ❌ |
this PR @ 94bb153b0 (this comment's head) |
running at post time | running at post time |
Shard 2 had twenty seconds of margin and this round's commits consumed it; that crossing is mine.
And main runs at 82–89% of the cap, so the PR is spending headroom that was already nearly gone.
I did not trim tests to fit: local integration is ~190s of wall time, so the shard is dominated by
build rather than execution, and deleting test bodies would cost real coverage for very little clock.
But shard 1 was ALREADY timing out at dd986e404, before any of this round's work — and that is
what makes this a blocker rather than a courtesy. No change I can make inside the repository
produces a green required Rust aggregate: even if I reverted every commit of this round, shard 1
still times out and the aggregate job still reports its cancelled dependency. The required-green you
are asking for is reachable only through .github/workflows/, which is yours and which I have not
touched. In ci.yml, the shard job at :121:
timeout-minutes: 20 # -> 30That cures shard 1 as well as shard 2. If you would rather add a third shard, or split the slow
dump-driven rows into their own job, I will prepare whichever you prefer — but one of them has to
land on your side before this PR can show the green aggregate the review asks for. I am not going to
pretend otherwise or quietly leave the check red without saying why.
The two AI gates were killed the same way one tier up: Paired-seed AI gate cancelled at 60m17s
and Decision-cost perf gate cancelled at 60m16s, against timeout-minutes: 60
(ai-gate.yml:32, :121). That is the third head on which I have measured it, alongside the ~15
unrelated branches cited in my previous comment, so I am treating it as infrastructure rather than
PR signal.
Local gate
| check | result |
|---|---|
cargo fmt --all --check |
rc=0 |
cargo clippy --workspace --all-targets -- -D warnings |
rc=0 |
cargo test -p phase-engine --lib |
18355 passed, 0 failed |
cargo test -p phase-engine --test integration |
4422 passed, 0 failed |
cargo test -p phase-ai |
0 failed |
All 43 commits in the range also compile on their own — swept one by one at the pushed tip, not
inferred from the tip building. Re-requesting review — though the request itself 404s for this
account (pull only, no triage), so consider this the ask.
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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.
Inline comments:
In `@scripts/migrate-dump-fixture.sh`:
- Around line 189-199: Update regenerate to register each staged path in the
shared cleanup list used by the EXIT, INT, and TERM traps, ensuring the stage
file is removed on interruption and failed mv. Replace the per-function cleanup
approach as needed, and update the control-mode trap near the existing EXIT trap
to append PATCHED and UNPATCHED to STAGE_FILES rather than replacing the shared
cleanup behavior.
- Around line 368-378: Update the ARM 3 validation around the STAGE_2B_LANDED
check to compare the allocator projection from $PATCHED with $UNPATCHED,
including both next_delayed_trigger_token and next_delayed_trigger_instance.
Treat identical projections as an already-satisfied control case rather than
reporting the allocator repair as landed, while preserving the existing failure
for patched output that still lacks valid allocator values and the success
logging for a genuine change.
In `@scripts/stamp-fixture-firing.sh`:
- Around line 229-253: Update the ALLOC_NEED calculation in the fixture loop to
skip allocator repair when .gameState is absent, matching the guard used by
stamp_delayed_allocators. Ensure non-gameState fixtures produce ALLOC_NEED=0 so
they remain unchanged and are skipped rather than failing ARM3.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: f0f24b22-a6fe-4e7d-9b4e-e82b373703e0
⛔ Files ignored due to path filters (6)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (48)
crates/engine/src/ai_support/candidates.rscrates/engine/src/analysis/corpus_tests.rscrates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/effects/choose.rscrates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/clash.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/proliferate.rscrates/engine/src/game/effects/separate_piles.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine.rscrates/engine/src/game/filter.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mod.rscrates/engine/src/game/phasing.rscrates/engine/src/game/players.rscrates/engine/src/game/replacement.rscrates/engine/src/game/resolution_prompt.rscrates/engine/src/game/sba.rscrates/engine/src/game/stack.rscrates/engine/src/game/targeting.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/tests/integration/fantastic_four_bounded_loop.rscrates/engine/tests/integration/gift_recipient_phased_out_opponent.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.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/loop_shortcut_offer_writer_census.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/rules/battle.rscrates/engine/tests/integration/sprout_inalla_realistic_offer.rscrates/phase-ai/src/policies/loop_shortcut.rscrates/phase-ai/src/projection.rscrates/phase-ai/src/search.rsscripts/lib/trigger-firing.jqscripts/migrate-dump-fixture.shscripts/stamp-fixture-firing.sh
🚧 Files skipped from review as they are similar to previous changes (44)
- crates/phase-ai/src/search.rs
- crates/engine/src/game/effects/proliferate.rs
- crates/phase-ai/src/projection.rs
- crates/engine/src/game/casting_costs.rs
- crates/engine/src/types/mod.rs
- crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs
- crates/engine/src/game/casting_tests.rs
- crates/engine/src/game/effects/clash.rs
- crates/engine/src/game/effects/separate_piles.rs
- crates/engine/src/game/casting.rs
- crates/phase-ai/src/policies/loop_shortcut.rs
- crates/engine/tests/integration/sprout_inalla_realistic_offer.rs
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/effects/token.rs
- crates/engine/src/analysis/corpus_tests.rs
- crates/engine/tests/integration/main.rs
- crates/engine/src/analysis/loop_check.rs
- crates/engine/src/game/filter.rs
- crates/engine/src/game/zone_pipeline.rs
- crates/engine/src/game/ability_utils.rs
- crates/engine/src/game/sba.rs
- crates/engine/src/game/mod.rs
- crates/engine/tests/integration/interaction_contract.rs
- crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs
- crates/engine/src/game/players.rs
- crates/engine/src/game/phasing.rs
- crates/engine/src/ai_support/candidates.rs
- crates/engine/src/game/effects/choose.rs
- crates/engine/src/game/replacement.rs
- crates/engine/src/types/game_state.rs
- crates/engine/tests/integration/rules/battle.rs
- crates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.rs
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- crates/engine/src/game/interaction.rs
- crates/engine/src/game/ability_scan.rs
- crates/engine/src/game/targeting.rs
- crates/engine/src/game/stack.rs
- crates/engine/tests/integration/fantastic_four_bounded_loop.rs
- crates/engine/src/analysis/decision_template.rs
- crates/engine/tests/integration/gift_recipient_phased_out_opponent.rs
- crates/engine/src/game/engine.rs
- crates/engine/tests/integration/loop_shortcut.rs
- crates/engine/src/analysis/resource.rs
- crates/engine/src/game/effects/choose_from_zone.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The fixture stamper rejects valid non-gameState envelopes. Evidence: scripts/stamp-fixture-firing.sh:229-253 reads allocator fields unconditionally, although scripts/lib/trigger-firing.jq:145-146 intentionally passes such envelopes through. Why it matters: for every fixture without gameState, NEED is zero but ALLOC_NEED becomes one and ARM3 fails, so this new general-purpose script refuses an unchanged valid fixture. Suggested fix: make allocator need and ARM3 conditional on a present gameState (as stamp_delayed_allocators already is), and add a non-gameState regression case that reaches the script's skip path.
The current parse-diff artifact is bound to 94bb153b0a03fa08f0b0af2d5f8153d03ebde629 and reports no parser changes. I also confirmed the populated per_cycle JSON issue raised earlier is fixed at this head by the map_key_pairs serde adapters and its persistence-boundary test.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/phase-ai/src/policies/loop_shortcut.rs (1)
246-248: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a test row for the
Fixed(0)reject arm.The test module covers
Fixed(4)andFixed(11)on a bounded offer, andFixed(4)on an unbounded one. No row exercisesFixed(0). Delete the arm at Lines 246-248 and every test still passes, so the arm carries no regression protection.♻️ Proposed test row
+ /// CR 732.2a — the zero-count arm asserted directly. A `Fixed(0)` declaration commits no + /// cycles while spending the CR 732.2b response window, so it is weakly dominated by + /// declining. REVERT-PROBE: delete the `Fixed(0)` arm ⇒ this row reads + /// `"loop_shortcut_bounded_declare_progress"` at the critical band ⇒ FAILS. + #[test] + fn loop_shortcut_bounded_declare_rejects_zero_count() { + let state = bounded_offer_state(10); + assert!(schema_of(&state).is_bounded(), "REACH-GUARD: the zero arm sits below the \ + unbounded branch, so this row is vacuous unless the schema reads bounded"); + let v = verdict_for(&state, &declare(IterationCount::Fixed(0))); + assert!( + matches!(v, PolicyVerdict::Reject { .. }), + "a zero-cycle declare commits nothing and must be vetoed, got {v:?}" + ); + assert_eq!(kind_of(&v), "loop_shortcut_bounded_declare_zero_count"); + }🤖 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/phase-ai/src/policies/loop_shortcut.rs` around lines 246 - 248, Add a test case in the loop-shortcut policy test module covering a bounded offer with IterationCount::Fixed(0), and assert it returns the rejection verdict with reason "loop_shortcut_bounded_declare_zero_count". Keep the existing Fixed(4), Fixed(11), and unbounded Fixed(4) cases unchanged.
🤖 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.
Inline comments:
In `@scripts/stamp-fixture-firing.sh`:
- Line 234: Update the staging flow around TMP and the final move so the
temporary compressed fixture is created in dirname "$FIX", ensuring the
subsequent mv remains atomic on the destination filesystem. Register this stage
path in the existing cleanup trap so interruptions during the gzip/jq pipeline
remove it, while preserving the current rm -f "$TMP" cleanup calls for other
paths and the final replacement behavior.
---
Nitpick comments:
In `@crates/phase-ai/src/policies/loop_shortcut.rs`:
- Around line 246-248: Add a test case in the loop-shortcut policy test module
covering a bounded offer with IterationCount::Fixed(0), and assert it returns
the rejection verdict with reason "loop_shortcut_bounded_declare_zero_count".
Keep the existing Fixed(4), Fixed(11), and unbounded Fixed(4) cases unchanged.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 8a8b6da3-ab1e-4993-8816-f57edac9f08b
⛔ Files ignored due to path filters (6)
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzis excluded by!**/*.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gzis excluded by!**/*.gz
📒 Files selected for processing (55)
client/src/i18n/locales/de/common.jsonclient/src/i18n/locales/en/common.jsonclient/src/i18n/locales/es/common.jsonclient/src/i18n/locales/fr/common.jsonclient/src/i18n/locales/it/common.jsonclient/src/i18n/locales/pl/common.jsonclient/src/i18n/locales/pt/common.jsoncrates/engine/src/ai_support/candidates.rscrates/engine/src/analysis/corpus_tests.rscrates/engine/src/analysis/decision_template.rscrates/engine/src/analysis/loop_check.rscrates/engine/src/analysis/resource.rscrates/engine/src/game/ability_scan.rscrates/engine/src/game/ability_utils.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/casting_tests.rscrates/engine/src/game/effects/choose.rscrates/engine/src/game/effects/choose_from_zone.rscrates/engine/src/game/effects/clash.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/proliferate.rscrates/engine/src/game/effects/separate_piles.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/engine.rscrates/engine/src/game/filter.rscrates/engine/src/game/interaction.rscrates/engine/src/game/mod.rscrates/engine/src/game/phasing.rscrates/engine/src/game/players.rscrates/engine/src/game/replacement.rscrates/engine/src/game/resolution_prompt.rscrates/engine/src/game/sba.rscrates/engine/src/game/stack.rscrates/engine/src/game/targeting.rscrates/engine/src/game/zone_pipeline.rscrates/engine/src/types/game_state.rscrates/engine/src/types/mod.rscrates/engine/tests/integration/fantastic_four_bounded_loop.rscrates/engine/tests/integration/gift_recipient_phased_out_opponent.rscrates/engine/tests/integration/interaction_contract.rscrates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.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/loop_shortcut_offer_writer_census.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/rules/battle.rscrates/engine/tests/integration/sprout_inalla_realistic_offer.rscrates/phase-ai/src/policies/loop_shortcut.rscrates/phase-ai/src/projection.rscrates/phase-ai/src/search.rsscripts/lib/trigger-firing.jqscripts/migrate-dump-fixture.shscripts/stamp-fixture-firing.sh
🚧 Files skipped from review as they are similar to previous changes (44)
- crates/engine/src/game/phasing.rs
- crates/phase-ai/src/search.rs
- crates/engine/src/game/effects/token.rs
- crates/engine/src/game/sba.rs
- crates/engine/src/game/mod.rs
- crates/engine/src/game/ability_utils.rs
- crates/engine/tests/integration/loop_shortcut_mana_engine.rs
- crates/engine/src/game/casting.rs
- crates/engine/src/game/effects/separate_piles.rs
- crates/engine/src/game/players.rs
- crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs
- crates/engine/src/game/zone_pipeline.rs
- crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs
- crates/engine/tests/integration/sprout_inalla_realistic_offer.rs
- crates/engine/src/game/effects/clash.rs
- crates/engine/tests/integration/main.rs
- crates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.rs
- crates/engine/tests/integration/interaction_contract.rs
- crates/engine/src/game/casting_costs.rs
- crates/engine/src/ai_support/candidates.rs
- crates/phase-ai/src/projection.rs
- crates/engine/src/game/effects/mod.rs
- crates/engine/src/game/interaction.rs
- crates/engine/src/game/effects/choose_from_zone.rs
- crates/engine/src/game/effects/choose.rs
- crates/engine/tests/integration/gift_recipient_phased_out_opponent.rs
- crates/engine/src/game/casting_tests.rs
- crates/engine/src/types/mod.rs
- crates/engine/src/game/filter.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/targeting.rs
- crates/engine/src/analysis/loop_check.rs
- crates/engine/src/analysis/corpus_tests.rs
- crates/engine/tests/integration/fantastic_four_bounded_loop.rs
- crates/engine/src/game/effects/proliferate.rs
- crates/engine/src/analysis/decision_template.rs
- crates/engine/src/game/stack.rs
- crates/engine/src/game/ability_scan.rs
- crates/engine/src/game/replacement.rs
- crates/engine/src/game/resolution_prompt.rs
- crates/engine/src/game/engine.rs
- scripts/lib/trigger-firing.jq
- crates/engine/tests/integration/loop_shortcut.rs
- crates/engine/src/analysis/resource.rs
Resolve maintainer-caused integration with main while preserving the loop-shortcut resolution binding and persisted offer validation. Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
Restore contributor changes lost during the delayed-trigger lifecycle merge. Co-authored-by: Lindsey Gray <lindsey.gray@gmail.com>
|
Maintainer hold — current head This follows a maintainer-caused integration correction: I restored loop-shortcut content that was inadvertently dropped while porting the branch across main's delayed-trigger lifecycle migration. The previous completed failures belong to superseded head No contributor action is requested. Next step: once the current card-data job publishes the parse-diff artifact and the new checks settle, perform a current-head maintainer review before considering approval or enqueue. |
…he zero-count arm
Two CodeRabbit findings, plus the class sweep the first one implies.
The stamper's staging was not atomic. `mktemp -t` resolves to /tmp (device 50 here)
while the fixture directory is on /home (device 47), so `mv "$TMP" "$FIX"` degraded
to copy-then-unlink. This script rewrites its fixtures IN PLACE and those fixtures
are TRACKED, so an interrupted run truncates a committed fixture — a worse failure
mode than the migrate script's stray temp file. Staging now happens beside the
destination, and stage paths are registered with a trap on EXIT/INT/TERM.
This is the same defect, and the same fix, as `migrate-dump-fixture.sh` already
carries. That one was fixed and its sibling was not: one recipe, two call sites, one
of them missed. The new `stage_locality_control` pre-flight arm compares stage and
destination DIRECTORIES rather than devices, because device equality does not
discriminate — under a `-t` revert the stage lands in /tmp and a test destination
under /tmp shares its device. Revert-probed: reverting `stage_beside` to `mktemp -t`
reports STAGE_BESIDE_DEST=false ("stage /tmp vs dest /tmp/tmp.KQJVvxNaF5") and the
script exits 1.
CLASS SWEEP over scripts/ for the recipe "mktemp into system tmp, then mv to a
destination outside it":
grep -rn "mktemp" scripts/ --include=*.sh
grep -rn "\bmv " scripts/ --include=*.sh
Every other mv-into-place stages in the destination's own directory
(deploy-cf.sh:67, fetch-comp-rules.sh:47, gen-card-data.sh:248/304,
lib/mtgjson-fetch.sh:70, lib/scryfall-fetch.sh:77/131) or never moves at all
(coverage-history.sh:110, tilt-wait.sh:104). One further instance exists —
suno-generate/generate.sh:82 — and is deliberately NOT touched here: it is an
unrelated media-tooling subsystem and its destination (.state.json) is gitignored,
so the failure mode is losing local generator state, not corrupting a tracked file.
The Fixed(0) reject arm had no test. The suite covered Fixed(4)/Fixed(11) bounded
and Fixed(4) unbounded, so deleting the arm left every test green. Added a row with
a reach-guard asserting the schema reads bounded, since the arm sits below the
unbounded `na()` branch and would otherwise measure that instead. Revert-probed:
deleting the arm makes the row FAIL.
The AI change is a test row only — no scoring arm moved, no arm order changed, so
`cargo ai-gate` carries no new obligation.
Assisted-by: ClaudeCode:claude-opus-5
… helper
Review found the locality control in both fixture scripts tests the HELPER and
not the CALL SITE, so it cannot see the defect it exists to catch. Measured:
revert ONLY `TMP="$(stage_beside "$FIX")"` to `mktemp -t`, leaving `stage_beside`
and `stage_locality_control` untouched, and the control still prints
STAGE_BESIDE_DEST=true while the run writes with its stage in /tmp — the original
non-atomic write, fully restored, past a green control. The sibling has the same
shape: `migrate-dump-fixture.sh`'s a0 self-test proves `stage_path` RETURNS a
beside-destination path and says nothing about whether `regenerate` still calls it.
Both call sites now compare `dirname` of stage and destination before writing and
fail closed. Fixed at every site rather than the one that was reported: one recipe
with two call sites was how the first defect survived, and repeating that would
leave the maintainer asking why only half the class moved.
REVERT-PROBES, each reverting ONLY the call-site binding with the helper intact:
stamp-fixture-firing.sh, on a real tracked fixture (dina_conqueror_4p):
guards present rc=0 ctrl_true=1 guard=0 debris=0, fixture written
call reverted rc=1 ctrl_true=1 guard=1 debris=0, fixture UNCHANGED
"stage not beside destination: /tmp/stamp-firing-vwHw0q.json.gz
vs /tmp/tmp.VtIWnOZvTQ/dina_conqueror_4p.json.gz"
migrate-dump-fixture.sh:
guards present rc=0 guard=0, SELFTEST ATOMIC_ON_FAILURE=true ENVELOPE_PRESERVED=true
call reverted rc=1 guard=1, SELFTEST still ATOMIC_ON_FAILURE=true
`ctrl_true=1` and the unchanged SELFTEST line in the reverted arms are the finding:
the helper-level checks stay green across the defect. The guards are what move.
Comparison is by directory, not device, for the reason the helper control already
documents — under a `-t` revert both paths sit under /tmp and share a device.
Assisted-by: ClaudeCode:claude-opus-5
…lper Two seams the phase-rs#6933 fold analysis flagged that the merge resolution left open. Both are additive here; nothing in the merge is rewritten. 1. CR 732.2a bound guard, second ingress. phase-rs#6933 split the decode surface: the free `decode_persisted_resolution_state` became a delegator, and the two authorities are now `GameStateDecode::decode_persisted_resolution_state` and `GameStateDecode::decode`. They are genuinely separate — the former deserializes `ResolutionStateWire` itself and never calls the latter — and `impl Deserialize for GameState` routes through `decode` with `GameStateDecodeMode::DirectCurrentRaw`. With the guard on one site only, a persisted `LoopShortcut` offer whose wire bound is 0 was refused through the persisted path and revived through the bare-`GameState` path. A discriminating row lands with it (see loop_shortcut.rs): bare-`GameState` decode of `with_bound(0)` must `Err`, with a `with_bound(5)` control and a reach-guard that the UNMUTATED fixture decodes bare at all — `DirectCurrentRaw` skips the legacy migrations, so without that guard an `Err` would not be attributable to the bound. REVERT-PROBE: delete only the new call in `decode` and every pre-existing arm still passes while that row flips to `Ok`. 2. `delayed_trigger_payload_matches` is dead. It is base code from the v1 provenance backfill that phase-rs#6933 replaced; upstream deleted the definition AND its only caller together, and this branch adds no caller of its own (`git diff 9b97019..ea1b0ac -- game_state.rs` contains no added reference). Kept, it is a private fn with zero callers under `-D warnings`. Post-removal grep for the name returns 0 hits repo-wide. Also documents, at the `bind_resolution_scope` caller in stack.rs, why the CR 603.4 settlement must stay at the caller and never move into the helper: `analysis/resource.rs` calls that helper on CLONED PROBE BOARDS at five sites, where running terminal delayed-trigger disposition would mutate lifecycle state for a board that is only being measured. Comment only; the code is unchanged. ON THE CR ANNOTATION AT THAT BLOCK, since it is easy to read as drift: the trigger-event-context block keeps `CR 608.2k`, not the `CR 603.7c` inherited from before this branch. CR 608.2k — "If an ability's effect refers to a specific untargeted object that has been previously referred to by that ability's cost or trigger condition, it still affects that object even if the object has changed characteristics" — is what the block implements: it stores `current_trigger_event` so `TriggeringSpellController`/`TriggeringSource` resolve at resolution time. CR 603.7c is scoped to delayed triggered abilities and zone changes and does not describe this code. `resolution_prompt.rs` already cross-references CR 608.2k for this helper. Assisted-by: ClaudeCode:claude-opus-5
…ted payload Two independent changes to the same file, both consequences of phase-rs#6933. 1. The second-ingress row for the CR 732.2a bound guard added in the previous commit. Arms: bare-`GameState` decode of `with_bound(0)` must `Err` naming `max_iterations 0`; `with_bound(5)` must load; and a REACH-GUARD that the unmutated fixture decodes bare at all, because `DirectCurrentRaw` skips the legacy migrations and an `Err` from a fixture that cannot decode at all would say nothing about the bound. The pre-existing arms above it cannot stand in for this one: they reach `decode_persisted_resolution_state`, and a revert of the guard call in `decode` ALONE leaves every one of them green. The orphaned `PersistedGameState` import goes with it — the branch's loader uses the fully-qualified path, so the merge left the short import unused (`-D warnings`). 2. r28_c's TRUSTED arm hand-built `{"state": <bare GameState>}`. phase-rs#6933 made `resolution_state_version` a required discriminator and gave only the PersistedRaw ingress permission to stamp v1 onto a legacy payload; the TrustedEnvelope ingress deliberately stamps nothing, because a trusted snapshot is WRITTEN as a versioned envelope and must retain its declared compatibility mode. `GameState`'s derived `Serialize` emits no such field, so the hand-built value was a shape production never writes and the trusted path was right to refuse it. Measured before the fix: `Error("resolution state wire is missing a numeric resolution_state_version")`. Fixed at the BUILDER, not the assertion: the payload is now constructed through `ResolutionStateWire::from_game_state`, which is exactly what `TrustedGameStateEnvelope`'s own `Serialize` does, so the arm round-trips the shape production writes. No assertion was weakened. The decode `.expect` also becomes a labelled panic. The row drives four arms (hostile x trusted) and the original message carried no `{label}`, so the failure above could not say which arm broke — it had to be re-derived by hand. Assisted-by: ClaudeCode:claude-opus-5
…er controls
Arms 5 and 6 asserted against `{"Delayed":null}`, a wire shape phase-rs#6933 removed.
The live `TriggerFiring` variants (`types/identifiers.rs`) are `"Ordinary"`,
`"LegacyDelayed"`, `{"ReceiptEligible":{token,instance,source_id}}` and
`"UnknownLegacy"`.
Those arms stayed GREEN across the removal, which is the point worth recording:
jq is untyped, so the controls fed themselves a shape the engine can no longer
emit and got it back unchanged. A control that passes on input its subject cannot
produce is not evidence about the subject. The arms were not wrong — they had
stopped being about the engine.
Adds a `ReceiptEligible` sub-arm alongside the unit-variant one. That variant
carries a payload, and a preservation rule written against the unit variants
alone could drop its `DelayedTriggerOrigin` and still satisfy every other arm —
so this asserts the VALUE survives, not just the discriminant.
Measured after the change: DEFINITION_SHAPES, CARRIER_PRESERVED (with
`receipt={"ReceiptEligible":{"token":3,"instance":4,"source_id":7}}`),
STALE_CARRIER_PRUNED, NON_GAMESTATE_SKIPPED and STAGE_BESIDE_DEST all true;
12 fixtures, 0 arm failures.
Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 Acknowledged — holding, no push. Thank you for the merge and the restore; for what it is worth, Two heads-ups on
We hold five small additive commits ready on top of Your call on timing: pushing supersedes this head's CI cycle and parse-diff artifact, so we will not push until you say — either now, or after your current-head review. Defaulting to holding. |
|
🤖 AI text below 🤖 Noting your hold, and that these five commits went up on top of it at the repository author's Thank you for landing the #6933 fold and the follow-up restore. That was the blocking piece and it One thing worth recording because it is a genuine cross-check rather than a courtesy: an independent The five commits, in order
Two seams on
|
| fixture | waiting_for | entry | firing | normalized |
|---|---|---|---|---|
dina_conqueror_4p |
Priority | set | Ordinary | yes — entry + stamped firing |
witherbloom_sprout_lumaret_4p |
Priority | set | Ordinary | yes — entry + stamped firing |
witherbloom_sprout_lumaret_simple_4p |
Priority | set | Ordinary | yes — entry + stamped firing |
kilo_freed_relic_pentad_4p |
Priority | set | null | yes — entry only |
sprout_witherbloom_realistic_lands_4p |
Priority | set | null | yes — entry only |
dellian_emblem_conqueror_4p |
TriggerTargetSelection | set | Ordinary | no |
fantastic_four_bounded_loop_4p |
TriggerTargetSelection | set | Ordinary | no |
tenacity_exquisite_blood_4p |
LoopShortcut | set | Ordinary | no |
No fixture was re-migrated and no expectation was rewritten, because the normalization is correct
and nothing depended on the old state: CR 117.3b — "The active player receives priority after a
spell or ability (other than a mana ability) resolves" — means a dump AT Priority with an empty
resolution stack has no resolution in progress, so the carrier there is leftover rather than live.
A v2 envelope would have preserved a state the rules say cannot be current. The full integration
suite confirms zero rows moved on account of this.
The stamping scripts are now redundant for v1 loadability, and that is your call to make.
#6933's auto-stamp assigns a blanket LegacyDelayed to unlabeled v1 carriers at load, which is
strictly sufficient to make the corpus load. The scripts derive per-record classification from
trigger_definitions instead, so they emit Ordinary where the engine-side default emits
LegacyDelayed. Both are defensible: a measured classification versus an honest fail-safe. The
committed stamps are kept because they still do work the engine-side migration does not — the
bare-GameState (DirectCurrentRaw) decode arms skip all migrations and read what is on disk,
and future captures still need stamping — but if you would rather drop the script apparatus and
rely on your engine-side migration, say so and it comes out. Your subsystem, your call; we are not
attached to the scripts.
Related: the stamper's own controls had been asserting against {"Delayed":null}, a wire shape
#6933 removed. They stayed green through the removal because jq is untyped and they were feeding
themselves the dead shape. Repinned to "Ordinary" / "LegacyDelayed" /
{"ReceiptEligible":{…}}, with a new sub-arm proving the payload-carrying variant keeps its
payload rather than only its discriminant.
Verification
A local gate at this tip is mid-run. Stated as it actually stands rather than as a finished table:
cargo fmt --all --check rc=0 and the parser combinator gate rc=0; clippy --workspace --all-targets -D warnings, phase-ai, engine --lib and --test integration, and the 12-fixture
stamper corpus are still running and are reported here when they land, green or not.
What is already measured at this content: cargo check --workspace --all-targets clean;
r28_c passing, having failed before the builder repair; the stamper's five control arms green
across 12 fixtures with zero stage debris and a clean tracked tree. Your CI on this head is the
authority regardless, and it now runs against the two closed seams.
…fold The census pins each producer by `file:line`, and folding upstream phase-rs#6933 grew engine.rs around one of them: `game/engine.rs:10640 => :11427`, +787, while the file's whole-file delta over the same range is +1134 — so 787 lands above this producer and 347 below, which is what a file growing AROUND a mint looks like rather than one gaining a mint. Identity re-established at the new coordinate, not assumed. The line at :11427 is byte-identical by sha256 to `ea1b0ac19:game/engine.rs:10640`, and it is still inside `begin_pending_trigger_target_selection` (fn opens at :11278) — the producer this row names. The old coordinate now holds copy-target-slot code that mints nothing. The other four entries did not move at all, which is the same set-preservation evidence the earlier rebases in this comment relied on: a census that had gained or lost a producer could not leave four entries byte-identical AND in place. Nothing is weakened. The total stays 37 and the partition stays 5/7/25; only the coordinate of an unchanged producer is updated. This is the failure mode the row is DESIGNED to have — it is line-pinned precisely so that a moved or added mint cannot pass silently — so the red was the instrument working, not a defect in it. Surfaced by `cargo test -p phase-engine --lib` at 596bdfa; every other gate stage at that tip was green (fmt, parser gate, clippy -D warnings, phase-ai, integration, fixture corpus). Assisted-by: ClaudeCode:claude-opus-5
|
🤖 AI text below 🤖 Completing the verification table the previous comment left open, including the one row that came
The red, and it was our own instrument rather than anything about the fold. Identity was re-established at the new coordinate rather than assumed: the line there is That is the failure mode this row is designed to have: it is line-pinned precisely so a moved or Fix: |
|
Maintainer hold — current head The current CI rollup still has Rust test shards 1/2 and 2/2 plus the paired-seed and decision-cost AI gates in progress. The sole Next step: let those checks settle and regenerate/confirm the parse-diff for this exact head; then a fresh implementation and external-review finding pass can determine whether any changes are still required. No closure or enqueue is implied by this hold. |
|
Maintainer hold — current head The full implementation spans hot engine/AI paths and a broad lifecycle port. Its PR evidence still records Gate A and final Next step: complete and record a clean current-head Gate A/final implementation review (including the broad loop-shortcut owner, serialization, prompt, and bounded-offer paths). This is a hold only; it neither closes nor rejects the PR. |
|
Maintainer hold — current head Required Rust tests shard 2/2 is still in progress. The sole |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — the current loop-shortcut policy still classifies an unbounded Fixed(0) declaration as neutral instead of rejecting the no-op.
🟡 Must address
crates/phase-ai/src/policies/loop_shortcut.rs:218 matches every Fixed(_) declaration on an unbounded schema as na() before the zero-count rejection at :233-248. Thus Fixed(0) on an unbounded offer never reaches the rejection, despite the rationale at :233-245 applying equally: it commits no cycles while spending the response window. The existing regression at :574-585 deliberately asserts a bounded schema, so it cannot detect this ordering error.
Move the Fixed(0) arm before the unbounded arm and add a companion unbounded-schema assertion that expects the same reject verdict. This is the current form of CodeRabbit's still-open finding at discussion_r3699911189; the rest of that review's concrete concerns are addressed on this head.
✅ Rechecked
The current a62782802870cc33b03db11ffa8b7e548078b921 parse artifact reports no parser changes, and current required CI is green. PeriodicDelta now uses the wire-safe map adapter with a populated persistence-boundary test (crates/engine/src/analysis/resource.rs:12649-12710), while production probe callers bind their cloned resolution boards before querying (resource.rs:1711-1718, 4342-4350).
Recommendation: reorder the policy arms, add the unbounded zero-count regression, then request a fresh review.
2 participants
🤖 AI text below 🤖
Summary
Chain 3 of 3 of the combo-feedback series (phase 5, sub-phases 5a–5d). Splits
LoopDetectSampleinto normalized and live ring halves, derives the resolution obligation from the event record rather than from the prompt (CR 616.1), adds the shape-B mint conjuncts and the declare-timetemplate.ownerfirewall (CR 732.2a + CR 603.5), re-derives the loop-shortcut probe budget from the beat the corpus actually offers on, routes 5c player choice through one legality authority, and pins the resulting behaviour with rows driven beat-by-beat through the publicapply()on real captured 4p dumps. It also migrates the six 4p dump fixtures onto the mandatory CR 603.7 firing carrier that upstream #6842 introduced after this work was reviewed.Every headline claim below is the measured version, not the planned version. Where the tree falsified a prediction, the row is keyed to the measurement and the deviation is listed in the disclosures rather than being written as a passing row.
Files changed
Engine — loop-shortcut / CR 732.2a surface:
crates/engine/src/game/engine.rs(+4356/−394)crates/engine/src/analysis/resource.rs(+6441/−345)crates/engine/src/analysis/decision_template.rs(+359/−24)crates/engine/src/analysis/loop_check.rs(+16/−0)crates/engine/src/analysis/corpus_tests.rs(+3/−0)crates/engine/src/ai_support/candidates.rs(+29/−6)Engine — resolution-obligation partition (5d U1) and its 5c legality routing:
crates/engine/src/game/resolution_prompt.rs(+1503/−0, new module)crates/engine/src/game/ability_scan.rs(+17/−564)crates/engine/src/game/replacement.rs(+350/−0)crates/engine/src/game/stack.rs(+319/−56)crates/engine/src/game/effects/separate_piles.rs(+162/−4)crates/engine/src/game/effects/choose.rs(+108/−2)crates/engine/src/game/effects/choose_from_zone.rs(+89/−1)crates/engine/src/game/effects/proliferate.rs(+80/−1)crates/engine/src/game/effects/mod.rs(+60/−4)crates/engine/src/game/effects/token.rs(+12/−17)crates/engine/src/game/effects/clash.rs(+7/−2)crates/engine/src/game/players.rs(+53/−0)crates/engine/src/game/targeting.rs(+50/−51)crates/engine/src/game/casting_costs.rs(+12/−3)crates/engine/src/game/filter.rs(+11/−3)crates/engine/src/game/sba.rs(+10/−1)crates/engine/src/game/ability_utils.rs(+6/−6)crates/engine/src/game/phasing.rs(+6/−5)crates/engine/src/game/interaction.rs(+15/−1)crates/engine/src/game/zone_pipeline.rs(+5/−1)crates/engine/src/game/casting.rs(+3/−1)crates/engine/src/game/mod.rs(+1/−0)crates/engine/src/types/game_state.rs(+96/−4)crates/engine/src/types/mod.rs(+4/−4)crates/engine/src/game/casting_tests.rs(+73/−0)AI:
crates/phase-ai/src/policies/loop_shortcut.rs(+185/−16)crates/phase-ai/src/projection.rs(+1/−0)crates/phase-ai/src/search.rs(+1/−0)Tests (integration):
crates/engine/tests/integration/loop_shortcut.rs(+4116/−32)crates/engine/tests/integration/fantastic_four_bounded_loop.rs(+1403/−0)crates/engine/tests/integration/loop_shortcut_offer_writer_census.rs(+387/−0)crates/engine/tests/integration/gift_recipient_phased_out_opponent.rs(+202/−0)crates/engine/tests/integration/rules/battle.rs(+144/−0)crates/engine/tests/integration/issue_3867_volcanic_offering_opponent_choice.rs(+104/−0)crates/engine/tests/integration/kilo_live_offer_from_real_dump.rs(+21/−3)crates/engine/tests/integration/sprout_inalla_realistic_offer.rs(+7/−3)crates/engine/tests/integration/interaction_contract.rs(+5/−1)crates/engine/tests/integration/main.rs(+3/−0)crates/engine/tests/integration/loop_shortcut_mana_engine.rs(+1/−0)Fixtures (binary; re-stamped for the #6842 carrier — see disclosure 9):
crates/engine/tests/fixtures/dellian_emblem_conqueror_4p.json.gzcrates/engine/tests/fixtures/dina_conqueror_4p.json.gzcrates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gz(new)crates/engine/tests/fixtures/tenacity_exquisite_blood_4p.json.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_4p.json.gzcrates/engine/tests/fixtures/witherbloom_sprout_lumaret_simple_4p.json.gzFixture-migration mechanism:
scripts/lib/trigger-firing.jq(+114/−0)scripts/stamp-fixture-firing.sh(+130/−0)scripts/migrate-dump-fixture.sh(+201/−0)54 paths, +21281/−1555.
Track
Developer
LLM
Model: claude-opus-5
Tier: Frontier
Thinking: max
Implementation method (required)
Method: /engine-implementer
CR references
95 distinct CR numbers appear on lines this PR adds. Load-bearing ones: CR 732.1b, CR 732.2a, CR 732.2b, CR 732.2c, CR 732.3, CR 732.4, CR 732.5 (loop shortcuts), CR 603.1, CR 603.2c, CR 603.3c, CR 603.3d, CR 603.4, CR 603.5, CR 603.12, CR 603.12a (triggered abilities and the "may" beat), CR 616.1 (the resolution obligation the partition derives), CR 700.2b, CR 700.3, CR 800.4, CR 800.4a (handback/rollback), CR 704.x (state-based actions), CR 608.2x (resolution).
Full list: CR 101.2, 102.1, 102.2, 102.3, 104.2a, 104.3c, 104.4b, 107.1c, 107.3m, 111.1, 113.3b, 113.7a, 114.2, 115.1, 115.2, 115.10a, 117.1b, 118.12, 119.3, 119.8, 120.3a, 121.1, 121.2, 121.4, 310.10, 310.11a, 400.7, 405.5, 500.8, 503.1a, 504.1, 506.1, 510.2, 601.2b, 601.2c, 601.2d, 601.2f, 601.2h, 603.2c, 603.3c, 603.3d, 603.4, 603.5, 603.12, 603.12a, 608.1, 608.2b, 608.2c, 608.2d, 608.2h, 608.2k, 613.1, 614.1, 614.1a, 616.1, 700.2b, 700.3, 701.4a, 701.21a, 701.30b, 701.34a, 702.6a, 702.11c, 702.16b, 702.18a, 702.26b, 702.52a, 702.132a, 702.150a, 702.174a, 703.1, 703.2, 703.3, 703.4d, 704, 704.3, 704.5a, 704.5c, 704.5j, 704.5w, 704.5x, 706.2, 706.4, 707.2c, 707.10, 710.4, 732.1b, 732.2a, 732.2b, 732.2c, 732.3, 732.4, 732.5, 800.4, 800.4a. Every number was verified against
docs/MagicCompRules.txt; the control stringCR 999.99zwas correctly not found.Verification
This box is deliberately unchecked and the reason is stated rather than papered over. The independent review-impl gate ran at
dbc81821df74a5ad63b6a928ca80a5a720603522, the pre-rebase tree, and returned zero correctness findings. Upstream then advanced and this branch was rebased ontoe12447f4f. Measured withgit range-diff 73fd7f6de..dbc81821d e12447f4f..dd986e404: of the 25 reviewed commits carried across, 20 are content-identical (=) and 5 changed (!) —ec3f88c2d,ed4c158d0,89eacc318,254e11183,0512fccf6— which are exactly the four conflict resolutions plus the three commits the drift fixes were autosquashed into. Two test/fixture-only commits were added after that (21477adc6,dd986e404). Those 5 changed commits and the 2 new ones have not been through an independent review pass; they have been through the full gate below. Eight further pre-boundary commits and one empty rustfmt commit dropped out of the range as already-merged upstream (#6838/#6839).Everything below was run directly in the branch worktree at head
dd986e404(Tilt watches the main checkout, not this worktree):cargo fmt --all --check— exit 0.cargo clippy --workspace --all-targets -- -D warnings— exit 0, 0 warnings.cargo test -p phase-engine --test integration— exit 0; 4416 passed, 0 failed, 2 ignored.cargo test -p phase-engine --lib— exit 0; 18344 passed, 0 failed, 6 ignored.cargo test -p phase-engine --doc— exit 0; 7 doc tests, allignore-annotated, so 0 run and 0 failed.cargo test -p phase-ai— exit 0; 2067 passed, 0 failed summed across the crate's test binaries.scripts/check-parser-combinators.sh e12447f4fbdc64d04ed41c1e77193185085a6a0c— exit 0 (Gate A, run against the true merge base rather than the hook's fork-relative default). 0 files undercrates/engine/src/parser/are touched by this PR.cargo check -p phase-engine --all-targetsat each of the 27 commits inupstream/main..HEAD— 27 OK / 0 FAIL. This is the check that catches the exhaustiveness-arm class, where an early commit'smatchgains arms only a later commit defines, so the branch is bisectable rather than only green at the tip.main(96e41b3ab, fix(engine): record a token battlefield entry even when its events are suppressed (CR 403.3) #6851):git merge-tree upstream/main HEAD⇒ rc=0, 0 conflicts. File intersection with fix(engine): record a token battlefield entry even when its events are suppressed (CR 403.3) #6851 = 3 paths.trigger_carrier_countdefinition: needed == stamped on all six fixtures (154 / 7 / 3 / 6 / 1 / 1 = 172), every valueOrdinary,delayed_triggers: 0everywhere, allocators at 1/1.Discriminating revert-probes on the declare-time owner firewall. Each mutates one guard, observes the named rows, then restores — both restores verified byte-identical (
git diffempty, andsha256sumof the restored file equal togit show HEAD:of it):if template.as_ref().is_some_and(|t| t.owner != offer.proposer) { .. }removed fromhandle_declare_shortcut):r28_a_declared_template_owning_another_seat_is_refused_at_declareFAILED,r28_a_the_owner_firewall_is_reached_on_an_empty_schema_offer_tooFAILED, andr28_b_the_drive_seat_guard_compares_a_client_supplied_owner_against_itselfFAILED at exactly its(b2)assertion — its(b1)half runs first and still passed, which is the documented behaviour, since the injector reads no firewall. Control:r28_c_a_restored_proposal_with_a_foreign_template_owner_is_refused_at_consumptionstayed ok, so the consumption ingress is a genuinely different seam and not the same guard asserted twice.!offer.schema.points.is_empty()block (existence unchanged, placement changed):r28_a_the_owner_firewall_is_reached_on_an_empty_schema_offer_tooFAILED whiler28_a_declared_template_owning_another_seat_is_refused_at_declareandr28_c_...both stayed ok. The firewall's placement outside that block is therefore independently load-bearing and independently measured — an empty-schema offer skipspredictability_gateandvalidate_pinsentirely, so a guard inside the block would let an unvalidatedownerreach the proposal.Gate A
Gate A PASS head=dd986e404aa005233a39eaed35868438a5a1c9ab base=e12447f4fbdc64d04ed41c1e77193185085a6a0c
Anchored on
reject_shortcut_declaration, the single authority the five pre-existing declaration-refusal arms already land on (upstream/main:crates/engine/src/game/engine.rs:2988). The newtemplate.ownerfirewall is its sixth call site, so no row can observe which refusal fired first — the "sixth reject path added later" that authority's own doc anticipates.migrate_legacy_trigger_firing_carriers, which derivesOrdinaryfor DEFERRED contexts from the historically-omitteddispatch_origin: Normaldefault. The fixture stamp derives the same CR 603.1-vs-CR 603.7a discriminant for the ACTIVE-pending and stack cases, which that function's(None, None)arm deliberately refuses to infer.Final review-impl
Final review-impl PASS head=dbc81821df74a5ad63b6a928ca80a5a720603522
That SHA is the pre-rebase reviewed tree and is not in this branch's history; the delta between it and the current head is enumerated in the Verification section above. It is stated as the true head of the review rather than restated as the current head.
Claimed parse impact
None. 0 files under
crates/engine/src/parser/are touched.Scope Expansion
The fixture migration (
scripts/lib/trigger-firing.jq,scripts/stamp-fixture-firing.sh,scripts/migrate-dump-fixture.sh, and the six re-stamped fixtures) is outside the phase-5 (5a–5d) implementation scope. It was forced by the base advance, not chosen: upstream8121fd1c6(#6842) made a CR 603.7TriggerFiringcarrier mandatory on persisted triggered records and fails closed without one, which turned 44 rows red on rebase (41 decoder rejections + 3 assertion rows) on a tree that was green on its own base. See disclosures 9 and 10.Within that migration, the delayed-trigger allocator stamp is a third migration stage beyond the two that were pre-agreed (
effect_kind, firing carriers). It is disclosed in item 9 rather than folded in silently.Validation Failures
None.
CI Failures
None.
Series
ed0a8e55c)9169d8f44)Predecessor: #6839.
Why the span is 5a–5d and not just 5c+5d. The 10 commits at the base of this stack —
ec3f88c2d(5a, the per-iteration pin machinery) through62718fe01— are sub-phases 5a and 5b: the bounded-offer core, its amendment round (ed4c158d0,8c3966e7c,813ffef0d,6d12c65e6), four review-loop documentation commits, and one rebase-hygiene docs commit (62718fe01, pointing repro commands at the renamedphase-enginepackage after upstream #6739 — rebase adaptation, never part of the review loop), authored 2026-07-28 to 07-30 on the lane branch. They merged nowhere else: #6838/#6839 carried only the 8 pre-boundary commits (phases 0–4), andPeriodicDelta— introduced byed4c158d0— does not exist onmainat all. Unlike the 5c and 5d commits, none of those 10 carries a sub-phase tag in its subject line, which is why earlier drafts of this description labeled the chain by its two newest sub-phases only. The code content is unchanged by this correction; only the label was wrong.Disclosures
Each of these bounds a claim a reader would otherwise over-read. They are carried up from the commit messages and the executor journals, in the measured wording.
1. The 854× frozen-exemption headline does not describe the corpus's offering beat. Verbatim from
0512fccf6:The offering beat is dina beat 19 (
spent=13 asks=13 skips=0 ring=3 stack=10), and its certifying basis is B (ResourceSignatureOnly). Basis A certified 0 times across all three dumps against 129 basis-B certifications, so the within-basis-A disjunct has no value on this corpus.2. The F4 bounded offer FIRES, but an accepted declaration commits ZERO cycles.
r1bis the armed tripwire: it fails loudly the moment any remedy widens the announced set. Remedy sizing was measured, not guessed — wideningannouncedalone is insufficient; the sampler gate is not the seam (two relaxations left the frame census unchanged); the resolution-order sequence is empty at the offering beat. Closing it needs a new prompt-window sampling site. Named follow-up: announced-set widening plus an engine authority for AI pin CONTENT. Not closed here.3. dellian never offers at
PROBE_BUDGET = 26. Verbatim from.combofb-5d-executor-journal.md:1169-1174:4. The AI's only effective action at the F4 offer is decline, and the candidate-generator gap is reported, not closed. The engine-side seam is exactly one: the
WaitingFor::LoopShortcutarm ofcrates/engine/src/ai_support/candidates.rs, whoseFixed(max_iterations)candidate — the one that exists precisely for bounded offers — is gated onschema.points.is_empty(). F4 publishes one point, so that candidate is never generated and the legal set collapses toDeclareShortcut { count: UntilLethal, template: None }(refused outright byhandle_declare_shortcut) plusDeclineShortcut. The phase-ai policy independently reaches decline: the offer latchespredicted_winner: None, routingLoopShortcutPolicyto its(None, UntilLethal) => rejectarm. Note for whoever closes it: that file holds R8'sai_support/candidates.rs 1production-multiset entry, so a fix that ADDS aWaitingFor::LoopShortcut {construction there moves the multiset and must update R8's expected counts in the SAME commit.5.
ShortcutProposal.per_cyclecannot round-trip, and this PR introduces the defect.ShortcutProposalis plain serde insideGameState.waiting_for, butper_cyclecarries aPlayerId-keyed resource map andPlayerIdcannot deserialize from a JSON object KEY (invalid type: string "0", expected u8). A bounded-shortcut save is therefore unloadable, and the persisted-ingress row is reachable only forper_cycle: Noneproposals.Ownership stated precisely, because the journal's wording invites the wrong reading: the field is pre-existing relative to the 5d sub-phase but not relative to
main. Measured onupstream/main: thePeriodicDeltatype is absent entirely, andShortcutProposal(crates/engine/src/analysis/loop_check.rs:154) has exactly six fields, none of themper_cycle. (Greppingper_cycleonmaindoes hit — those areper_cycle_delta: u32, an unrelated field with no map key to decode.)git log -S'pub per_cycle' upstream/main..HEADbisects the field — on bothLoopCertificateandShortcutProposal— to a single commit at the base of this PR's stack (the 2nd of 27, sub-phase 5b):ed4c158d0 feat(engine): bounded CR 732.2a cycle fast-forward for a multiplayer drain. So the defect ships with this PR. Both arms of the affected row nullper_cycle, so they stay byte-identical exceptowner. The fix is deferred to its own lane; say the word if it should land here instead.6. Plan-deviation ledger. Rows the plan predicted but the tree falsified were not written as passing rows:
template.owner; it does not and cannot, becauseinject_pinned_answerholds the template but not the offer, so the seat guard compares a client-supplied value against itself.(b1)asserts that measured breach;(b2)supplies the refusal at the seam that does have the engine-issued comparand. If a future change closes the drive seam,(b1)flips and must be re-keyed, not deleted — its doc says so.:2832, superseded by the tree.7. Review record. Independent full-diff review-impl; Maintainer-Simulation Gate PASS; zero correctness findings; 3 LOW documentation fixes applied and delta-verified (
79f485728). The review was audited, not sampled. Its head and the post-review delta are stated exactly in the Verification section — please read that before treating the review as covering the current head.The 7-commit post-review delta (5 adapted + 2 new) was then put through a second, independent read-only review, which returned CLEAN — 0 defects. Its measured findings: the 5 adapted commits' deltas are mechanical only (the four refusal statements are byte-identical to upstream's
reject_shortcut_declaration; the dropped85cadabd3was whitespace-only rustfmt); all six fixtures are additions-only, a subset of the 5 stamped keys, with zero removals and canonical identity after stripping (thedina/witherbloom_..._simplebyte difference is a trailing jq newline, and the.gzgrowth is gzip-level only); the CR 603.5 census was re-derived independently and equals the committed expectation (same 5 producers, coordinates only); CR 603.1 / 603.5 / 603.7 / 603.7a all verified; hard-stop scan zero; and no test was weakened anywhere in the range.Scope caveat, stated so this is not over-read: that second review is static and structural — it ran no builds and no tests. Runtime green is carried entirely by the gate table in the Verification section above, not by it.
Known documentation nit, disclosed rather than force-pushed.
scripts/stamp-fixture-firing.sh:14says arm 1 strips "the three new keys", but the executabledel()list at:60names five (3 carrier keys + 2 allocator keys). The comment undercounts; the code is correct, and the control it actually runs is stronger than the comment claims. This is comment-only, found after the branch was pushed, and is deliberately not rewritten here — amending it would rewrite a published tip for a one-word change. It will be folded into the first requested change if there is one.8. Fold adaptation ledger. The rebase onto
e12447f4fhit four conflicts, each resolved preserving both intents rather than by taking a side:analysis/resource.rsin two commits — neither side compiled alone; took upstream's class-quantifiedarg2plus the widenedwindow_scope_from_cover_framesargument list.game/engine.rs+game/interaction.rs+types/game_state.rsin the 5c commit — 5c moves pin-validation after the count cap; upstream factored the handback intoreject_shortcut_declaration. Composed. Keeping HEAD's copy (the pre-move block) would have run pin validation twice on an unchecked count, silently defeating the hostile-Fixed(4e9)guard.game_state.rskept BOTH independent validators.game/engine.rsin the 5d U2 commit — the owner firewall routed throughreject_shortcut_declaration.Separately, three upstream-blamed test-literal sites broke only under composition and were fixed by content-neutral
--autosquashinto their owning commits (diff against the known-good pre-fold tree empty, 0fixup!left). One local commit (85cadabd3, a rustfmt oftriggers.rs) dropped as empty, verified redundant with upstream683a6e66bby reverse-apply, with an unapplied-commit control that reverse-applied non-clean.9. Fixture migration for #6842's mandatory CR 603.7 carrier. Derived per record, never defaulted:
trigger_definitions/base_trigger_definitions, matched by exactdescription(CR 603.1: a printed or granted triggered ability of a permanent is an ordinary triggered ability). Delayed ⟸ an install receipt indelayed_triggers(CR 603.7a). Anything else aborts by name; there is deliberately no fallback stamp.(source, description)classes, zero defaulted, allOrdinary— 154 dellian / 7 dina / 3 F4 / 6 tenacity / 1 + 1 witherbloom. All six fixtures recorddelayed_triggers: 0, soDelayed(Some(..))could not have validated regardless.UnknownLegacywas REFUTED as an option, not skipped.validate_firing(crates/engine/src/types/game_state.rs, 7 call sites) returnsErr("{carrier} has no canonical trigger firing discriminator")for it: it is the field-absent marker (skip_serializing_if) and the redaction default, never a legal persisted value.dina_conqueror_4pandwitherbloom_sprout_lumaret_simple_4pdiffer from their pristine regeneration in exactly one object each (Priest of Forgotten Gods'abilities/base_abilitiesAST), because the committed fixture carries a LATER parser state than the capture. Regenerating would have silently reverted that.fantastic_four_bounded_loop_4pis byte-identical across both paths.next_delayed_trigger_tokencarries#[serde(default)], so a bareGameStatedecode restores 0 while the productionPersistedGameStatepath runs a load-time repair to 1 — and 0 is invalid on its face, sincevalidate_trigger_firing_coherencerejectsnext_delayed_trigger_token <= max_token. Adjudicated by migrating, not by relaxing the assertion. Only the collapsed no-install-roots case is stamped; anything else aborts by name, because the general used-token walk is engine logic and re-deriving it in jq is the exact mistakemigrate-dump-fixture.shrefuses to make forEffectKind. Non-vacuity proven in both directions: all six dumps show 0 install commands across 158/30/2/516/50/5094 journal entries, and injecting one synthetic install command flips the selector to 1 AND flips the script toexit 1, nothing written.descriptioncollided exactly with a printed trigger on the same source object, would be stampedOrdinary. That conjunction is measured unreachable on this corpus — all six fixtures recorddelayed_triggers: 0and zeroDelayedTriggerInstallcommands across 158 / 30 / 2 / 516 / 50 / 5094 journal entries — so it is stated as a known bound of the derivation rather than left implicit.scripts/lib/trigger-firing.jqis the single definition of both derivations, loaded by both the in-place and the pristine-regeneration path, so neither path certifies its own copy.scripts/stamp-fixture-firing.shhas three control arms and refuses to write if any fails. Two self-caught errors are worth recording: an arm keyed on byte difference gave a false pass on zero-carrier fixtures (jq re-serialization changes bytes without stamping) and was re-keyed on carrier count; and the filter would have CREATED agameStatekey on the dumps stored in theturn_numberenvelope — caught by a control before any write, and guarded.migrate-dump-fixture.shalso fixes a pre-existing bug where an unguarded|=aborted on the 4 dumps lackingtarget_slots, so it had only ever been usable on 2 of 6.game/effects/mod.rs:5896/5973/8927 ⇒ :5918/5995/8949(uniform +22),game/engine.rs:10500 ⇒ :10589(+89),game/effects/scoped_library_search.rs:452UNMOVED. All five were re-read at their new coordinates and are byte-identical to the pre-rebase tree at the old ones; a genuinely new producer could not leave an untouched file's coordinate fixed while shifting the others by a constant. Correcting my own earlier report for the record: I previously said upstream had ADDED a CR 603.5 producer. That was wrong — the row fired on coordinates. The correction is carried in the tripwire's own doc block so the next reader does not inherit the mistake.10. Framing. Phase 5d was green on its own base. The incompatibility was introduced by the base advance (#6842), and it was resolved by DERIVING the missing discriminator per record, not by relaxing the assertion that demands it. Upstream's own migration deliberately stops short here: for an ACTIVE pending trigger its
(None, None)arm returnsErr("active legacy pending trigger has no firing discriminator")— the exact text of the observed failures, which is empirical proof the manual stamp was necessary rather than redundant.11. Adjacent lane. #6851 (
96e41b3ab, merged) touches the same token-entry seam as chain 2's occurrence index. Re-measured at dispatch time: file intersection 3,git merge-tree upstream/main HEAD⇒ rc=0 with 0 conflicts. It is not a dependency of this PR.Not enqueued — leaving disposition to the maintainer.
Summary by CodeRabbit