ship/ai reliability fixes - #6842
Merged
Merged
Conversation
matthewevans
enabled auto-merge
July 31, 2026 16:46
Contributor
|
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 reducer-backed Evoke, prospective-mana, Pact, swarm, draw, and life-safety analysis. It adds delayed-trigger provenance across persistence, replay, stacking, and resolution. It integrates certified routes into AI search, policies, combat selection, mulligan forecasting, and public scoring APIs. ChangesEngine state and certified previews
Certified engine AI support
Phase AI integration
Estimated code review effort: 5 (Critical) | ~120 minutes Possibly related PRs
Suggested labels: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/engine/src/game/engine.rs (1)
9049-9053: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winTwo dead-branch trigger drops are missing the
pending_trigger_firingclear added everywhere else in this function.In
begin_pending_trigger_target_selection, three sibling sites pop the uncommitted trigger entry and clearstate.pending_trigger. Two of them were updated by this PR to also clear the new field, but two others were not:
- L9015-9018 (modal,
modal_choice_with_target_assignment_limitreturnsNone): updated, clearspending_trigger_firing.- L9035-9038 (random-selection defensive drop): updated, clears
pending_trigger_firing.- L9049-9053 (modal, "all modes unavailable"): NOT updated.
- L9165-9167 (non-modal, no legal target — the comment explicitly says Symmetric to the modal
all-modes-unavailablebranch above: if the "push first" dispatcher already pushed an in-construction entry for this trigger, pop it before clearing the cursor.): NOT updated.The comment at L9046-9048 calls the first missed branch a dead branch — kept as a defensive cleanup for any delayed-revalidation paths, but a latent bug behind a guard is still a bug if that guard is ever removed or the branch is reached. Since
state.pending_trigger_firingis read directly and independently elsewhere (e.g. the Pact-plan tracker checks it without gating onpending_trigger.is_some()), leaving it stale after abandoning a trigger under construction can make a dropped trigger's firing classification (potentiallyDelayed(Some(provenance))) still appear live.🐛 Proposed fix: clear `pending_trigger_firing` at both remaining drop sites
if unavailable_modes.len() >= modal.mode_count { super::stack::pop_uncommitted_pending_trigger_entry(state); state.pending_trigger = None; + state.pending_trigger_firing = None; return Ok(None); }let Some((target_slots, selection)) = selection_result? else { super::stack::pop_uncommitted_pending_trigger_entry(state); state.pending_trigger = None; + state.pending_trigger_firing = None; return Ok(None); };Also applies to: 9158-9168
🤖 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 9049 - 9053, In begin_pending_trigger_target_selection, clear state.pending_trigger_firing at both remaining trigger-drop sites: the modal all-modes-unavailable branch near unavailable_modes.len() and the non-modal no-legal-target branch. Keep the existing pop_uncommitted_pending_trigger_entry and pending_trigger cleanup intact, ensuring every abandoned trigger path resets the firing state.crates/engine/src/game/stack.rs (1)
3318-3339: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winPreserve trigger firing during batched resolution
resolve_batcheddiscards eachPoppedStackEntry.trigger_firingafter clearingresolving_trigger_firing. A batchable delayedEffect::Tokencan therefore execute without itsTriggerFiring::Delayedprovenance, causing Pact tracking to treat the obligation as expired. Preserve the firing classification beforeplan.execute. BecauseBatchRunKeyexcludes it, also prevent mixed ordinary and delayed entries from sharing a batch.🤖 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/stack.rs` around lines 3318 - 3339, Update resolve_batched to retain each popped entry’s trigger_firing and pass the preserved classification through plan.execute so delayed Effect::Token entries remain associated with TriggerFiring::Delayed. Also update batch grouping and BatchRunKey construction to include trigger-firing classification, preventing ordinary and delayed entries from sharing a batch.
🟠 Major comments (25)
crates/engine/src/ai_support/evoke.rs-250-259 (1)
250-259: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winConstrain the stack-origin
ChangeZonearm to removal destinations.The first
ChangeZonearm matches onorigin: Some(Zone::Stack)only. It ignoresdestination. Any stack-origin zone change is therefore classified asImmediateEffect::ExileStackObject, andcandidate_is_opponent_stack_objectapproves any opposing stack object. A "put that card onto the battlefield under its owner's control" or "return target spell to its owner's hand" effect then reportsProvenUseful, which is wrong for the battlefield case and mislabeled for the others.Match the destination explicitly so a non-removal destination falls through to
Noneand yieldsUnknown.🐛 Proposed fix
Effect::ChangeZone { origin: Some(Zone::Stack), + destination: Zone::Exile | Zone::Graveyard, target, .. } => Some(ImmediateEffect::ExileStackObject(target)),Choose the destination set that matches the value you intend to certify. If a bounce or library-shuffle stack removal should also count, add those destinations and rename the variant to reflect the class.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/evoke.rs` around lines 250 - 259, Update the stack-origin `Effect::ChangeZone` arm that produces `ImmediateEffect::ExileStackObject` to require an explicit removal destination, such as `Zone::Exile`, so battlefield or hand destinations fall through to `None` and remain `Unknown`. Preserve the existing `ExilePermanent` handling for exile destinations.crates/engine/src/game/derived_views.rs-394-434 (1)
394-434: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftRoute server state messages through the client-safe serializer.
GameStartedandStateUpdateserialize filteredGameStatevalues directly, bypassingClientGameStateRef. This exposes delayed-trigger allocators, firing maps, anddelayed_triggers[].provenancethatclient_state_wire_valueremoves. Move this redaction intofilter_state_for_vieweror enforce one client-safe serialization path.🤖 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/derived_views.rs` around lines 394 - 434, Update the server-state message serialization used by GameStarted and StateUpdate so it routes GameState through the client-safe path, rather than serializing filtered GameState values directly. Reuse ClientGameStateRef/client_state_wire_value or move the equivalent redaction into filter_state_for_viewer, ensuring delayed-trigger allocators, firing maps, resolved_rules_journal, and delayed_triggers[].provenance are excluded from client payloads.Source: Path instructions
crates/engine/src/game/derived_views.rs-439-463 (1)
439-463: 🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy liftMake client-state redaction compiler-enforced.
A renamed or newly added private carrier can bypass this key list and leak trigger authority. A future unrelated field with one of these names can also be removed silently.
serde_json::to_value(state)additionally allocates and traverses the full state for every client emission. Use an explicit client projection or per-type serialization redaction.🤖 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/derived_views.rs` around lines 439 - 463, Replace redact_private_trigger_firing’s recursive key-based filtering with an explicit client-state projection or per-type serialization redaction so private trigger-firing carriers are excluded by the type system. Update the client emission path to serialize that projection directly, avoiding serde_json::to_value(state) and the full-state allocation/traversal; ensure renamed or newly added private fields cannot be emitted accidentally.crates/phase-ai/src/policies/mulligan/mod.rs-109-114 (1)
109-114: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe all-
falseforecast value means both "no action found" and "never probed". BecauseSelf::default()satisfiesis_certified_dead_landless(), an unprobed hand is certified dead, and a test that supplies no hand cannot distinguish a real dead certification from the unprobed fallback.
crates/phase-ai/src/policies/mulligan/mod.rs#L109-L114: returnNone(or add aprobedfield) instead ofSelf::default(), and makecard_floor.rstreat the unprobed case as keepable.crates/phase-ai/src/policies/mulligan/card_floor.rs#L287-L303: add a card to the hand and assert the hand is non-empty, so the dead classification comes from an engine legality query and not from the fallback.🤖 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/mulligan/mod.rs` around lines 109 - 114, The mulligan forecast currently treats an unprobed hand as certified dead; update the player lookup in the forecast implementation at crates/phase-ai/src/policies/mulligan/mod.rs:109-114 to represent the unprobed case distinctly, such as returning None or tracking a probed state, and make the related card-floor logic treat it as keepable. In crates/phase-ai/src/policies/mulligan/card_floor.rs:287-303, add a card to the hand and assert it is non-empty so dead classification comes from the engine legality query rather than the fallback.crates/phase-ai/src/policies/mulligan/mod.rs-851-869 (1)
851-869: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winThe fast-mana row does not exercise
usable_nonland_mana_source.
add_zero_cost_mana_sourcecallsadd_zero_cost_actionfirst, so the object already carries a castable{0}Spellability. The added mana ability setscost = AbilityCost::Tapbut leavesactivation_zoneunset, unlikeadd_unaffordable_hand_activation, which setsSome(Zone::Hand). An ability with no hand activation zone produces noActivateAbilityaction for a card in hand. The keep therefore comes fromnormal_actionthrough the inherited{0}spell, not fromusable_nonland_mana_source. That field has no test that proves it.Set
activation_zone = Some(Zone::Hand)on the mana ability, build the fixture without the{0}spell ability, and assert the forecast field directly.💚 Suggested fixture and assertion change
fn add_zero_cost_mana_source(state: &mut GameState, idx: u64) -> ObjectId { - let object_id = add_zero_cost_action(state, idx); + let object_id = add_hand_card(state, idx, "Zero-Cost Mana Source", vec![CoreType::Artifact]); let mut ability = AbilityDefinition::new( AbilityKind::Activated, engine::types::ability::Effect::Mana {ability.cost = Some(AbilityCost::Tap); + ability.activation_zone = Some(Zone::Hand);+ let fast_mana_forecast = + OpeningHandActionForecast::for_hand(&[fast_mana], &fast_mana_state); + assert!( + fast_mana_forecast.usable_nonland_mana_source, + "the fixture must witness a hand-activatable mana source, not a castable spell" + ); let fast_mana_decision = registry.evaluate_hand(Based on the path instruction that a test must exercise the failure path the fix prevents and drive the engine through its production pipeline.
🤖 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/mulligan/mod.rs` around lines 851 - 869, Update the fast-mana test fixture around add_zero_cost_mana_source so it contains only the mana ability, with activation_zone set to Some(Zone::Hand), and no inherited zero-cost Spell ability. Drive the hand through registry.evaluate_hand and assert the resulting forecast’s usable_nonland_mana_source field directly, preserving coverage of the failure path this test is intended to validate.Source: Path instructions
crates/phase-ai/src/policies/mulligan/mod.rs-176-190 (1)
176-190: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winInclude
ForetellandPlayFaceDownin the opening-action classifier. Both are legal priority actions from cards in hand. Their omission can classify a viable hand as certified dead. The existingCastSpellarm already covers Adventure, Disturb, Plot, and other casting variants.🤖 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/mulligan/mod.rs` around lines 176 - 190, The is_normal_opening_hand_action classifier must recognize GameAction::Foretell and GameAction::PlayFaceDown as normal opening-hand actions. Add both variants to the hand-object matching arm, using each action’s card/object identifier so the action returns true only when that card is present in hand; preserve all existing classifications.Source: Path instructions
crates/engine/src/game/life_safety.rs-484-540 (1)
484-540: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftOptional-cost arming revalidates against a pending shape the reducer never produces, and the mismatch fails open.
begin_optional_additional_cost_attemptdemandsadditional_cost_source == SpellCostSource::Otherplus an unchanged flow, whilehandle_decide_repeatable_additional_costsets neither and clonespending_beforeafter the caller installed the flow. Any mismatch setsAttemptState::Invalid, records no receipt, and returnsCandidateLifeSafety::NotUnsafe, which the AI reads as proven-safe.
crates/engine/src/game/life_safety.rs#L484-L540: derive the accepted pending shape from what each reducer path actually produces, or drop theadditional_cost_sourceand flow-identity predicates and keep only the identity checks that the reducer guarantees on every path.crates/engine/src/game/casting_costs.rs#L1040-L1050: pass apending_beforecaptured before the flow install and apending_afterwhose fields satisfy the arming contract, and pass the prompt's innerOptionalcost asmaterialized_costifnext_repeatable_additional_costreturns a different value.🤖 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/life_safety.rs` around lines 484 - 540, The optional-cost arming validation does not match the reducer’s actual pending-state transitions and fails open as proven-safe. In crates/engine/src/game/life_safety.rs:484-540, update begin_optional_additional_cost_attempt validation to use reducer-produced pending shapes, or remove the additional_cost_source and flow-identity predicates while retaining reducer-guaranteed identity checks. In crates/engine/src/game/casting_costs.rs:1040-1050, capture pending_before before installing the flow, construct pending_after to satisfy the arming contract, and pass the prompt’s inner Optional cost as materialized_cost when next_repeatable_additional_cost differs.crates/engine/src/ai_support/swarm.rs-46-75 (1)
46-75: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
binds_declarationnever checks the incarnation, so the documented recycled-ID protection does not exist.Line 47 states that the incarnation ref prevents a recycled object ID from inheriting a claim.
binds_declarationreceives onlyObjectIdvalues and comparesattacker.object_id == *attacker_id. The incarnation counter stored indeclarationis never compared, so a witness that is cached, stored, or redeemed after any zone change binds a different incarnation of the same ID. The current caller incrates/phase-ai/src/combat_ai.rsre-derives the witness from the samestate, so the gap is latent today, but the guard cannot hold for a stored route (the search layer stores and redeems routes for other certificate kinds in this stack).Bind against the live state so the incarnation is part of the check, or delete the claim from the doc comment.
🔒 Proposed identity-binding check
pub fn binds_declaration( &self, + state: &GameState, attacks: &[(crate::types::identifiers::ObjectId, AttackTarget)], ) -> bool { self.declaration.len() == attacks.len() && self.declaration.iter().zip(attacks).all( |((attacker, defending_player), (attacker_id, target))| { attacker.object_id == *attacker_id + && state + .objects + .get(attacker_id) + .is_some_and(|object| { + ObjectIncarnationRef::from_object(object) == *attacker + }) && *defending_player == self.defending_player && *target == AttackTarget::Player(*defending_player) }, ) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/swarm.rs` around lines 46 - 75, Update SwarmCombatWitness::binds_declaration to validate each attacker's live ObjectIncarnationRef, not only its ObjectId, by accepting the relevant state or equivalent incarnation lookup and comparing it with the stored declaration entry. Preserve the existing order, target, and length checks so recycled IDs cannot satisfy a cached witness; alternatively remove the incarnation-protection claim from the declaration documentation if identity validation cannot be added.crates/phase-ai/src/combat_ai.rs-222-258 (1)
222-258: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winAdd a topology guard before
adversarial_swarm_witness. The witness clones the state and explores blocker declarations, but it immediately returnsUnsupportedTopologyunless the game has exactly two non-team-based players. Guard those conditions before building the certification input. Use a damage precondition only with an engine-derived upper bound;twicethe current power is not always sound when declaration-time effects change power or keywords.🤖 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/combat_ai.rs` around lines 222 - 258, Add a topology guard before constructing certified_candidates or calling adversarial_swarm_witness, allowing certification only when the engine reports exactly two non-team-based players; otherwise preserve the existing non-certification flow. If adding a damage precondition, derive the upper bound from the engine’s authoritative combat calculation and do not use twice the current power, since declaration effects may change power or keywords.crates/engine-wasm/src/lib.rs-2013-2017 (1)
2013-2017: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winThis endpoint lost its
SessionCachereuse.
score_candidates_for_parallel_workerbuilds its own session internally (AiSession::arc_from_game(state)incrates/phase-ai/src/search.rs). This call site previously supplied the cached session throughai_session_for. Each invocation now runs a full per-player deck analysis:DeckProfile,DeckFeatures,SynergyGraph, and the derived plan snapshot.
get_ai_scored_candidatesis the parallel-worker scoring endpoint and is called once per determinized sample per worker, so the cost repeats on a hot path.crates/phase-ai/src/search.rsalready treats that analysis as expensive enough to avoid duplicating.Let the new function accept an optional caller-owned session, or keep using
ai_session_for(state)here and apply the Pact guard separately.🤖 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-wasm/src/lib.rs` around lines 2013 - 2017, Restore SessionCache reuse in the get_ai_scored_candidates endpoint by passing the caller-owned session from ai_session_for(state) into score_candidates_for_parallel_worker, updating that function to accept an optional session if needed. Preserve the existing parallel-worker scoring behavior while ensuring the per-player analysis is not rebuilt for every determinized sample.crates/phase-ai/src/planner/mod.rs-65-75 (1)
65-75: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy lift
unwrap_or(0.0)is not neutral for an absolute state evaluation.
continuation_witnessholds a state evaluation produced byservices.evaluate_state, so its scale is the evaluator's, not a delta around zero. Bothbeam_priorityandroot_scoreadd the raw witness and substitute0.0when no witness exists. Two consequences follow:
- A candidate whose witness evaluates negative ranks below an otherwise identical candidate that has no witness. Beam truncation can then drop the reducer-proven route, which the doc comment states must not happen.
- A candidate whose witness evaluates positive gains an unearned advantage over non-witnessed siblings, because the comparison baseline for those siblings is
0.0rather than their own continuation value.
run_iterative_deepeningalready handles this correctly withcont.max(witness), which compares two values on the same scale. Apply the same treatment at the ranking seam: keep the witness as a comparison floor against a scale-matched baseline instead of an additive term over an implicit zero.🤖 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/planner/mod.rs` around lines 65 - 75, Update PlannerState::beam_priority and PlannerState::root_score to compare continuation_witness with the existing continuation score using a scale-matched maximum, following the cont.max(witness) behavior in run_iterative_deepening. Remove the raw witness addition and unwrap_or(0.0) fallback while preserving tactical weighting in root_score and ensuring a witness acts only as a comparison floor.crates/engine/src/ai_support/prospective_mana.rs-459-506 (1)
459-506: 🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy liftBound the continuation branch of
advance_pact_to_install.The loop condition only counts
priority_beats, and thewaiting if waiting.acting_player() == Some(owner)arm never increments it. Each iteration of that arm applies oneTarget/Selectioncontinuation without consuming any budget. If the reducer keeps exposing exactly one continuation candidate (for example a repeat-style prompt that re-enters the same waiting shape), the loop does not terminate, and the AI decision thread hangs inside certification.The module already defines a decision-scoped allowance for this class of work. Consume one unit per continuation transition.
🔒 Proposed fix: charge continuation transitions against a bound
fn advance_pact_to_install( state: &mut GameState, owner: PlayerId, source_id: ObjectId, journal_start: usize, ) -> Option<PactReceipt> { let mut priority_beats = 0; - while priority_beats < PROSPECTIVE_MAX_PRIORITY_BEATS { + let mut continuations_remaining = PROSPECTIVE_MAX_FORCED_TRANSITIONS; + while priority_beats < PROSPECTIVE_MAX_PRIORITY_BEATS { @@ waiting if waiting.acting_player() == Some(owner) => { + if continuations_remaining == 0 { + return None; + } + continuations_remaining -= 1; let mut continuations =🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/prospective_mana.rs` around lines 459 - 506, Update the continuation arm in advance_pact_to_install so each applied Target/Selection continuation consumes one unit of the module’s existing decision-scoped allowance, alongside the priority-beat bound. Ensure repeated continuation prompts eventually terminate when that allowance is exhausted, while preserving the current candidate validation and application behavior.crates/engine/tests/integration/prospective_fetchland_mana.rs-1-23 (1)
1-23: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winAdd the missing card-data fixture
prospective_fetchland_manais registered, but neither referencedcard-data.jsonpath exists.full_card_db()will panic when the test runs. Add the fixture or update the path to an existing export.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/prospective_fetchland_mana.rs` around lines 1 - 23, Fix full_card_db() so it loads a valid existing card-data export: either add the missing card-data.json fixture at the path derived from CARGO_MANIFEST_DIR or update the Path construction to an existing export location, while preserving the CardDatabase::from_export initialization and test behavior.Source: Path instructions
crates/engine/src/types/game_state.rs-7919-7952 (1)
7919-7952: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winThe continuation detector misses continuations that serialize without
trigger_context.
PendingContinuation::trigger_contextuses#[serde(default, skip_serializing_if = "Option::is_none")](lines 2097-2098). A continuation parked with no trigger context therefore serializes withchainbut withouttrigger_context. The guard on line 7930 then never matches that frame, so its legacyfiring_classification/dispatch_originis neither converted nor removed. The doc comment on line 7921 states that a continuation always owns both keys; that is not true for the serialized form.Two failure modes follow. A legacy delayed classification on such a frame is silently dropped. If the restored state also carries
resolving_trigger_firing,validate_trigger_firing_coherencethen rejects the state with "paused trigger continuation has no firing carrier for the active resolving trigger".Detect the frame by
chainalone, or bychainplus any of the firing keys.🛠️ Proposed detector fix
- if object.contains_key("chain") && object.contains_key("trigger_context") { + // `trigger_context` is skipped when absent, so `chain` plus a + // firing key is the only reliable continuation signature. + if object.contains_key("chain") + && (object.contains_key("trigger_context") + || object.contains_key("trigger_firing") + || object.contains_key("firing_classification") + || object.contains_key("dispatch_origin")) + { let canonical = object.get("trigger_firing").cloned();🤖 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 7919 - 7952, Update migrate_legacy_continuation_firing to identify serialized continuation frames using the presence of chain alone, rather than requiring trigger_context. Keep the existing legacy firing_classification/dispatch_origin conversion, removal, and canonical conflict validation unchanged so continuations without trigger_context retain a valid trigger_firing carrier.crates/engine/src/types/game_state.rs-8198-8226 (1)
8198-8226: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftTwo identical legacy delayed triggers make restore fail.
delayed_trigger_payload_matchescompares the whole payload minus provenance. Two delayed triggers created by the same source with the same condition, ability, controller, andone_shotproduce byte-identical payloads. That state is legitimate: two resolutions of the same effect install two identical delayed triggers. In a legacy save neither record carries provenance and both install commands stay unbound, so each record sees two candidates and hits the_arm. The whole save then fails to load.Bind identical candidates deterministically instead. Take the first unbound match and mark it bound; only report an error when a record has no candidate at all and the ambiguity is genuinely unresolvable (for example, when the number of records exceeds the number of matching commands).
🛠️ Sketch: deterministic first-match binding
- match matches.as_slice() { - [index] => { + match matches.as_slice() { + // Identical payloads are indistinguishable by construction. + // Bind the lowest unbound command so the mapping is total and + // deterministic across peers rather than rejecting the save. + [index, ..] => { let provenance = migrated_commands[*index] .get("provenance") .cloned() .expect("migrated command always has private provenance"); delayed .as_object_mut() .expect("delayed trigger was validated as an object") .insert("provenance".to_string(), provenance); bound_commands.insert(*index); } [] => {} - _ => { - return Err( - "legacy delayed trigger matches multiple durable install commands" - .to_string(), - ); - } }🤖 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 8198 - 8226, Update the legacy delayed-trigger binding logic around delayed_trigger_payload_matches so each record selects and binds the first unbound matching migrated command deterministically instead of rejecting multiple matches. Track matching records or otherwise ensure ambiguity is reported only when no candidate remains, including when delayed records outnumber matching commands; preserve provenance insertion and bound_commands updates for successful bindings.crates/engine/src/types/resolution.rs-2482-2486 (1)
2482-2486: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftRebind legacy stack and resolving trigger carriers after provenance migration.
migrate_legacy_delayed_trigger_provenanceupdates install commands and live records, but not legacy carriers. A saved state whose delayed record leftdelayed_triggerscan therefore failvalidate_trigger_firing_coherencewith “has no install root”. Rebind both carriers to the migrated provenance.🤖 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/resolution.rs` around lines 2482 - 2486, Update the resolution-state migration around migrate_legacy_delayed_trigger_provenance to rebind both legacy stack and resolving trigger firing carriers after provenance migration, before validate_trigger_firing_coherence runs. Ensure the carriers reference the migrated install provenance so delayed records moved from delayed_triggers retain a valid install root.crates/engine/src/types/game_state.rs-5333-5358 (1)
5333-5358: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winReplace the three integration-test
DelayedTriggerliterals. The privateprovenancefield prevents these external-crate literals from compiling. UseDelayedTrigger::new(...)incrates/engine/tests/integration/lose_control_this_turn_delayed_trigger.rs,breeches_blastmaker_coin_flip_copy.rs, andmechtitan_core_return_exiled.rs.🤖 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 5333 - 5358, Replace the three integration-test DelayedTrigger struct literals in lose_control_this_turn_delayed_trigger.rs, breeches_blastmaker_coin_flip_copy.rs, and mechtitan_core_return_exiled.rs with DelayedTrigger::new(...), passing each literal’s existing condition, ability, controller, source_id, and one_shot values; do not construct the private provenance field externally.crates/engine/src/types/resolved_commands.rs-1046-1049 (1)
1046-1049: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftAdd a migration for legacy
ResolvedRulesCommand::StackPushentries.
GameStatepersists the journal, but the trigger-carrier migration does not process stack-push commands. Serde suppliesNonefor the missing field, andapply_resolved_stack_pushrejects historical triggered entries withTriggerFiringShapeMismatch.Migrate these commands or handle missing classifications without rejecting replay.
🤖 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/resolved_commands.rs` around lines 1046 - 1049, Update the legacy deserialization/replay migration for ResolvedRulesCommand::StackPush so entries missing trigger_firing are handled compatibly instead of being rejected by apply_resolved_stack_push with TriggerFiringShapeMismatch. Ensure the migration derives and attaches the appropriate TriggerFiring classification for historical triggered stack pushes, while preserving None for genuinely untriggered commands.crates/engine/src/types/game_state.rs-19173-19177 (1)
19173-19177: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winInclude
stack_trigger_firingsin loop-state equality, or document and test its exclusion.
normalize_for_loopretains this map, andTriggerFiring::Delayeddiffers fromTriggerFiring::Ordinary. States with identical stack entries but different firing classifications therefore compare equal throughloop_states_equal, which can cause a false CR 104.4b repeat and affect equality-based AI deduplication.🤖 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 19173 - 19177, Update the loop-state comparison around normalize_for_loop and loop_states_equal to include stack_trigger_firings in equality, preserving the distinction between TriggerFiring::Delayed and TriggerFiring::Ordinary. If exclusion is intentional instead, document it and add tests covering differing firing classifications; otherwise ensure such states no longer compare equal.crates/engine/src/game/triggers.rs-7245-7267 (1)
7245-7267: 🚀 Performance & Scalability | 🟠 Major | 🏗️ Heavy liftThe full
GameStateclone runs on every targeted trigger dispatch.
prepared_state = state.clone()executes before the code determines whether any mutation is required.auto_select_targets_for_abilitytakes&prepared_stateandassign_targets_in_chaintakes&prepared_state, so only theTargetSelectionMode::Randompath needs a mutable state. Trigger dispatch is a hot path — a board with many simultaneous triggers clones the whole state once per trigger.Clone only for the random branch, and keep the deterministic branch on
state.♻️ Restrict the clone to the RNG-advancing branch
- let mut prepared_state = state.clone(); - let mut prepared_trigger = trigger.clone(); - let auto_targets = if matches!( - prepared_trigger.ability.target_selection_mode, - crate::types::ability::TargetSelectionMode::Random - ) { - super::ability_utils::random_select_targets_for_ability( - &mut prepared_state, - &target_slots, - &prepared_trigger.target_constraints, - ) - .map(Some) - } else { - super::ability_utils::auto_select_targets_for_ability( - &prepared_state, - &prepared_trigger.ability, - &target_slots, - &prepared_trigger.target_constraints, - ) - }; + let mut prepared_trigger = trigger.clone(); + let random_selection = matches!( + prepared_trigger.ability.target_selection_mode, + crate::types::ability::TargetSelectionMode::Random + ); + // Only random selection advances the engine RNG, so only it needs a + // throwaway state. + let mut prepared_state = if random_selection { + state.clone() + } else { + // Borrow the live state read-only for the deterministic path. + state.clone_shallow_for_target_preparation() + };An alternative is to keep
statefor the deterministic path and introduce a smallPreparedRngcarrier that only the random path commits.🤖 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/triggers.rs` around lines 7245 - 7267, Move the prepared_state clone out of the common trigger-dispatch path and create it only inside the TargetSelectionMode::Random branch before random_select_targets_for_ability. Keep auto_select_targets_for_ability and subsequent deterministic target assignment using the original state reference, while preserving the random branch’s transactional state handling.crates/engine/src/game/triggers.rs-315-320 (1)
315-320: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
#[serde(default)]onfiringdrops delayed identity for previously serialized contexts.
dispatch_originis retained for wire compatibility, butfiringdefaults independently. A payload written before this change containsdispatch_origin: "Delayed"and nofiringfield. After deserialization the context becomesTriggerFiring::Ordinary. Two behaviors then change for that context:apply_trigger_doublingdoubles it (line 7619), anddispatch_pending_trigger_context_with_originstops applying theDroppedTargetUnresolved→Pushedfallback (line 7012).Derive
firingfromdispatch_originwhen the field is absent.🛡️ Derive the default from the legacy field
- #[serde(default)] + #[serde(default = "TriggerFiring::default")] pub(crate) firing: TriggerFiring,Then add a
#[serde(deserialize_with = ...)]or a post-deserialize normalization that mapsdispatch_origin == Delayedwith a defaultedfiringtoTriggerFiring::Delayed(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/game/triggers.rs` around lines 315 - 320, Update deserialization for the scheduler carrier containing `firing` so legacy payloads without that field derive `TriggerFiring::Delayed(None)` when `dispatch_origin` is `Delayed`, while preserving explicit `firing` values and ordinary behavior for other origins. Use a serde deserializer or post-deserialization normalization near the `firing` field and retain compatibility with existing `PendingTrigger` literals.crates/engine/src/game/triggers.rs-5795-5795 (1)
5795-5795: 🎯 Functional Correctness | 🟠 Major | 🏗️ Heavy liftDelayed triggers can no longer auto-order.
TriggerFiring::Delayed(Some(provenance))carries the uniquetokenandinstance, so two delayed triggers never compare equal. The previousdispatch_origincomparison treated two delayed firings as equal. With this change,group_is_order_independentreturnsfalsefor every group that contains two delayed triggers, and CR 603.3b auto-ordering degrades into a mandatoryOrderTriggersprompt.A concrete case: two Dash permanents create identical "return this permanent to its owner's hand at the beginning of the next end step" delayed triggers. Both are no-input triggers with byte-identical normalized abilities, so they must auto-order. They now prompt.
Compare the dispatch class, not the individual identity.
♻️ Compare only the ordinary-versus-delayed class
- ctx.firing() == first.firing() + ctx.firing().is_delayed() == first.firing().is_delayed()🤖 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/triggers.rs` at line 5795, Update the comparison in group_is_order_independent to compare each firing’s dispatch class—ordinary versus delayed—instead of full TriggerFiring identity or dispatch_origin. Preserve auto-ordering for groups containing equivalent delayed triggers while still distinguishing ordinary and delayed firings.crates/engine/src/game/triggers.rs-893-947 (1)
893-947: 🚀 Performance & Scalability | 🟠 Major | ⚡ Quick winThe duplicate-provenance scan is quadratic in installs per game.
Each install walks the whole
resolved_rules_journaltwice and the wholedelayed_triggersvector twice. The journal grows monotonically, so total cost grows as O(n²) in the number of delayed-trigger installs. Delayed triggers are created by common keywords (Dash, Blitz, Decayed, end-step sacrifice riders), so a long game accumulates many entries.Track the issued tokens and instances in two sets on
GameState, or perform a single journal pass that checks both keys.♻️ Single journal pass for both keys
- let journal_has = |matches: &dyn Fn(DelayedTriggerProvenance) -> bool| { - state - .resolved_rules_journal - .entries() - .iter() - .filter_map(|entry| entry.command.as_ref()) - .filter_map(|command| match command { - crate::types::resolved_commands::ResolvedRulesCommand::DelayedTriggerInstall( - command, - ) => command.trigger.provenance, - _ => None, - }) - .any(matches) - }; - if journal_has(&|installed| installed.token == provenance.token) { - return Err( - ResolvedDelayedTriggerReplayInvariantError::DuplicateProvenanceToken { - token: provenance.token, - }, - ); - } - if journal_has(&|installed| installed.instance == provenance.instance) { - return Err( - ResolvedDelayedTriggerReplayInvariantError::DuplicateProvenanceInstance { - instance: provenance.instance, - }, - ); - } + let installed_provenances = state + .resolved_rules_journal + .entries() + .iter() + .filter_map(|entry| entry.command.as_ref()) + .filter_map(|command| match command { + crate::types::resolved_commands::ResolvedRulesCommand::DelayedTriggerInstall( + command, + ) => command.trigger.provenance, + _ => None, + }) + .chain(state.delayed_triggers.iter().filter_map(|trigger| trigger.provenance)); + for installed in installed_provenances { + if installed.token == provenance.token { + return Err( + ResolvedDelayedTriggerReplayInvariantError::DuplicateProvenanceToken { + token: provenance.token, + }, + ); + } + if installed.instance == provenance.instance { + return Err( + ResolvedDelayedTriggerReplayInvariantError::DuplicateProvenanceInstance { + instance: provenance.instance, + }, + ); + } + }This also removes the two separate
delayed_triggersscans below.🤖 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/triggers.rs` around lines 893 - 947, Optimize the duplicate-provenance validation around the replay/install logic by scanning the resolved-rules journal and active delayed_triggers once each, checking both token and instance in those passes. Preserve the existing DuplicateProvenanceToken and DuplicateProvenanceInstance errors, and avoid repeating full-collection scans for each key.crates/engine/src/game/triggers.rs-821-825 (1)
821-825: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winFix the remaining
install_delayed_triggercall sites.Five callers still pass only two arguments:
crates/engine/src/game/effects/{rebound,counters,token,myriad}.rs. Passeventsor remove the unused parameter.blitz.rsanddash.rsalready pass three arguments.🤖 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/triggers.rs` around lines 821 - 825, Update all remaining install_delayed_trigger call sites in rebound.rs, counters.rs, token.rs, and myriad.rs to match the three-argument signature by passing the available events collection, or remove the unused _events parameter from install_delayed_trigger and adjust every caller consistently; preserve the already-correct blitz.rs and dash.rs usage.crates/engine/src/game/stack.rs-41-60 (1)
41-60: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winPreserve delayed-trigger provenance across copy and batch resolution.
push_copy_to_stackrecords every copiedTriggeredAbilityasTriggerFiring::Ordinary, althoughCopySpellcan copy anyStackAbility, including delayed triggers. Propagate the source entry’s firing classification.resolve_batcheddiscards each popped entry’s firing classification beforeplan.execute. A batch of delayedEffect::Tokentriggers therefore clearsresolving_trigger_firing, causing the Pact tracker to report the obligation as expired. Preserve the batch firing during execution.🤖 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/stack.rs` around lines 41 - 60, Preserve TriggerFiring provenance for delayed triggers in both copy and batch resolution. Update push_copy_to_stack to reuse the source StackEntry’s firing classification when copying TriggeredAbility entries instead of forcing Ordinary. Update resolve_batched to retain each popped entry’s firing value through plan.execute so delayed Effect::Token triggers keep resolving_trigger_firing available to the Pact tracker.
🟡 Minor comments (7)
crates/engine/src/game/casting.rs-10363-10366 (1)
10363-10366: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winUse
object.ownerfor hand-zone authentication.
move_to_zonepreservescontroller, so a permanent returned to its owner’s hand can retaincontroller != owner. This guard can then hide the owner’s legal Evoke choice while normal casting remains available. Compareobject.ownerforZone::Hand(CR 404.2), or resetcontrollerwhen the object leaves the battlefield.🤖 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/casting.rs` around lines 10363 - 10366, Update the hand-zone authentication guard in the relevant casting function to compare the object’s owner with player instead of its controller when validating Zone::Hand cards, while retaining the card_id and zone checks.Source: Path instructions
crates/engine/src/ai_support/evoke.rs-56-59 (1)
56-59: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winReplace
CR 115.1dwith citations for target legality and opponent identification.CR 115.1donly defines when a triggered ability is targeted. It does not define legal target enumeration or opponent identification. UseCR 115.2and the applicable opponent rule.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/evoke.rs` around lines 56 - 59, Update the documentation comment above the Evoke handling to replace CR 115.1d with CR 115.2 for target legality and add the applicable opponent-identification rule citation, while preserving the existing CR 603.6a and Evoke behavior descriptions.Source: Path instructions
crates/engine/src/game/derived_views.rs-2755-2797 (1)
2755-2797: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winAdd a deserialize assertion for the redacted payload.
The test proves the private fields are absent. It does not prove the client can still consume the payload.
client_state_wire_valueremoves keys from a serializedGameState, soClientGameStatedeserialization now depends on every removed field having a serde default. No other test in this module populates those fields, so a missing default would fail only in real games that carry delayed triggers.Assert the round-trip inside the same loop.
💚 Proposed test addition
for client_state in [&client["state"], &filtered_client["state"]] { + serde_json::from_value::<crate::types::game_state::GameState>(client_state.clone()) + .expect("redacted client state must still deserialize"); for private_field in [🤖 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/derived_views.rs` around lines 2755 - 2797, Within the loop over client and filtered client state in the relevant test, deserialize each redacted client_state using the existing ClientGameState type and assert that it succeeds. Place this round-trip assertion after client_state_wire_value removes private fields, preserving the existing omission checks while verifying all removed fields have serde defaults.crates/engine/src/game/life_safety.rs-119-134 (1)
119-134: 🚀 Performance & Scalability | 🟡 Minor | ⚡ Quick winReuse the generated candidate set during life-safety validation.
is_complete_generated_candidateregenerates all candidates for each preview and again at the cost authority.candidate_actionsdoes not calllife_safety, so recursion is not present. Pass the validated candidate provenance through the probe and reuse it instead of repeating full enumeration on the AI path.🤖 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/life_safety.rs` around lines 119 - 134, Update the life-safety probe flow around is_complete_generated_candidate to accept the already validated generated-candidate provenance and reuse it during validation. Remove the repeated crate::ai_support::candidate_actions enumeration from this function, while preserving the semantic_owner, actor, action, and tactical_class matching checks against the passed candidate set. Ensure the same provenance is forwarded through the preview and cost-authority paths.Source: Path instructions
crates/engine/src/ai_support/swarm.rs-296-304 (1)
296-304: 🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick winA missing object silently shortens
worst_declarationinstead of abstaining.The
filter_mapwith?drops any pair whose blocker or attacker is not found inafter_attack.objects. The certificate then reports a block declaration that is shorter than the one the reducer replayed. Every other lookup failure in this file returnsIndeterminate. Keep that fail-closed shape here.🛡️ Proposed fail-closed collection
- let bound_declaration = declaration + let Some(bound_declaration) = declaration .iter() - .filter_map(|(blocker, attacker)| { + .map(|(blocker, attacker)| { Some(( ObjectIncarnationRef::from_object(after_attack.objects.get(blocker)?), ObjectIncarnationRef::from_object(after_attack.objects.get(attacker)?), )) }) - .collect(); + .collect::<Option<Vec<_>>>() + else { + failure = Some(SwarmWitnessIndeterminate::InvalidAttack); + return ControlFlow::Break(()); + };🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/swarm.rs` around lines 296 - 304, Update the collection building worst_declaration so a missing blocker or attacker does not get silently filtered out. Replace the filter_map/? path with fail-closed handling that returns Indeterminate when any lookup in after_attack.objects fails, while preserving the complete declaration when all ObjectIncarnationRef conversions succeed.crates/engine/tests/integration/swarm_combat_witness.rs-217-223 (1)
217-223: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winNo test reaches the empty-
attacksguard.This sub-case passes an empty attack list, but
swarm_witness_innerreturnsInvalidAttackat theWaitingFor::DeclareAttackerscheck first, because the fixture stays inPreCombatMain. Theattacks.is_empty()condition incrates/engine/src/ai_support/swarm.rsline 131 is never exercised. Add a case that advances to combat and then passes&[], so the guard has a reach-proving test.As per path instructions: "For every negative assertion ... require a paired positive reach-guard proving the input actually reached the code under test."
💚 Proposed added reach-guard case
let mut precombat = GameScenario::new(); precombat.at_phase(Phase::PreCombatMain); let precombat = precombat.build(); assert_eq!( adversarial_swarm_witness(precombat.state(), P0, &[]), SwarmWitnessResult::Indeterminate(SwarmWitnessIndeterminate::InvalidAttack) ); + + // Reach guard: in the declare-attackers step, an empty declaration is + // rejected by the `attacks.is_empty()` branch, not by the step check. + let mut empty_declaration = GameScenario::new(); + empty_declaration.at_phase(Phase::PreCombatMain); + empty_declaration.add_creature(P0, "Bear", 3, 3); + let mut empty_declaration = empty_declaration.build(); + empty_declaration.advance_to_combat(); + assert!(matches!( + empty_declaration.state().waiting_for, + engine::types::game_state::WaitingFor::DeclareAttackers { .. } + )); + assert_eq!( + adversarial_swarm_witness(empty_declaration.state(), P0, &[]), + SwarmWitnessResult::Indeterminate(SwarmWitnessIndeterminate::InvalidAttack) + );🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/swarm_combat_witness.rs` around lines 217 - 223, Update the test around adversarial_swarm_witness to add a paired case that advances the scenario into combat, reaches the DeclareAttackers state, and passes an empty attack slice so swarm_witness_inner evaluates its attacks.is_empty() guard. Keep the existing PreCombatMain InvalidAttack assertion, and assert the combat-phase case produces the guard’s expected result.Source: Path instructions
crates/engine/src/ai_support/swarm.rs-191-196 (1)
191-196: 📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick winCorrect the combat-damage rule citation and claim. CR 614.1a defines replacement effects; it does not establish a combat-damage event before blockers are declared. Cite CR 510.1 for combat-damage assignment and retain CR 614.1a only for the replacement-effect rationale. Update the repeated citations in
crates/engine/tests/integration/swarm_combat_witness.rs.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/swarm.rs` around lines 191 - 196, The comment above the early return in the combat-damage replacement check incorrectly attributes event timing to CR 614.1a; cite CR 510.1 for combat-damage assignment and retain CR 614.1a only to explain the replacement-effect rationale. Apply the same citation and claim correction to the repeated comments in the swarm combat witness integration tests.Source: Learnings
matthewevans
force-pushed
the
ship/ai-reliability-fixes
branch
from
July 31, 2026 22:11
cbb5967 to
74a6f4c
Compare
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
force-pushed
the
ship/ai-reliability-fixes
branch
from
July 31, 2026 22:34
74a6f4c to
d8eb0ad
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 10
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/types/game_state.rs (1)
18392-18446: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winCanonicalize the trigger-firing carriers in
normalize_for_loop.This function already zeroes both delayed-trigger allocators and clears
provenancefrom every live delayed trigger, because a per-iteration identity would defeat CR 104.4b comparison. The new firing carriers keep that identity:
pending_trigger_firingholdsTriggerFiring::Delayed(Some(provenance))and is compared inimpl PartialEq for GameState(Line 19302).stack_trigger_firingsholds the same payload and is compared inloop_states_equal(Line 18839).A loop that re-installs a delayed trigger each iteration mints a new token and instance, so those carriers differ between iterations. The two normalized states then never compare equal, and the mandatory-loop draw is missed. This is the same failure mode the file documents for
resolution_source_relatch.Map
Delayed(Some(_))toDelayed(None)for the pending, stack, resolving, and parked-continuation carriers while normalizing.🛠️ Proposed fix sketch
for dt in clone.delayed_triggers.iter_mut() { dt.ability.clear_trigger_identity_recursive(); dt.provenance = None; } + // CR 104.4b: a per-iteration delayed-trigger identity must not enter + // loop equality through a firing carrier either. + let canonicalize = |firing: &mut TriggerFiring| { + if matches!(firing, TriggerFiring::Delayed(Some(_))) { + *firing = TriggerFiring::Delayed(None); + } + }; + if let Some(firing) = clone.pending_trigger_firing.as_mut() { + canonicalize(firing); + } + if let Some(firing) = clone.resolving_trigger_firing.as_mut() { + canonicalize(firing); + } + for firing in clone.stack_trigger_firings.values_mut() { + canonicalize(firing); + }🤖 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 18392 - 18446, Update normalize_for_loop to canonicalize delayed-trigger provenance in all trigger-firing carriers: pending_trigger_firing, stack_trigger_firings, resolving, and parked-continuation state. Traverse each carrier and convert TriggerFiring::Delayed(Some(_)) to TriggerFiring::Delayed(None), preserving other TriggerFiring variants and existing normalization behavior so loop comparisons ignore per-iteration delayed-trigger identity.
🧹 Nitpick comments (6)
crates/engine/src/ai_support/swarm.rs (1)
240-242: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winBound the reducer work, not only the declaration count.
checked_declaration_productpermits up toSWARM_WITNESS_MAX_DECLARATIONS(4096) leaves. Each legal leaf clones the wholeGameStateat line 268 and then drives the reducer through damage completion.combat_ai::choose_attackers_with_targets_with_profilecalls this witness on every attack decision, so a wide board can cost thousands of full state clones plus reducer runs per decision.Two low-risk mitigations: add an early exit when
life_loss == 0(no defense can do better for the attacker), and cap the number of executed candidate branches separately from the enumeration cap so the worst case is a fixed reducer budget.♻️ Proposed early exit
if worst .as_ref() .is_none_or(|(least_loss, _)| life_loss < *least_loss) { worst = Some((life_loss, bound_declaration)); } + if matches!(worst.as_ref(), Some((0, _))) { + // Zero life loss is the minimum; no other declaration can beat it. + return ControlFlow::Break(()); + } ControlFlow::Continue(())Note: an early
Breakmust not be confused with thefailurepath;failurestaysNonehere, so the existingif let Some(reason) = failurecheck still passes.Also applies to: 268-268
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/ai_support/swarm.rs` around lines 240 - 242, Update the witness reducer loop around checked_declaration_product so it exits immediately when life_loss reaches zero, preserving failure as None so normal success handling remains unchanged. Add a separate fixed cap on executed candidate branches, distinct from SWARM_WITNESS_MAX_DECLARATIONS, and stop processing further leaves once that reducer budget is reached; apply the cap at the point where each candidate clones GameState and runs the reducer.crates/phase-ai/src/policies/mulligan/mod.rs (1)
100-106: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winModel the probe outcome with a typed shape instead of a
probedflag plus three witness bools.
probedencodes "no result" in the same struct as the results, soSelf::default()is both "not probed" and "no witnesses found". A caller that forgetsprobedreads an unprobed hand as dead. ReturnOption<OpeningHandActionForecast>(or a two-variant enumInconclusive/Probed { .. }) and keep only the witness fields inside the probed variant.Based on path instructions: "any new
boolstruct field orboolvariant payload where a typed enum ... would carry the same information with more meaning".Also applies to: 173-178
🤖 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/mulligan/mod.rs` around lines 100 - 106, Replace OpeningHandActionForecast’s probed flag plus witness booleans with an explicit optional or two-variant typed result, keeping only the witness fields in the probed value. Update the probe-producing and consuming logic around OpeningHandActionForecast so an unprobed hand is represented as absent/inconclusive and cannot be interpreted as a probed result with all witnesses false.Source: Path instructions
crates/engine/src/types/game_state.rs (4)
18838-18840: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument or align where
stack_trigger_firingsis compared.
loop_states_equalnow comparesstack_trigger_firings, butimpl PartialEq for GameStatedoes not.stackitself is compared inPartialEq. Two states that differ only in one stack entry's firing classification therefore compare equal for AI-search dedup and for every other==consumer, whileloop_states_equaltreats them as distinct.Either add the field to the
PartialEqchain besidepending_trigger_firing, or state in the field doc why the comparison belongs only to loop identity.🤖 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 18838 - 18840, Align GameState equality with loop_states_equal by adding stack_trigger_firings to the PartialEq comparison chain alongside pending_trigger_firing and the existing stack comparison. Ensure states differing only in firing classification compare as distinct for all == consumers.
8203-8224: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winAvoid the per-pair deep clone in the legacy binding scan.
delayed_trigger_payload_matchesclones both JSON values on every comparison, and this loop compares each provenance-less live delayed trigger against every migrated install command. Each clone copies a full resolved-ability tree. The cost islive × commandsdeep clones on the restore path.Strip
provenanceonce per command trigger and once per live record before the scan, then compare the stripped values.🤖 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 8203 - 8224, Update the legacy binding scan around delayed_trigger_payload_matches to avoid cloning JSON values for every live-trigger/command pair. Before the nested comparisons, create provenance-stripped payloads once for each migrated command trigger and each live delayed record, then compare those stripped values while preserving the existing bound_commands selection and provenance insertion behavior.
7541-7565: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winReplace the linear reuse scans with dedicated index sets.
register_rootscansroots.keys()twice for every registration. The function registers one root perDelayedTriggerInstalljournal entry plus one per live delayed trigger, so restore cost is quadratic in the number of delayed installations recorded for the game. The journal is append-only, so a long game pays this on every restore.Track tokens and instances in separate
BTreeSet<u64>values and query them directly.♻️ Proposed refactor sketch
- let mut roots = BTreeMap::<(u64, u64), ObjectId>::new(); + let mut roots = BTreeMap::<(u64, u64), ObjectId>::new(); + let mut used_tokens = BTreeSet::<u64>::new(); + let mut used_instances = BTreeSet::<u64>::new(); @@ - if roots.keys().any(|(token, _)| *token == provenance.token.0) { + if used_tokens.contains(&provenance.token.0) { return Err(format!( "{carrier} delayed-trigger provenance reuses a token" )); } - if roots - .keys() - .any(|(_, instance)| *instance == provenance.instance.0) - { + if used_instances.contains(&provenance.instance.0) { return Err(format!( "{carrier} delayed-trigger provenance reuses an instance" )); } roots.insert(key, source_id); + used_tokens.insert(provenance.token.0); + used_instances.insert(provenance.instance.0);🤖 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 7541 - 7565, Update the register_root logic to maintain dedicated BTreeSet<u64> indexes for provenance tokens and instances alongside roots. Replace both roots.keys().any(...) reuse scans with direct set membership checks, and insert each token and instance into the sets only after validation succeeds; preserve existing alias and error behavior.
20044-20064: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a failure-path test for the allocator bound checks.
validate_trigger_firing_coherencerejects a state whosenext_delayed_trigger_tokenornext_delayed_trigger_instanceis not above every install root (Lines 7732-7739). The new tests cover migration normalization of those allocators, but no test drives the validator's rejection. A regression that drops either bound check would stay green.Add a case that installs one delayed trigger, lowers each allocator to the root value, and asserts the restore error text for both fields.
As per path instructions: "A test must exercise the FAILURE path the fix prevents".
🤖 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 20044 - 20064, The migration tests lack coverage for validator rejection when delayed-trigger allocators are not above an install root. Extend the relevant delayed-trigger test coverage around validate_trigger_firing_coherence to install one delayed trigger, set next_delayed_trigger_token and next_delayed_trigger_instance to the root value in separate cases, and assert each restore failure contains the corresponding field’s error text.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/ai_support/swarm.rs`:
- Around line 291-297: Clamp the computed life loss to zero before converting it
to u32 in the damage-resolution flow around advance_to_damage_completion. Update
the life_loss calculation using the existing before/after life values so
defender life gains produce zero loss, while actual losses retain their correct
magnitude and downstream lethal checks remain valid.
In `@crates/phase-ai/src/combat_ai.rs`:
- Around line 3637-3640: Update both abstention tests around
adversarial_swarm_witness to match the specific
SwarmWitnessIndeterminate::DamageChoice reason inside
SwarmWitnessResult::Indeterminate, using the re-export from engine::ai_support.
Preserve the existing assertions that the result is indeterminate while
requiring DamageChoice at both test locations.
In `@crates/phase-ai/src/policies/mulligan/mod.rs`:
- Around line 181-197: Update is_normal_opening_hand_action to match every
GameAction variant explicitly, preserving hand.contains checks for the listed
hand-object actions and returning false through named arms for all
non-opening-action variants. Remove the wildcard fallback so adding a new
GameAction variant causes a compiler error until it is classified.
- Around line 122-164: The mulligan forecast loop around legal_actions_full
currently probes equivalent unadvanced state clones twice. Either advance the
cloned game state through the engine between first_turn and first_turn +
player_count so the second window reflects turn progression and resources, or
reduce the logic to one probe and update its comment to describe a single-window
model; keep the forecast flags consistent with the chosen behavior.
- Around line 891-916: Strengthen
forecast_rejects_unaffordable_or_targetless_opening_actions with positive
reach-guards for both fixtures. For the activation case, change only the
rejected affordability condition by providing sufficient mana or using
AbilityCost::Tap, then assert is_certified_dead_landless() is false; for the
spell case, add a battlefield creature while keeping the spell otherwise
unchanged, then assert the forecast is not certified dead. Keep each control
paired with its existing negative assertion to verify the code reaches legal
action evaluation.
- Around line 109-115: Update `Mulligan::for_hand` to attribute hand objects
using each object's `owner`, never `controller`, when selecting the player for
the forecast. Ensure the hand is validated as belonging to one consistent owner
rather than binding to an arbitrary object from a mixed-ownership slice;
preserve the default result when no valid owner can be established.
- Around line 118-121: Guard the zero-player case in the function containing the
first_turn calculation before evaluating the modulo, returning the default
forecast for an empty player list. Preserve the existing calculation and
WaitingFor::Priority behavior when player_count is nonzero, and continue using
the GameObject controller field as required by the probe.
In `@crates/phase-ai/src/policies/self_cost.rs`:
- Around line 555-573: Replace the boolean
residual_effects_are_trivial_at_x_zero result with a three-state ResidualVerdict
that distinguishes TrivialAtXZero, MeaningfulAtXZero, and Unknown. Map
classify_effects results accordingly, treating Unmodeled as Unknown rather than
trivial, and update x_cast_gate to abstain whenever the residual verdict is
Unknown.
- Around line 471-480: Update effect_benefit_value’s AI-only Effect::Draw
handling so chained draws are not independently previewed against the unchanged
base state. Prefer previewing the complete chain against evolving state, or
conservatively return None when more than one AI-only draw exists; ensure
pricing does not count undeliverable duplicate previews.
- Around line 714-720: Update the opponent-only Draw arm in the Effect
triviality handling to credit remaining_opponent_draw_churn only when the draw
count expression is mandatory, checking count.is_up_to() before resolving and
adding the count. Preserve the existing scalar budget semantics and verify that
resolve_quantity supplies a per-player count rather than an aggregate; adjust
only if needed to prevent over-crediting across multiple opponents.
---
Outside diff comments:
In `@crates/engine/src/types/game_state.rs`:
- Around line 18392-18446: Update normalize_for_loop to canonicalize
delayed-trigger provenance in all trigger-firing carriers:
pending_trigger_firing, stack_trigger_firings, resolving, and
parked-continuation state. Traverse each carrier and convert
TriggerFiring::Delayed(Some(_)) to TriggerFiring::Delayed(None), preserving
other TriggerFiring variants and existing normalization behavior so loop
comparisons ignore per-iteration delayed-trigger identity.
---
Nitpick comments:
In `@crates/engine/src/ai_support/swarm.rs`:
- Around line 240-242: Update the witness reducer loop around
checked_declaration_product so it exits immediately when life_loss reaches zero,
preserving failure as None so normal success handling remains unchanged. Add a
separate fixed cap on executed candidate branches, distinct from
SWARM_WITNESS_MAX_DECLARATIONS, and stop processing further leaves once that
reducer budget is reached; apply the cap at the point where each candidate
clones GameState and runs the reducer.
In `@crates/engine/src/types/game_state.rs`:
- Around line 18838-18840: Align GameState equality with loop_states_equal by
adding stack_trigger_firings to the PartialEq comparison chain alongside
pending_trigger_firing and the existing stack comparison. Ensure states
differing only in firing classification compare as distinct for all ==
consumers.
- Around line 8203-8224: Update the legacy binding scan around
delayed_trigger_payload_matches to avoid cloning JSON values for every
live-trigger/command pair. Before the nested comparisons, create
provenance-stripped payloads once for each migrated command trigger and each
live delayed record, then compare those stripped values while preserving the
existing bound_commands selection and provenance insertion behavior.
- Around line 7541-7565: Update the register_root logic to maintain dedicated
BTreeSet<u64> indexes for provenance tokens and instances alongside roots.
Replace both roots.keys().any(...) reuse scans with direct set membership
checks, and insert each token and instance into the sets only after validation
succeeds; preserve existing alias and error behavior.
- Around line 20044-20064: The migration tests lack coverage for validator
rejection when delayed-trigger allocators are not above an install root. Extend
the relevant delayed-trigger test coverage around
validate_trigger_firing_coherence to install one delayed trigger, set
next_delayed_trigger_token and next_delayed_trigger_instance to the root value
in separate cases, and assert each restore failure contains the corresponding
field’s error text.
In `@crates/phase-ai/src/policies/mulligan/mod.rs`:
- Around line 100-106: Replace OpeningHandActionForecast’s probed flag plus
witness booleans with an explicit optional or two-variant typed result, keeping
only the witness fields in the probed value. Update the probe-producing and
consuming logic around OpeningHandActionForecast so an unprobed hand is
represented as absent/inconclusive and cannot be interpreted as a probed result
with all witnesses false.
🪄 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: 6a0123bd-236a-4300-b5c1-b116d7eada8c
📒 Files selected for processing (72)
crates/engine-wasm/src/lib.rscrates/engine/src/ai_support/evoke.rscrates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/prospective_mana.rscrates/engine/src/ai_support/swarm.rscrates/engine/src/game/archenemy_tests.rscrates/engine/src/game/blitz.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/cipher.rscrates/engine/src/game/dash.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/effects/cast_copy_of_card.rscrates/engine/src/game/effects/copy_spell.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/draw.rscrates/engine/src/game/effects/encore.rscrates/engine/src/game/effects/end_phase.rscrates/engine/src/game/effects/epic.rscrates/engine/src/game/effects/exile_resolving_spell.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/myriad.rscrates/engine/src/game/effects/paradigm.rscrates/engine/src/game/effects/phase_out.rscrates/engine/src/game/effects/rebound.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/elimination.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_exile_return_tests.rscrates/engine/src/game/engine_keyword_action_stack_tests.rscrates/engine/src/game/engine_modes.rscrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/engine_priority.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/life_safety.rscrates/engine/src/game/mod.rscrates/engine/src/game/stack.rscrates/engine/src/game/static_abilities.rscrates/engine/src/game/triggers.rscrates/engine/src/game/triggers_pr7_order_template_tests.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/src/types/identifiers.rscrates/engine/src/types/resolution.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rscrates/engine/tests/integration/cr733_resolved_modifier_install.rscrates/engine/tests/integration/cr733_resolved_trigger_collection.rscrates/engine/tests/integration/draw_delivery_preview.rscrates/engine/tests/integration/lose_control_this_turn_delayed_trigger.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/mechtitan_core_return_exiled.rscrates/engine/tests/integration/pact_of_negation_upkeep_payment.rscrates/engine/tests/integration/prospective_fetchland_mana.rscrates/engine/tests/integration/swarm_combat_witness.rscrates/phase-ai/src/combat_ai.rscrates/phase-ai/src/lib.rscrates/phase-ai/src/planner/mod.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/effect_classify.rscrates/phase-ai/src/policies/mulligan/card_floor.rscrates/phase-ai/src/policies/mulligan/keepables_by_land_count.rscrates/phase-ai/src/policies/mulligan/mod.rscrates/phase-ai/src/policies/self_cost.rscrates/phase-ai/src/policies/self_cost_value.rscrates/phase-ai/src/policies/x_cast_gate.rscrates/phase-ai/src/search.rscrates/phase-ai/src/session.rscrates/phase-ai/tests/cedh_integration.rscrates/server-core/src/filter.rs
💤 Files with no reviewable changes (1)
- crates/phase-ai/src/policies/mulligan/keepables_by_land_count.rs
🚧 Files skipped from review as they are similar to previous changes (46)
- crates/engine/src/game/mod.rs
- crates/engine/src/game/static_abilities.rs
- crates/phase-ai/src/lib.rs
- crates/engine/src/game/engine_resolution_choices.rs
- crates/engine/src/game/triggers_pr7_order_template_tests.rs
- crates/engine/src/game/archenemy_tests.rs
- crates/engine/tests/integration/prospective_fetchland_mana.rs
- crates/server-core/src/filter.rs
- crates/engine/src/game/effects/end_phase.rs
- crates/engine/src/game/engine_keyword_action_stack_tests.rs
- crates/engine/src/game/cipher.rs
- crates/engine/src/types/identifiers.rs
- crates/engine/tests/integration/main.rs
- crates/engine/tests/integration/cr733_resolved_trigger_collection.rs
- crates/engine/src/game/effects/life.rs
- crates/engine/tests/integration/cr733_resolved_modifier_install.rs
- crates/engine/src/game/engine_priority.rs
- crates/engine/src/game/engine_phase_trigger_regression_tests.rs
- crates/engine-wasm/src/lib.rs
- crates/phase-ai/src/session.rs
- crates/engine/tests/integration/swarm_combat_witness.rs
- crates/engine/src/game/elimination.rs
- crates/engine/src/game/effects/mod.rs
- crates/phase-ai/src/policies/x_cast_gate.rs
- crates/engine/src/game/engine_modes.rs
- crates/engine/src/game/casting_costs.rs
- crates/engine/tests/integration/pact_of_negation_upkeep_payment.rs
- crates/engine/src/types/resolved_commands.rs
- crates/engine/src/game/effects/draw.rs
- crates/phase-ai/tests/cedh_integration.rs
- crates/engine/src/ai_support/mod.rs
- crates/phase-ai/src/planner/mod.rs
- crates/engine/src/game/derived_views.rs
- crates/engine/src/types/resolution.rs
- crates/phase-ai/src/policies/effect_classify.rs
- crates/phase-ai/src/policies/mulligan/card_floor.rs
- crates/engine/src/game/casting.rs
- crates/engine/src/ai_support/evoke.rs
- crates/phase-ai/src/policies/anti_self_harm.rs
- crates/phase-ai/src/policies/self_cost_value.rs
- crates/engine/src/game/life_safety.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/ai_support/prospective_mana.rs
- crates/engine/src/game/triggers.rs
- crates/engine/src/game/stack.rs
- crates/phase-ai/src/search.rs
matthewevans
force-pushed
the
ship/ai-reliability-fixes
branch
5 times, most recently
from
August 1, 2026 04:03
b7f9c61 to
ac98f31
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/phase-ai/src/policies/self_cost.rs (1)
416-445: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winA chain of drawbacks plus an unmodeled rider reports
Trivial, bypassing the fail-open rule.The
Drawbackarm does not setany_nontrivial. The!any_nontrivialearly return at line 432 runs before theany_unmodeledcheck at line 438. So a chain of oneDrawbackplus oneUnmodeledeffect returnsBenefitAppraisal::Trivial, which the caller reads as "no real payoff". The comment at lines 439-443 states the opposite intent: the unmodeled rider population carries both signs, so no directional conclusion is sound.Order the unmodeled check ahead of the all-trivial return, or make the all-trivial return require that nothing was unmodeled.
🐛 Proposed fix
- if !any_nontrivial { + if any_unmodeled { + // An unmodeled rider can be a benefit or a drawback, so neither the + // all-trivial conclusion nor the summed total is sound. + return BenefitAppraisal::Unpriced; + } + if !any_nontrivial { // All-trivial AND all-unmodeled chains land here — preserving the old // `benefit_is_trivial() == true` behaviour byte for byte (the catch-all // mapped to trivial), so the reject/marginal gate is not weakened. return BenefitAppraisal::Trivial; } - if any_unmodeled { - return BenefitAppraisal::Unpriced; - } BenefitAppraisal::Priced { value: total }Note the deliberate behavior this changes: an all-unmodeled chain currently returns
Trivialto preserve the pre-change gate. If that must be kept, gate the new early return onany_nontrivial || total != 0.0instead, and record the reason in the comment.🤖 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/self_cost.rs` around lines 416 - 445, Ensure unmodeled effects cannot be classified as Trivial when combined with drawbacks: update the appraisal flow around the any_nontrivial and any_unmodeled checks so any_unmodeled is handled first, or require the all-trivial return to confirm no unmodeled effects. Preserve the existing all-unmodeled compatibility behavior only if required, while ensuring a drawback plus an unmodeled rider returns BenefitAppraisal::Unpriced.
♻️ Duplicate comments (3)
crates/phase-ai/src/policies/mulligan/mod.rs (3)
196-199: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick winThe
_ => falsefallback is still present.
GameActionis a known enum. Any cast or hand-activation variant that is not listed reads as "no opening action". The failure is one-directional: a missed variant makesis_certified_dead_landless()returntrue, the card floor abstains, and the AI mulligans a keepable hand. A newCastSpellAs*variant produces that regression with no compile error.This was raised on an earlier commit and marked addressed, but the wildcard remains in the code under review.
As per coding guidelines: "Use exhaustive
matchexpressions without wildcard fallbacks when matching known enums, allowing the compiler to detect missing variants."🤖 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/mulligan/mod.rs` around lines 196 - 199, Replace the wildcard fallback in the GameAction match within is_certified_dead_landless() with an exhaustive match over every GameAction variant. Ensure all cast and hand-activation variants are handled consistently so newly added variants cause a compile-time error until explicitly classified, while preserving the existing behavior for listed variants.Source: Coding guidelines
110-115: 🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
for_handstill derives the player fromcontrollerinstead ofowner.
handis a non-battlefield zone. Objects there must be attributed byobj.owner. A hand object can carry acontrollerthat differs from its owner, and the probe then clones a state whoseactive_playerandpriority_playerare the wrong seat, so the forecast reports another player's opening window. The singlefind_mapalso binds an arbitrary object, so a mixed-ownership slice silently selects one seat.This was raised on an earlier commit and marked addressed, but the code under review still reads
controller. Please confirm which commit is current.🐛 Proposed fix
- let Some(player) = hand - .iter() - .find_map(|object_id| state.objects.get(object_id).map(|object| object.controller)) - else { - return Self::default(); - }; + let mut owners = hand + .iter() + .filter_map(|object_id| state.objects.get(object_id).map(|object| object.owner)); + let Some(player) = owners.next() else { + return Self::default(); + }; + // A mixed-ownership slice is not one player's opening hand. + if owners.any(|owner| owner != player) { + return Self::default(); + }As per path instructions: "Player-scoped queries on NON-battlefield zones (graveyard/library/hand/exile) must filter by
obj.owner, notcontroller(CR 404.2)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/phase-ai/src/policies/mulligan/mod.rs` around lines 110 - 115, Update for_hand’s player selection to use each hand object’s owner rather than controller, and ensure the hand is attributed consistently instead of selecting an arbitrary object from a mixed-ownership slice. Use the owner-derived player when cloning the probe state so active_player and priority_player represent that owner’s opening window.Source: Path instructions
894-919: 📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick winBoth negative assertions still lack a paired positive reach-guard.
Each assertion expects
is_certified_dead_landless() == true. Both pass whenever the fixture object never reacheslegal_actions_fullat all, for example ifactivation_zone = Some(Zone::Hand)is not honored, if the pushed ability is not indexed, or if theSpellability shape is rejected before targeting. The test then proves nothing about cost or target rejection.Add a control per case that flips only the rejected condition:
- Unaffordable activation: add enough mana for the
{2}cost (or setAbilityCost::Tap) and assert the forecast is no longer certified dead.- Targetless spell: put a creature on the battlefield and assert the forecast is no longer certified dead.
This was raised on an earlier commit and marked addressed, but neither control is present in the code under review.
As per path instructions: "For every negative assertion (
!detector(...), "not applied", "does not parse to X"), require a paired positive reach-guard proving the input actually reached the code under test".🤖 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/mulligan/mod.rs` around lines 894 - 919, The negative cases in forecast_rejects_unaffordable_or_targetless_opening_actions lack positive reach-guards. Add a paired control for each fixture: provide enough mana or otherwise satisfy the activation cost and assert the forecast is no longer certified dead, and add a battlefield creature for the targetless spell and assert the forecast is no longer certified dead. Keep each control limited to changing only the rejected cost or targeting condition.Source: Path instructions
🧹 Nitpick comments (8)
crates/engine/tests/integration/draw_delivery_preview.rs (1)
329-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCross-check the empty-library exact zero against the live pipeline.
Every other
Exactrow in this file pairs the preview withcompleted_deliveryon an independent runner. This row does not. The empty-library zero is then asserted only by the code under test, so a preview that wrongly reports zero on an empty library would still pass.♻️ Proposed addition of the live sibling
preview_is_read_only( &runner(0, Vec::new()), 1, DrawDeliveryPreview::Exact { delivered: 0 }, ); + assert_eq!(completed_delivery(runner(0, Vec::new()), 1).0, 0); preview_is_read_only(As per path instructions, a test must exercise the failure path the fix prevents and drive the engine through its production pipeline.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/tests/integration/draw_delivery_preview.rs` around lines 329 - 333, Update the empty-library exact-zero case in preview_is_read_only to also compare against completed_delivery using an independent runner, matching the other Exact cases. Drive both the preview and live production pipeline so the test fails if an empty library incorrectly reports zero delivery.Source: Path instructions
crates/engine/src/game/effects/draw.rs (1)
64-80: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winCorrect the
CR 121.2bcitation.CR 121.2covers sequential individual draws.CR 121.2bcovers per-turn draw restrictions.CR 121.6bcorrectly describes completing a replacement before resuming the remaining draws.🤖 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/draw.rs` around lines 64 - 80, Correct the documentation comment above the draw-count preview function by removing the CR 121.2b citation from the description of multi-card sequential draws. Keep CR 121.2 for sequential individual draws and retain CR 121.6b for replacement completion before resuming the sequence.Source: Path instructions
crates/phase-ai/src/policies/self_cost.rs (1)
397-431: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueMerge the identical
NonTrivialandDrawbackpricing bodies.The two arms differ only in the
any_nontrivial = trueassignment. Theeffect_benefit_valuecall and itsSome/Nonehandling are duplicated verbatim across lines 403-415 and 417-428.♻️ Proposed fix
match triviality { EffectTriviality::Trivial => continue, - EffectTriviality::Unmodeled => any_unmodeled = true, - EffectTriviality::NonTrivial => { - any_nontrivial = true; - match effect_benefit_value(...) { - Some(value) => total += value, - None => return BenefitAppraisal::Unpriced, - } - } - EffectTriviality::Drawback => { - match effect_benefit_value(...) { - Some(value) => total += value, - None => return BenefitAppraisal::Unpriced, - } - } + EffectTriviality::Unmodeled => { + any_unmodeled = true; + continue; + } + EffectTriviality::NonTrivial | EffectTriviality::Drawback => { + any_nontrivial |= triviality == EffectTriviality::NonTrivial; + match effect_benefit_value( + state, + ai_player, + source_id, + effect, + recipient, + &mut pricing, + penalties, + ) { + Some(value) => total += value, + None => return BenefitAppraisal::Unpriced, + } + } }🤖 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/self_cost.rs` around lines 397 - 431, Merge the EffectTriviality::NonTrivial and EffectTriviality::Drawback arms in the classified loop so they share one effect_benefit_value call and identical Some/None handling. Preserve setting any_nontrivial only for NonTrivial before executing the shared pricing logic.crates/phase-ai/src/search.rs (4)
163-170: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReplace the
durable_pact_routesboolean with a typed mode enum.The parameter controls five separate branches (
retain_live_pact_route, draft, arm, filter, fallback filter). At the call sites it appears as a barefalse(line 149) andtrue(line 160), which carries no meaning to a reader. A two-variant enum states the contract directly.♻️ Proposed shape
/// Whether the caller owns an authoritative session that can retain an opaque /// Pact receipt across turns. enum PactRouteMode { /// Stateless entry point: certified Pact roots are excluded. Stateless, /// Caller-owned session: certified Pact roots are drafted and armed. Durable, }Then
choose_actionpassesPactRouteMode::Statelessandchoose_action_with_sessionpassesPactRouteMode::Durable, and each branch readsmatches!(mode, PactRouteMode::Durable).As per coding guidelines: "Prefer typed enums and existing parameterized types such as
ControllerRef,Comparator, andOption<T>over raw booleans or proliferating sibling enum variants".🤖 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/search.rs` around lines 163 - 170, Replace the durable_pact_routes boolean with a two-variant PactRouteMode enum in choose_action_with_session_inner, defining Stateless and Durable semantics. Update choose_action to pass PactRouteMode::Stateless and choose_action_with_session to pass PactRouteMode::Durable, then make all five route branches use matches!(mode, PactRouteMode::Durable) instead of boolean checks.Source: Coding guidelines
287-291: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
remove_certified_pact_rootsfor the stateless retain.Line 290 duplicates the predicate that
remove_certified_pact_roots(line 2463) already owns. Two copies can diverge when the certification rule changes.♻️ Proposed fix
} else { - scored.retain(|(action, _)| !is_certified_pact_root(state, ai_player, action)); + remove_certified_pact_roots(state, ai_player, &mut scored); }🤖 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/search.rs` around lines 287 - 291, Replace the inline retain predicate in the stateless branch of the scored-action flow with the existing remove_certified_pact_roots helper. Reuse that helper’s certification logic while preserving the current durable_pact_routes branch and the resulting filtering behavior.
367-370: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueRemove the unreachable fetch-route guard.
This branch requires an empty hand, so
hand_identity_bindingsis empty andhas_certified_fetch_then_cast_routealways returnsfalse. Remove the guard or document it as protection against a future change to the hand precondition.🤖 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/search.rs` around lines 367 - 370, Remove the unreachable has_certified_fetch_then_cast_route guard from the large_board_main_phase_has_no_development_sources branch and return GameAction::PassPriority directly when the condition holds.
4211-4214: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse one configured card-data path for both tests.
Neither hard-coded path is present or tracked in the repository.
CardDatabase::from_export(...).expect(...)will panic when either test runs without an externally generated export. Use one shared resolver, such as thePHASE_CARDS_PATHconvention, or add an explicit test-data generation step.🤖 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/search.rs` around lines 4211 - 4214, Update the test setup around CardDatabase::from_export to resolve card data through one shared configured path instead of the hard-coded client/public/card-data.json location. Reuse the existing PHASE_CARDS_PATH convention or shared resolver for both tests, while preserving the expect-based failure behavior when the configured export cannot be loaded.crates/phase-ai/src/policies/mulligan/mod.rs (1)
125-167: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDrop the single-element
forloop.
for turn_number in [first_turn]iterates once. The loop form and the|=accumulation at line 151 are residue from the earlier two-window model, and the doc comment at line 96 now describes one window. A straight-line body states the single-probe contract directly and letsusable_nonland_mana_sourcebe a plain assignment.🤖 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/mulligan/mod.rs` around lines 125 - 167, Replace the single-element for loop around the opening probe with straight-line code using first_turn directly. Update the usable_nonland_mana_source accumulation to a direct assignment, while preserving the existing action scanning and single precombat-main probe behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/phase-ai/src/policies/self_cost.rs`:
- Around line 471-486: Update the chained AI-only draw handling in the
surrounding pricing state and Effect::Draw branch to track whether an AI draw
has been previewed independently of pricing.ai_draws_so_far. Set that marker
whenever preview_draw_delivery returns Exact, including delivered == 0, and use
it to return None for subsequent chained draws while preserving ai_draws_so_far
for delivered-card valuation.
- Around line 462-470: Correct the replacement-effect citations in the comments
at crates/phase-ai/src/policies/self_cost.rs lines 462-470 and
crates/phase-ai/src/search.rs lines 2260-2264: use CR 615.1 for prevention
effects and reserve CR 614.11a for replacement effects within a sequence of
draws. Keep all other cited rules unchanged.
In `@crates/phase-ai/src/search.rs`:
- Around line 3780-3788: Update select_safe_action_from_scores to remove Pact
payment casts from scored before calling softmax_select_pairs, so selection only
considers legal candidates and returns None only when all candidates are
filtered out. Preserve the existing state-aware filtering via
is_pact_payment_cast, and add a mixed-candidate test alongside
stateless_pact_apis_never_return_a_certified_pact to verify the non-Pact action
remains selectable.
- Around line 220-229: Move the prospective_fetch_follow_up insertion in the
action_for handling block so it occurs only after exact_contract_action(action)
confirms the action is accepted. Preserve the existing behavior of returning the
contracted action, and avoid storing prompt.follow_up() when the contract check
rejects it.
---
Outside diff comments:
In `@crates/phase-ai/src/policies/self_cost.rs`:
- Around line 416-445: Ensure unmodeled effects cannot be classified as Trivial
when combined with drawbacks: update the appraisal flow around the
any_nontrivial and any_unmodeled checks so any_unmodeled is handled first, or
require the all-trivial return to confirm no unmodeled effects. Preserve the
existing all-unmodeled compatibility behavior only if required, while ensuring a
drawback plus an unmodeled rider returns BenefitAppraisal::Unpriced.
---
Duplicate comments:
In `@crates/phase-ai/src/policies/mulligan/mod.rs`:
- Around line 196-199: Replace the wildcard fallback in the GameAction match
within is_certified_dead_landless() with an exhaustive match over every
GameAction variant. Ensure all cast and hand-activation variants are handled
consistently so newly added variants cause a compile-time error until explicitly
classified, while preserving the existing behavior for listed variants.
- Around line 110-115: Update for_hand’s player selection to use each hand
object’s owner rather than controller, and ensure the hand is attributed
consistently instead of selecting an arbitrary object from a mixed-ownership
slice. Use the owner-derived player when cloning the probe state so
active_player and priority_player represent that owner’s opening window.
- Around line 894-919: The negative cases in
forecast_rejects_unaffordable_or_targetless_opening_actions lack positive
reach-guards. Add a paired control for each fixture: provide enough mana or
otherwise satisfy the activation cost and assert the forecast is no longer
certified dead, and add a battlefield creature for the targetless spell and
assert the forecast is no longer certified dead. Keep each control limited to
changing only the rejected cost or targeting condition.
---
Nitpick comments:
In `@crates/engine/src/game/effects/draw.rs`:
- Around line 64-80: Correct the documentation comment above the draw-count
preview function by removing the CR 121.2b citation from the description of
multi-card sequential draws. Keep CR 121.2 for sequential individual draws and
retain CR 121.6b for replacement completion before resuming the sequence.
In `@crates/engine/tests/integration/draw_delivery_preview.rs`:
- Around line 329-333: Update the empty-library exact-zero case in
preview_is_read_only to also compare against completed_delivery using an
independent runner, matching the other Exact cases. Drive both the preview and
live production pipeline so the test fails if an empty library incorrectly
reports zero delivery.
In `@crates/phase-ai/src/policies/mulligan/mod.rs`:
- Around line 125-167: Replace the single-element for loop around the opening
probe with straight-line code using first_turn directly. Update the
usable_nonland_mana_source accumulation to a direct assignment, while preserving
the existing action scanning and single precombat-main probe behavior.
In `@crates/phase-ai/src/policies/self_cost.rs`:
- Around line 397-431: Merge the EffectTriviality::NonTrivial and
EffectTriviality::Drawback arms in the classified loop so they share one
effect_benefit_value call and identical Some/None handling. Preserve setting
any_nontrivial only for NonTrivial before executing the shared pricing logic.
In `@crates/phase-ai/src/search.rs`:
- Around line 163-170: Replace the durable_pact_routes boolean with a
two-variant PactRouteMode enum in choose_action_with_session_inner, defining
Stateless and Durable semantics. Update choose_action to pass
PactRouteMode::Stateless and choose_action_with_session to pass
PactRouteMode::Durable, then make all five route branches use matches!(mode,
PactRouteMode::Durable) instead of boolean checks.
- Around line 287-291: Replace the inline retain predicate in the stateless
branch of the scored-action flow with the existing remove_certified_pact_roots
helper. Reuse that helper’s certification logic while preserving the current
durable_pact_routes branch and the resulting filtering behavior.
- Around line 367-370: Remove the unreachable
has_certified_fetch_then_cast_route guard from the
large_board_main_phase_has_no_development_sources branch and return
GameAction::PassPriority directly when the condition holds.
- Around line 4211-4214: Update the test setup around CardDatabase::from_export
to resolve card data through one shared configured path instead of the
hard-coded client/public/card-data.json location. Reuse the existing
PHASE_CARDS_PATH convention or shared resolver for both tests, while preserving
the expect-based failure behavior when the configured export cannot be loaded.
🪄 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: d7c8ca9c-38f9-4f51-80b5-66d3f33813c2
📒 Files selected for processing (73)
crates/engine-wasm/src/lib.rscrates/engine/src/ai_support/evoke.rscrates/engine/src/ai_support/mod.rscrates/engine/src/ai_support/prospective_mana.rscrates/engine/src/ai_support/swarm.rscrates/engine/src/game/archenemy_tests.rscrates/engine/src/game/blitz.rscrates/engine/src/game/casting.rscrates/engine/src/game/casting_costs.rscrates/engine/src/game/cipher.rscrates/engine/src/game/dash.rscrates/engine/src/game/derived_views.rscrates/engine/src/game/effects/cast_copy_of_card.rscrates/engine/src/game/effects/copy_spell.rscrates/engine/src/game/effects/counters.rscrates/engine/src/game/effects/delayed_trigger.rscrates/engine/src/game/effects/draw.rscrates/engine/src/game/effects/encore.rscrates/engine/src/game/effects/end_phase.rscrates/engine/src/game/effects/epic.rscrates/engine/src/game/effects/exile_resolving_spell.rscrates/engine/src/game/effects/life.rscrates/engine/src/game/effects/mod.rscrates/engine/src/game/effects/myriad.rscrates/engine/src/game/effects/paradigm.rscrates/engine/src/game/effects/phase_out.rscrates/engine/src/game/effects/rebound.rscrates/engine/src/game/effects/token.rscrates/engine/src/game/elimination.rscrates/engine/src/game/engine.rscrates/engine/src/game/engine_exile_return_tests.rscrates/engine/src/game/engine_keyword_action_stack_tests.rscrates/engine/src/game/engine_modes.rscrates/engine/src/game/engine_phase_trigger_regression_tests.rscrates/engine/src/game/engine_priority.rscrates/engine/src/game/engine_resolution_choices.rscrates/engine/src/game/life_safety.rscrates/engine/src/game/mod.rscrates/engine/src/game/stack.rscrates/engine/src/game/static_abilities.rscrates/engine/src/game/triggers.rscrates/engine/src/game/triggers_pr7_order_template_tests.rscrates/engine/src/game/visibility.rscrates/engine/src/types/game_state.rscrates/engine/src/types/identifiers.rscrates/engine/src/types/resolution.rscrates/engine/src/types/resolved_commands.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rscrates/engine/tests/integration/cr733_resolved_modifier_install.rscrates/engine/tests/integration/cr733_resolved_trigger_collection.rscrates/engine/tests/integration/draw_delivery_preview.rscrates/engine/tests/integration/lose_control_this_turn_delayed_trigger.rscrates/engine/tests/integration/main.rscrates/engine/tests/integration/mechtitan_core_return_exiled.rscrates/engine/tests/integration/pact_of_negation_upkeep_payment.rscrates/engine/tests/integration/prospective_fetchland_mana.rscrates/engine/tests/integration/swarm_combat_witness.rscrates/phase-ai/src/combat_ai.rscrates/phase-ai/src/lib.rscrates/phase-ai/src/planner/mod.rscrates/phase-ai/src/policies/anti_self_harm.rscrates/phase-ai/src/policies/effect_classify.rscrates/phase-ai/src/policies/mulligan/card_floor.rscrates/phase-ai/src/policies/mulligan/keepables_by_land_count.rscrates/phase-ai/src/policies/mulligan/mod.rscrates/phase-ai/src/policies/self_cost.rscrates/phase-ai/src/policies/self_cost_value.rscrates/phase-ai/src/policies/x_cast_gate.rscrates/phase-ai/src/search.rscrates/phase-ai/src/session.rscrates/phase-ai/tests/cedh_integration.rscrates/server-core/src/filter.rs
💤 Files with no reviewable changes (1)
- crates/phase-ai/src/policies/mulligan/keepables_by_land_count.rs
🚧 Files skipped from review as they are similar to previous changes (63)
- crates/engine/src/game/effects/delayed_trigger.rs
- crates/engine/src/game/effects/counters.rs
- crates/engine/src/game/effects/cast_copy_of_card.rs
- crates/engine/src/game/dash.rs
- crates/engine/src/game/casting_costs.rs
- crates/engine/src/game/blitz.rs
- crates/phase-ai/tests/cedh_integration.rs
- crates/engine/tests/integration/mechtitan_core_return_exiled.rs
- crates/engine/src/game/archenemy_tests.rs
- crates/engine/src/game/cipher.rs
- crates/engine/tests/integration/cr733_resolved_trigger_collection.rs
- crates/engine/src/game/effects/copy_spell.rs
- crates/engine/src/game/effects/token.rs
- crates/engine/src/game/effects/end_phase.rs
- crates/engine/src/game/effects/epic.rs
- crates/engine/src/game/derived_views.rs
- crates/engine/src/game/effects/myriad.rs
- crates/engine/src/ai_support/mod.rs
- crates/engine/tests/integration/cr733_resolved_modifier_install.rs
- crates/engine/src/game/static_abilities.rs
- crates/engine/src/game/effects/phase_out.rs
- crates/phase-ai/src/session.rs
- crates/engine/src/game/triggers_pr7_order_template_tests.rs
- crates/engine/src/game/engine_modes.rs
- crates/engine-wasm/src/lib.rs
- crates/engine/src/game/effects/encore.rs
- crates/engine/src/game/engine_priority.rs
- crates/engine/src/game/mod.rs
- crates/engine/src/game/engine_exile_return_tests.rs
- crates/engine/src/game/effects/rebound.rs
- crates/engine/src/game/engine_keyword_action_stack_tests.rs
- crates/engine/src/game/engine_resolution_choices.rs
- crates/engine/tests/integration/breeches_blastmaker_coin_flip_copy.rs
- crates/engine/src/game/effects/exile_resolving_spell.rs
- crates/engine/src/game/effects/paradigm.rs
- crates/engine/src/game/engine_phase_trigger_regression_tests.rs
- crates/engine/src/types/identifiers.rs
- crates/engine/tests/integration/swarm_combat_witness.rs
- crates/engine/src/game/effects/life.rs
- crates/engine/tests/integration/main.rs
- crates/engine/src/game/effects/mod.rs
- crates/phase-ai/src/policies/anti_self_harm.rs
- crates/engine/src/game/visibility.rs
- crates/engine/src/game/casting.rs
- crates/engine/src/types/resolution.rs
- crates/engine/src/ai_support/swarm.rs
- crates/phase-ai/src/lib.rs
- crates/engine/src/game/elimination.rs
- crates/engine/src/game/life_safety.rs
- crates/engine/tests/integration/lose_control_this_turn_delayed_trigger.rs
- crates/engine/src/types/resolved_commands.rs
- crates/phase-ai/src/planner/mod.rs
- crates/engine/src/game/engine.rs
- crates/engine/src/types/game_state.rs
- crates/engine/src/game/stack.rs
- crates/engine/src/game/triggers.rs
- crates/server-core/src/filter.rs
- crates/engine/src/ai_support/prospective_mana.rs
- crates/phase-ai/src/policies/x_cast_gate.rs
- crates/engine/src/ai_support/evoke.rs
- crates/phase-ai/src/policies/effect_classify.rs
- crates/phase-ai/src/policies/self_cost_value.rs
- crates/phase-ai/src/policies/mulligan/card_floor.rs
Comment on lines
+462
to
+470
| // CR 121.1 + CR 121.2: one delivered card is one registry score unit, | ||
| // but a multi-card instruction may deliver only a partial count. The | ||
| // engine-owned preview runs that exact cloned instruction rather than | ||
| // duplicating draw or replacement logic here. | ||
| // | ||
| // CR 614.6 + CR 614.11: a draw the live pipeline REMOVES — mandatorily | ||
| // prevented, substituted away by a non-Draw chain (Notion Thief, | ||
| // Hullbreacher, Chains of Mephistopheles), or count-modified to zero — | ||
| // "never happens"; a `CantDraw` static, an exhausted `PerTurnDrawLimit`, | ||
| // or an empty library (CR 704.5b, where a mere zero is if anything an | ||
| // UNDER-penalty) likewise delivers no card. Such a draw buys exactly | ||
| // nothing, so it is priced 0.0 — priced, NOT `None`: `None` is | ||
| // `BenefitAppraisal::Unpriced`, which stands the whole comparison down | ||
| // and ALLOWS the activation, the opposite of what the engine has just | ||
| // certified. The zero is not a heuristic: `can_draw_at_least_one` | ||
| // delegates every leg to the authority the live pipeline itself uses | ||
| // (`proposed_draw_survives_replacement`), and an OPTIONAL replacement is | ||
| // never assumed to apply, so a deliverable draw is never under-priced. | ||
| // | ||
| // Deliberately binary, not count-aware. `can_draw_at_least_one` asks | ||
| // only whether ONE card arrives, so two legs still price the full | ||
| // nominal count of a draw the engine will only partially deliver: | ||
| // 1. Draw-limit headroom shorter than the count: with a | ||
| // `PerTurnDrawLimit` headroom of 1 a `draw(3)` still prices 3.0 | ||
| // (CR 121.2 performs it as three individual draws, only one of | ||
| // which is permitted). | ||
| // 2. LIBRARY shorter than the count — strictly the worse leg. The | ||
| // preflight probes with count 1, so a 1-card library against a | ||
| // `draw(3)` reports deliverable and prices 3.0; one card arrives | ||
| // and the two failed draw attempts LOSE the game at the next SBA | ||
| // check (CR 704.5b). `select_cards_to_draw` is explicit that a | ||
| // partial draw (`count > library.len()`) yields only what is | ||
| // available, so this is a certified loss the net comparison cannot | ||
| // see — not merely an over-price. | ||
| // Both legs are PRE-EXISTING (this arm priced 3.0 before the | ||
| // deliverability gate existed) and are DISCLOSED, not fixed, here. | ||
| // Over-pricing is the conservative direction for this module's charter | ||
| // — it can let a marginal crack through, never forbid a paying one — | ||
| // but note that over-pricing a benefit is under-vetoing, so neither leg | ||
| // should be read as harmless. Pricing them correctly needs a new `pub` | ||
| // engine authority ("how many of N will actually arrive"), which is | ||
| // tracked as its own unit rather than built for one consumer here. | ||
| Effect::Draw { | ||
| // CR 614.6 + CR 614.11a: prevention and fully settled non-draw | ||
| // substitutions are exact zeroes. A choice required to order or accept | ||
| // a replacement—or raised by its continuation—is not a zero; the policy | ||
| // returns `None` so `BenefitAppraisal::Unpriced` fails open. |
Contributor
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
RULES=$(fd -t f 'MagicCompRules.txt' | head -1)
echo "pinned rules file: $RULES"
for rule in '121\.1' '121\.2' '614\.6' '614\.11a' '701\.9' '609\.3' '702\.74'; do
printf '\n===== CR %s =====\n' "$rule"
rg -n -A4 "^${rule}[.a-z]" "$RULES" || echo "NOT FOUND"
done
printf '\n===== Evoke entry =====\n'
rg -n -B2 -A6 '^702\.[0-9]+\. Evoke' "$RULES" || echo "Evoke heading NOT FOUND"Repository: phase-rs/phase
Length of output: 1039
🏁 Script executed:
set -euo pipefail
printf '%s\n' '== candidate rules files =='
git ls-files | rg -i '(^|/)(MagicCompRules|.*Comprehensive.*Rules|.*rules.*txt)(\.[^/]*)?$' || true
printf '%s\n' '== referenced source files =='
git ls-files crates/phase-ai/src/policies/self_cost.rs crates/phase-ai/src/search.rs
printf '%s\n' '== citation context =='
rg -n -A12 -B4 'CR 121\.1|CR 614\.6|CR 609\.3|CR 702\.74' crates/phase-ai/src/policies/self_cost.rs crates/phase-ai/src/search.rs || trueRepository: phase-rs/phase
Length of output: 5724
🌐 Web query:
Magic Comprehensive Rules 121.1 121.2 614.6 614.11a 609.3 701.9a 702.74a official
💡 Result:
The following Magic: The Gathering Comprehensive Rules sections are defined as follows: 121.1: A player draws a card by putting the top card of their library into their hand [1]. This action occurs as a turn-based action during the draw step, or as part of a spell or ability's cost or effect [1]. 121.2: Cards must be drawn one at a time [1]. If a player is instructed to draw multiple cards, they perform that many individual draws [1]. 609.3: If an effect attempts to do something impossible, it does as much as possible [2]. For example, if an effect instructs a player to discard two cards but they only hold one, they discard only that one [2]. 614.6: If an event is replaced, the original event never occurs [3]. Instead, a modified event takes place, which may trigger abilities [3]. Any instructions in the modified event that cannot be carried out are simply ignored [3]. 614.11a: If an effect replaces a draw within a sequence of card draws, all actions required by the replacement must be completed, if possible, before the sequence of draws resumes [3]. 701.9a: This rule defines the keyword action "Exchange." It states that when a player exchanges two objects, each object currently under the control of a player is moved to the other player's control [4]. (Note: While 701.9a is the standard index for "Exchange," exact text may vary slightly by specific version updates). 702.74a: This rule defines "Evoke" [5][6]. Evoke represents two abilities: a static ability that functions in any zone from which the card can be cast, and a triggered ability that functions on the battlefield [5][6]. "Evoke [cost]" means "You may cast this card by paying [cost] rather than paying its mana cost" and "When this permanent enters, if its evoke cost was paid, its controller sacrifices it" [5][6]. (Note: In some versions of the Comprehensive Rules, 702.74 also refers to "Hideaway," which represents a static ability and a triggered ability [7][8]. Always verify the specific rules version for the intended card mechanic.)
Citations:
- 1: https://mtg-rules.vercel.app/chapters/121
- 2: https://magic-rulebook.vercel.app/rules/609
- 3: https://mtg-rules.vercel.app/chapters/614
- 4: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.pdf
- 5: https://mtg.wiki/page/Evoke
- 6: https://mtga.untapped.gg/codex/mechanics/evoke
- 7: https://apps.magicjudges.org/forum/topic/10372/
- 8: https://mtg-archive.fandom.com/wiki/Hideaway
🌐 Web query:
official Magic Comprehensive Rules CR 614.11a replacement effect sequence of draws CR 614.6 exact text
💡 Result:
According to the official Magic: The Gathering Comprehensive Rules (as of April 17, 2026), rule 614.11a describes the sequence of events when a card draw is replaced. The exact text of rule 614.11a is: "If an effect replaces a draw within a sequence of card draws, all actions required by the replacement are completed, if possible, before resuming the sequence." [1][2][3] Rule 614.6 defines when replacement effects occur: "A replacement effect doesn't invoke itself repeatedly; it gets only one opportunity to affect an event or any modified events that may replace it." [Note: This is the standard definition of CR 614.6 across recent versions.] These rules are part of the broader section 614, which governs Replacement Effects. [4][5]
Citations:
- 1: https://ancestral.vision/spells-abilities-and-effects/replacement-effects.html
- 2: https://mtg.wiki/page/Replacement_effect
- 3: https://mtg-rules.vercel.app/chapters/614
- 4: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.pdf
- 5: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt
🌐 Web query:
official Magic Comprehensive Rules CR 701.9a discard exchange exact text CR 609.3
💡 Result:
The official Magic: The Gathering Comprehensive Rules define the requested sections as follows: 701.9a: To discard a card, move it from its owner’s hand to that player’s graveyard [1]. 609.3: If an effect attempts to do something impossible, it does only as much as possible [2]. These rules are standard in the Comprehensive Rules documentation [3][4]. While rule numbering for specific keyword actions such as "Discard" may occasionally shift in different versions of the rules, the quoted text above represents the authoritative definition for these game concepts [1][5][2].
Citations:
- 1: https://mtg.wiki/page/Discard
- 2: https://ancestral.vision/spells-abilities-and-effects/effects.html
- 3: https://media.wizards.com/2026/downloads/MagicCompRules%2020260227.pdf
- 4: https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.pdf
- 5: https://ancestral.vision/additional-rules/keyword-actions.html
🏁 Script executed:
set -euo pipefail
url='https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt'
tmp=$(mktemp)
curl -fsSL "$url" -o "$tmp"
printf '%s\n' "retrieved: $url"
for rule in '609\.3' '614\.6' '614\.11a' '701\.9a' '702\.74a'; do
printf '\n===== CR %s =====\n' "$rule"
rg -n -A5 "^${rule}[[:space:]]" "$tmp" || echo "NOT FOUND"
done
rm -f "$tmp"Repository: phase-rs/phase
Length of output: 3515
🏁 Script executed:
set -euo pipefail
url='https://media.wizards.com/2026/downloads/MagicCompRules%2020260417.txt'
tmp=$(mktemp)
curl -fsSL "$url" -o "$tmp"
for section in 609 614; do
printf '\n===== section %s headings =====\n' "$section"
rg -n "^${section}\." "$tmp" | head -40
done
printf '\n===== nearby 609/614 text =====\n'
awk 'BEGIN{p=0} /^609\./{p=1} p{print NR ":" $0} /^610\./{exit}' "$tmp"
awk 'BEGIN{p=0} /^614\./{p=1} p{print NR ":" $0} /^615\./{exit}' "$tmp"
rm -f "$tmp"Repository: phase-rs/phase
Length of output: 14946
🏁 Script executed:
set -euo pipefail
printf '%s\n' '===== self_cost.rs 450-525 ====='
sed -n '450,525p' crates/phase-ai/src/policies/self_cost.rs
printf '%s\n' '===== self_cost.rs replacement-related symbols ====='
rg -n -C3 'preview_draw_delivery|replacement|prevention|ai_draws_so_far|BenefitAppraisal' crates/phase-ai/src/policies/self_cost.rsRepository: phase-rs/phase
Length of output: 10988
Correct the replacement-effect citations. CR 614.11a applies only to replacement effects within a sequence of draws. Use CR 615.1 for prevention effects. The other cited rules are applicable.
📍 Affects 2 files
crates/phase-ai/src/policies/self_cost.rs#L462-L470(this comment)crates/phase-ai/src/search.rs#L2260-L2264
🤖 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/self_cost.rs` around lines 462 - 470, Correct
the replacement-effect citations in the comments at
crates/phase-ai/src/policies/self_cost.rs lines 462-470 and
crates/phase-ai/src/search.rs lines 2260-2264: use CR 615.1 for prevention
effects and reserve CR 614.11a for replacement effects within a sequence of
draws. Keep all other cited rules unchanged.
Sources: Path instructions, Learnings
Comment on lines
+220
to
+229
| if let Ok(mut pending) = session.prospective_fetch_prompt.write() { | ||
| if let Some(prompt) = pending.remove(&ai_player) { | ||
| if let Some(action) = prompt.action_for(state, ai_player) { | ||
| if let Ok(mut follow_ups) = session.prospective_fetch_follow_up.write() { | ||
| follow_ups.insert(ai_player, prompt.follow_up()); | ||
| } | ||
| return exact_contract_action(action); | ||
| } | ||
| } | ||
| } |
Contributor
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
Store the fetch follow-up only after the contract check passes.
The code inserts prompt.follow_up() into the session, then returns exact_contract_action(action). If the contract rejects action, the function returns None while the follow-up stays armed for the next decision. Move the insert after the contract check so the follow-up is armed only when the fetch selection is actually returned.
🐛 Proposed fix
if let Some(prompt) = pending.remove(&ai_player) {
if let Some(action) = prompt.action_for(state, ai_player) {
- if let Ok(mut follow_ups) = session.prospective_fetch_follow_up.write() {
- follow_ups.insert(ai_player, prompt.follow_up());
+ let Some(action) = exact_contract_action(action) else {
+ return None;
+ };
+ if let Ok(mut follow_ups) = session.prospective_fetch_follow_up.write() {
+ follow_ups.insert(ai_player, prompt.follow_up());
}
- return exact_contract_action(action);
+ return Some(action);
}
}📝 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.
Suggested change
| if let Ok(mut pending) = session.prospective_fetch_prompt.write() { | |
| if let Some(prompt) = pending.remove(&ai_player) { | |
| if let Some(action) = prompt.action_for(state, ai_player) { | |
| if let Ok(mut follow_ups) = session.prospective_fetch_follow_up.write() { | |
| follow_ups.insert(ai_player, prompt.follow_up()); | |
| } | |
| return exact_contract_action(action); | |
| } | |
| } | |
| } | |
| if let Ok(mut pending) = session.prospective_fetch_prompt.write() { | |
| if let Some(prompt) = pending.remove(&ai_player) { | |
| if let Some(action) = prompt.action_for(state, ai_player) { | |
| let Some(action) = exact_contract_action(action) else { | |
| return None; | |
| }; | |
| if let Ok(mut follow_ups) = session.prospective_fetch_follow_up.write() { | |
| follow_ups.insert(ai_player, prompt.follow_up()); | |
| } | |
| return Some(action); | |
| } | |
| } | |
| } |
🤖 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/search.rs` around lines 220 - 229, Move the
prospective_fetch_follow_up insertion in the action_for handling block so it
occurs only after exact_contract_action(action) confirms the action is accepted.
Preserve the existing behavior of returning the contracted action, and avoid
storing prompt.follow_up() when the contract check rejects it.
Comment on lines
+3780
to
+3788
| pub fn select_safe_action_from_scores( | ||
| state: &GameState, | ||
| scored: &[(GameAction, f64)], | ||
| temperature: f64, | ||
| rng: &mut impl Rng, | ||
| ) -> Option<GameAction> { | ||
| if scored.is_empty() { | ||
| return None; | ||
| } | ||
| softmax_select_pairs(scored, temperature, rng) | ||
| .filter(|action| !is_pact_payment_cast(state, action)) | ||
| } |
Contributor
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Filter Pact casts before the softmax, not after.
The current order runs softmax_select_pairs over the full vector and discards the result when it is a Pact payment cast. Two consequences:
- The function returns
Nonewhile non-Pact candidates remain inscored, so a caller that treatsNoneas "no action available" loses a legal play. - The outcome depends on the RNG draw. The same
scoredslice returns an action on one call andNoneon another, which makes the caller contract untestable.
Filter first. Then None means "every candidate needs a durable Pact route", which is the signal the doc comment describes.
🐛 Proposed fix
pub fn select_safe_action_from_scores(
state: &GameState,
scored: &[(GameAction, f64)],
temperature: f64,
rng: &mut impl Rng,
) -> Option<GameAction> {
- softmax_select_pairs(scored, temperature, rng)
- .filter(|action| !is_pact_payment_cast(state, action))
+ let safe: Vec<(GameAction, f64)> = scored
+ .iter()
+ .filter(|(action, _)| !is_pact_payment_cast(state, action))
+ .cloned()
+ .collect();
+ softmax_select_pairs(&safe, temperature, rng)
}The existing test stateless_pact_apis_never_return_a_certified_pact_root still passes, because its single candidate is a Pact cast. Add a mixed-vector case so the "keeps the non-Pact candidate" behavior is pinned.
📝 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.
Suggested change
| pub fn select_safe_action_from_scores( | |
| state: &GameState, | |
| scored: &[(GameAction, f64)], | |
| temperature: f64, | |
| rng: &mut impl Rng, | |
| ) -> Option<GameAction> { | |
| if scored.is_empty() { | |
| return None; | |
| } | |
| softmax_select_pairs(scored, temperature, rng) | |
| .filter(|action| !is_pact_payment_cast(state, action)) | |
| } | |
| pub fn select_safe_action_from_scores( | |
| state: &GameState, | |
| scored: &[(GameAction, f64)], | |
| temperature: f64, | |
| rng: &mut impl Rng, | |
| ) -> Option<GameAction> { | |
| let safe: Vec<(GameAction, f64)> = scored | |
| .iter() | |
| .filter(|(action, _)| !is_pact_payment_cast(state, action)) | |
| .cloned() | |
| .collect(); | |
| softmax_select_pairs(&safe, temperature, rng) | |
| } |
🤖 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/search.rs` around lines 3780 - 3788, Update
select_safe_action_from_scores to remove Pact payment casts from scored before
calling softmax_select_pairs, so selection only considers legal candidates and
returns None only when all candidates are filtered out. Preserve the existing
state-aware filtering via is_pact_payment_cast, and add a mixed-candidate test
alongside stateless_pact_apis_never_return_a_certified_pact to verify the
non-Pact action remains selectable.
matthewevans
force-pushed
the
ship/ai-reliability-fixes
branch
from
August 1, 2026 15:16
8e8b172 to
e4dec96
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
scripts/gen-test-fixture.py (1)
53-56: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider auto-discovering Phase AI fixture consumers instead of a hand-maintained list.
FIXTURE_CONSUMER_TEST_FILESis a manually maintained list with one entry today.src_fixture_files()already auto-discovers engine-crate consumers by scanning for theshared_card_dbmarker string. Apply the same pattern to Phase AI files (for example, scancrates/phase-aifor files that referenceintegration_cards.json) so a future test that loads the fixture directly is captured automatically instead of requiring a manual addition to this list.♻️ Proposed refactor sketch
-FIXTURE_CONSUMER_TEST_FILES = [REPO_ROOT / "crates/phase-ai/src/search.rs"] +PHASE_AI_DIR = REPO_ROOT / "crates/phase-ai" + + +def fixture_consumer_test_files() -> list[Path]: + """Phase AI files that load the engine fixture directly.""" + files = [] + for rs in PHASE_AI_DIR.rglob("*.rs"): + text = rs.read_text(encoding="utf-8", errors="ignore") + if "integration_cards.json" in text: + files.append(rs) + return filesAlso applies to: 82-86
🤖 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/gen-test-fixture.py` around lines 53 - 56, Replace the manually maintained FIXTURE_CONSUMER_TEST_FILES list with automatic discovery in src_fixture_files, scanning crates/phase-ai for files referencing integration_cards.json and including matching paths alongside the existing engine-crate shared_card_db consumers. Preserve the current fixture path and deduplication behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@scripts/gen-test-fixture.py`:
- Around line 53-56: Replace the manually maintained FIXTURE_CONSUMER_TEST_FILES
list with automatic discovery in src_fixture_files, scanning crates/phase-ai for
files referencing integration_cards.json and including matching paths alongside
the existing engine-crate shared_card_db consumers. Preserve the current fixture
path and deduplication behavior.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 369b2d21-73c1-4a34-8f66-72c6eaf02af6
⛔ Files ignored due to path filters (1)
crates/engine/tests/fixtures/cr733/authority_matrix.json.gzis excluded by!**/*.gz
📒 Files selected for processing (4)
crates/engine/src/analysis/resource.rscrates/engine/tests/fixtures/integration_cards.jsoncrates/phase-ai/src/search.rsscripts/gen-test-fixture.py
matthewevans
force-pushed
the
ship/ai-reliability-fixes
branch
from
August 1, 2026 15:50
e4dec96 to
f3511ef
Compare
4 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 2, 2026
Upstream phase-rs#6842 (8121fd1) made a `TriggerFiring` carrier MANDATORY on every persisted triggered record and fails CLOSED without one. The six 4p dump fixtures this branch drives were captured before that commit, so on the rebased base they no longer decode at all: 41 of the 44 red rows were production-decoder rejections naming the three carriers (`active legacy pending trigger has no firing discriminator`, `triggered stack entry has no firing carrier`, `resolving triggered entry has no firing carrier`). The carrier cannot be RECOVERED, only DERIVED. The read-only pristine root (`combofb-dumps-pristine/`, captured 2026-07-22/25) predates phase-rs#6842 too and records zero firing carriers, so re-deriving from pristine cannot mint a value that was never captured. `TriggerFiring::UnknownLegacy` is not an escape hatch, and this was measured rather than assumed: `validate_firing` (7 call sites, types/game_state.rs :7688/:7691/:7700/:7721/:7732/:7749/:7779) returns `Err("... 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. THE DISCRIMINANT (CR 603.1 ordinary vs CR 603.7a delayed), applied per record: Ordinary <= the fired trigger's definition is present on its SOURCE OBJECT's own `trigger_definitions`/`base_trigger_definitions`, matched by exact `description`. A printed or granted triggered ability of a permanent is an ordinary triggered ability. Delayed <= the trigger has an install receipt in `delayed_triggers`. Anything else ABORTS by name. There is deliberately no fallback stamp: a wrong carrier silently re-classifies a CR 603.7 firing identity, which is precisely the inference upstream refuses to make. Measured, per fixture — carriers stamped, and `delayed_triggers` length: | fixture | carriers | delayed_triggers | sha256 (stamped) | |---|---|---|---| | dellian_emblem_conqueror_4p | 154 | 0 | 3f8b06eb7986de10 | | dina_conqueror_4p | 7 | 0 | 773e902daccf7199 | | fantastic_four_bounded_loop_4p | 3 | 0 | 89ff377b53b7bd55 | | tenacity_exquisite_blood_4p | 6 | 0 | a296ab6e37129ca3 | | witherbloom_sprout_lumaret_4p | 1 | 0 | 71efd26867e9fc57 | | witherbloom_sprout_lumaret_simple_4p | 1 | 0 | 607fbe632e759692 | 172 carriers total, ALL `Ordinary`, ZERO undetermined. Every fixture records `delayed_triggers: []` and no install journal, so `Delayed(Some(prov))` could not have validated regardless — `validate_firing` demands a registered install root. Every one of the 172 was classified from its own record's source object, never defaulted; the per-record witness table (carrier, source id, card name) is in the PR body. Five resolving entries elsewhere in the corpus are NOT migration targets and are deliberately left unstamped: `combo_infinite_pile_4p_untapped_precast`, `kilo_freed_relic_pentad_4p`, `sprout_witherbloom_realistic_lands_4p`, `basalt_power_artifact_infinite_colorless`, `combo_infinite_pile_4p_offer`. Each `kind.type` is `Spell` or `ActivatedAbility`, not `TriggeredAbility`, and the validator's `(Some(_), None) => {}` arm accepts a non-triggered resolving entry with no carrier. An earlier classifier of mine flagged these as UNDETERMINED; reading each entry's `kind.type` showed the flag was wrong. SECOND FIELD CLASS — the CR 603.7 delayed-trigger ALLOCATORS. Restoring the dumps exposed a further phase-rs#6842 change that the decode failures had been masking: `next_delayed_trigger_token` carries `#[serde(default)]`, so a pre-phase-rs#6842 dump that omits it restores as 0 through a bare `GameState` decode, while the production `PersistedGameState` path runs a load-time repair (`next = max(existing // 1, max_used_token + 1)`) and restores 1. The two decoders therefore disagree on a legacy dump, and 0 is invalid on its face: `validate_trigger_firing_coherence` rejects `next_delayed_trigger_token <= max_token`, which is 0 when there are no install roots. This surfaced as the R8 state-neutrality arm of `migrated_dump_decodes_through_both_decoders_and_unmigrated_through_neither` reporting `differing paths: ["state.next_delayed_trigger_token"]`. That arm is NOT relaxed. The fixtures are stamped with the repaired value instead, which makes them look like a modern capture, keeps both decoders in agreement, and survives the eventual deletion of the load-time shim. Measured, all six fixtures: field absent, `delayed_triggers: 0`, and ZERO `DelayedTriggerInstall` commands across 158/30/2/516/50/5094 journal entries. So both used-token sets are empty and the formula collapses to `max(1, 1) = 1`. The scan is not vacuous: injecting one synthetic install command into a fixture makes the same selector report 1 instead of 0. The GENERAL derivation of the used-token set is deliberately NOT reimplemented in jq — it walks `resolved_rules_journal` install commands and `delayed_triggers` provenance with reuse and nonzero checks, and re-deriving it here would repeat exactly the mistake this script's sibling refuses to make for `EffectKind`. Only the collapsed no-install-roots case is stamped; anything else ABORTS BY NAME, verified by probe (one injected install command => "UNDETERMINED delayed-trigger allocators: 1 install command(s) ...", nothing written, exit 1). MECHANISM. `scripts/lib/trigger-firing.jq` holds the SINGLE definition of the derivation; both the pristine-regeneration path (`migrate-dump-fixture.sh`) and the in-place path (`stamp-fixture-firing.sh`) load that one file, so neither can certify its own copy of the recipe. WHY IN PLACE RATHER THAN A PRISTINE REGENERATION for these six. Regeneration is the stronger provenance and is preferred where it applies, but it does not apply here: measured, `dina_conqueror_4p` and `witherbloom_sprout_lumaret_simple_4p` differ from their pristine regeneration in exactly one object each (Priest of Forgotten Gods' `abilities`/`base_abilities` AST), because the committed fixture carries a LATER parser state than the capture. Regenerating them would silently REVERT that. Stamping in place is additive and cannot revert anything. ALL THREE control arms are enforced, and the stamper refuses to write if any fails: arm 1 NO_COLLATERAL the stamped artifact minus the five stamped keys is BYTE-IDENTICAL to what was committed, so nothing but the carriers moved. arm 2 CARRIERS_ADDED got == need && got > 0, keyed on CARRIER COUNT rather than on byte difference. An earlier revision of this arm keyed on bytes and reported a false pass for zero-carrier fixtures, where gzip/jq re-serialization alone changes bytes without stamping anything. Fixtures needing zero carriers are now SKIPped outright. arm 3 ALLOCATORS_CANONICAL both allocators exist and are >= 1 — i.e. above the value the engine's own coherence validator rejects. Also fixes a pre-existing defect in `migrate-dump-fixture.sh` that the corpus sweep exposed: the `target_slots` stage used an unguarded `|=`, which aborts with "Cannot iterate over null" on a dump paused at a beat with no target prompt. The script was therefore only ever usable on 2 of the 6 dumps. The stage is now guarded, so ONE recipe covers the whole corpus. The `gameState`-envelope guard in the jq library is the same class of fix, caught by the control before any write: without it, `.gameState |= ...` would have CREATED a `gameState` key on the four `turn_number`-enveloped dumps, i.e. corrupted them. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 2, 2026
…nto phase-rs#6842 The census tripwire pins the exact source coordinates of every CR 603.5 prompt producer so that a SIXTH producer is a counted event rather than a silent one. Rebasing this branch onto upstream phase-rs#6842 (8121fd1) moved five of those coordinates, and the row fired. ADJUDICATED, NOT RELAXED. The row fired on COORDINATES, not on population. The producer count is still 5, not 6: game/effects/mod.rs :5896 :5973 :8927 => :5918 :5995 :8949 game/engine.rs :10500 => :10589 game/effects/scoped_library_search.rs :452 => :452 (UNMOVED) `mod.rs` is a uniform +22 shift and `engine.rs` a +89 shift — the signature of lines inserted ABOVE each site, not of a new producer. That `scoped_library_search.rs:452` did NOT move is itself evidence the SET did not change: a genuinely new producer would not leave an untouched file's coordinate fixed while shifting the others by a constant. The producer TEXT at all five sites was verified byte-identical between the pre-rebase tree (`chain3-prefold-backup`, dbc81821d) at the old coordinates and this tree at the new ones. So the expectation array is re-anchored to the new coordinates and the assertion keeps its full strength: a sixth producer still fails this row. Correcting my own earlier report for the record: I previously stated that upstream had ADDED a CR 603.5 producer. That was wrong — the count never left 5, and the failure was purely positional. The correction is carried in the tripwire's own doc block so the next reader of this row does not inherit the mistake. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 2, 2026
Upstream phase-rs#6842 (8121fd1) made a `TriggerFiring` carrier MANDATORY on every persisted triggered record and fails CLOSED without one. The six 4p dump fixtures this branch drives were captured before that commit, so on the rebased base they no longer decode at all: 41 of the 44 red rows were production-decoder rejections naming the three carriers (`active legacy pending trigger has no firing discriminator`, `triggered stack entry has no firing carrier`, `resolving triggered entry has no firing carrier`). The carrier cannot be RECOVERED, only DERIVED. The read-only pristine root (`combofb-dumps-pristine/`, captured 2026-07-22/25) predates phase-rs#6842 too and records zero firing carriers, so re-deriving from pristine cannot mint a value that was never captured. `TriggerFiring::UnknownLegacy` is not an escape hatch, and this was measured rather than assumed: `validate_firing` (7 call sites, types/game_state.rs :7688/:7691/:7700/:7721/:7732/:7749/:7779) returns `Err("... 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. THE DISCRIMINANT (CR 603.1 ordinary vs CR 603.7a delayed), applied per record: Ordinary <= the fired trigger's definition is present on its SOURCE OBJECT's own `trigger_definitions`/`base_trigger_definitions`, matched by exact `description`. A printed or granted triggered ability of a permanent is an ordinary triggered ability. Delayed <= the trigger has an install receipt in `delayed_triggers`. Anything else ABORTS by name. There is deliberately no fallback stamp: a wrong carrier silently re-classifies a CR 603.7 firing identity, which is precisely the inference upstream refuses to make. Measured, per fixture — carriers stamped, and `delayed_triggers` length: | fixture | carriers | delayed_triggers | sha256 (stamped) | |---|---|---|---| | dellian_emblem_conqueror_4p | 154 | 0 | 3f8b06eb7986de10 | | dina_conqueror_4p | 7 | 0 | 773e902daccf7199 | | fantastic_four_bounded_loop_4p | 3 | 0 | 89ff377b53b7bd55 | | tenacity_exquisite_blood_4p | 6 | 0 | a296ab6e37129ca3 | | witherbloom_sprout_lumaret_4p | 1 | 0 | 71efd26867e9fc57 | | witherbloom_sprout_lumaret_simple_4p | 1 | 0 | 607fbe632e759692 | 172 carriers total, ALL `Ordinary`, ZERO undetermined. Every fixture records `delayed_triggers: []` and no install journal, so `Delayed(Some(prov))` could not have validated regardless — `validate_firing` demands a registered install root. Every one of the 172 was classified from its own record's source object, never defaulted; the per-record witness table (carrier, source id, card name) is in the PR body. Five resolving entries elsewhere in the corpus are NOT migration targets and are deliberately left unstamped: `combo_infinite_pile_4p_untapped_precast`, `kilo_freed_relic_pentad_4p`, `sprout_witherbloom_realistic_lands_4p`, `basalt_power_artifact_infinite_colorless`, `combo_infinite_pile_4p_offer`. Each `kind.type` is `Spell` or `ActivatedAbility`, not `TriggeredAbility`, and the validator's `(Some(_), None) => {}` arm accepts a non-triggered resolving entry with no carrier. An earlier classifier of mine flagged these as UNDETERMINED; reading each entry's `kind.type` showed the flag was wrong. SECOND FIELD CLASS — the CR 603.7 delayed-trigger ALLOCATORS. Restoring the dumps exposed a further phase-rs#6842 change that the decode failures had been masking: `next_delayed_trigger_token` carries `#[serde(default)]`, so a pre-phase-rs#6842 dump that omits it restores as 0 through a bare `GameState` decode, while the production `PersistedGameState` path runs a load-time repair (`next = max(existing // 1, max_used_token + 1)`) and restores 1. The two decoders therefore disagree on a legacy dump, and 0 is invalid on its face: `validate_trigger_firing_coherence` rejects `next_delayed_trigger_token <= max_token`, which is 0 when there are no install roots. This surfaced as the R8 state-neutrality arm of `migrated_dump_decodes_through_both_decoders_and_unmigrated_through_neither` reporting `differing paths: ["state.next_delayed_trigger_token"]`. That arm is NOT relaxed. The fixtures are stamped with the repaired value instead, which makes them look like a modern capture, keeps both decoders in agreement, and survives the eventual deletion of the load-time shim. Measured, all six fixtures: field absent, `delayed_triggers: 0`, and ZERO `DelayedTriggerInstall` commands across 158/30/2/516/50/5094 journal entries. So both used-token sets are empty and the formula collapses to `max(1, 1) = 1`. The scan is not vacuous: injecting one synthetic install command into a fixture makes the same selector report 1 instead of 0. The GENERAL derivation of the used-token set is deliberately NOT reimplemented in jq — it walks `resolved_rules_journal` install commands and `delayed_triggers` provenance with reuse and nonzero checks, and re-deriving it here would repeat exactly the mistake this script's sibling refuses to make for `EffectKind`. Only the collapsed no-install-roots case is stamped; anything else ABORTS BY NAME, verified by probe (one injected install command => "UNDETERMINED delayed-trigger allocators: 1 install command(s) ...", nothing written, exit 1). MECHANISM. `scripts/lib/trigger-firing.jq` holds the SINGLE definition of the derivation; both the pristine-regeneration path (`migrate-dump-fixture.sh`) and the in-place path (`stamp-fixture-firing.sh`) load that one file, so neither can certify its own copy of the recipe. WHY IN PLACE RATHER THAN A PRISTINE REGENERATION for these six. Regeneration is the stronger provenance and is preferred where it applies, but it does not apply here: measured, `dina_conqueror_4p` and `witherbloom_sprout_lumaret_simple_4p` differ from their pristine regeneration in exactly one object each (Priest of Forgotten Gods' `abilities`/`base_abilities` AST), because the committed fixture carries a LATER parser state than the capture. Regenerating them would silently REVERT that. Stamping in place is additive and cannot revert anything. ALL THREE control arms are enforced, and the stamper refuses to write if any fails: arm 1 NO_COLLATERAL the stamped artifact minus the five stamped keys is BYTE-IDENTICAL to what was committed, so nothing but the carriers moved. arm 2 CARRIERS_ADDED got == need && got > 0, keyed on CARRIER COUNT rather than on byte difference. An earlier revision of this arm keyed on bytes and reported a false pass for zero-carrier fixtures, where gzip/jq re-serialization alone changes bytes without stamping anything. Fixtures needing zero carriers are now SKIPped outright. arm 3 ALLOCATORS_CANONICAL both allocators exist and are >= 1 — i.e. above the value the engine's own coherence validator rejects. Also fixes a pre-existing defect in `migrate-dump-fixture.sh` that the corpus sweep exposed: the `target_slots` stage used an unguarded `|=`, which aborts with "Cannot iterate over null" on a dump paused at a beat with no target prompt. The script was therefore only ever usable on 2 of the 6 dumps. The stage is now guarded, so ONE recipe covers the whole corpus. The `gameState`-envelope guard in the jq library is the same class of fix, caught by the control before any write: without it, `.gameState |= ...` would have CREATED a `gameState` key on the four `turn_number`-enveloped dumps, i.e. corrupted them. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 2, 2026
…nto phase-rs#6842 The census tripwire pins the exact source coordinates of every CR 603.5 prompt producer so that a SIXTH producer is a counted event rather than a silent one. Rebasing this branch onto upstream phase-rs#6842 (8121fd1) moved five of those coordinates, and the row fired. ADJUDICATED, NOT RELAXED. The row fired on COORDINATES, not on population. The producer count is still 5, not 6: game/effects/mod.rs :5896 :5973 :8927 => :5918 :5995 :8949 game/engine.rs :10500 => :10589 game/effects/scoped_library_search.rs :452 => :452 (UNMOVED) `mod.rs` is a uniform +22 shift and `engine.rs` a +89 shift — the signature of lines inserted ABOVE each site, not of a new producer. That `scoped_library_search.rs:452` did NOT move is itself evidence the SET did not change: a genuinely new producer would not leave an untouched file's coordinate fixed while shifting the others by a constant. The producer TEXT at all five sites was verified byte-identical between the pre-rebase tree (`chain3-prefold-backup`, dbc81821d) at the old coordinates and this tree at the new ones. So the expectation array is re-anchored to the new coordinates and the assertion keeps its full strength: a sixth producer still fails this row. Correcting my own earlier report for the record: I previously stated that upstream had ADDED a CR 603.5 producer. That was wrong — the count never left 5, and the failure was purely positional. The correction is carried in the tripwire's own doc block so the next reader of this row does not inherit the mistake. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
Upstream phase-rs#6842 (8121fd1) made a `TriggerFiring` carrier MANDATORY on every persisted triggered record and fails CLOSED without one. The six 4p dump fixtures this branch drives were captured before that commit, so on the rebased base they no longer decode at all: 41 of the 44 red rows were production-decoder rejections naming the three carriers (`active legacy pending trigger has no firing discriminator`, `triggered stack entry has no firing carrier`, `resolving triggered entry has no firing carrier`). The carrier cannot be RECOVERED, only DERIVED. The read-only pristine root (`combofb-dumps-pristine/`, captured 2026-07-22/25) predates phase-rs#6842 too and records zero firing carriers, so re-deriving from pristine cannot mint a value that was never captured. `TriggerFiring::UnknownLegacy` is not an escape hatch, and this was measured rather than assumed: `validate_firing` (7 call sites, types/game_state.rs :7688/:7691/:7700/:7721/:7732/:7749/:7779) returns `Err("... 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. THE DISCRIMINANT (CR 603.1 ordinary vs CR 603.7a delayed), applied per record: Ordinary <= the fired trigger's definition is present on its SOURCE OBJECT's own `trigger_definitions`/`base_trigger_definitions`, matched by exact `description`. A printed or granted triggered ability of a permanent is an ordinary triggered ability. Delayed <= the trigger has an install receipt in `delayed_triggers`. Anything else ABORTS by name. There is deliberately no fallback stamp: a wrong carrier silently re-classifies a CR 603.7 firing identity, which is precisely the inference upstream refuses to make. Measured, per fixture — carriers stamped, and `delayed_triggers` length: | fixture | carriers | delayed_triggers | sha256 (stamped) | |---|---|---|---| | dellian_emblem_conqueror_4p | 154 | 0 | 3f8b06eb7986de10 | | dina_conqueror_4p | 7 | 0 | 773e902daccf7199 | | fantastic_four_bounded_loop_4p | 3 | 0 | 89ff377b53b7bd55 | | tenacity_exquisite_blood_4p | 6 | 0 | a296ab6e37129ca3 | | witherbloom_sprout_lumaret_4p | 1 | 0 | 71efd26867e9fc57 | | witherbloom_sprout_lumaret_simple_4p | 1 | 0 | 607fbe632e759692 | 172 carriers total, ALL `Ordinary`, ZERO undetermined. Every fixture records `delayed_triggers: []` and no install journal, so `Delayed(Some(prov))` could not have validated regardless — `validate_firing` demands a registered install root. Every one of the 172 was classified from its own record's source object, never defaulted; the per-record witness table (carrier, source id, card name) is in the PR body. Five resolving entries elsewhere in the corpus are NOT migration targets and are deliberately left unstamped: `combo_infinite_pile_4p_untapped_precast`, `kilo_freed_relic_pentad_4p`, `sprout_witherbloom_realistic_lands_4p`, `basalt_power_artifact_infinite_colorless`, `combo_infinite_pile_4p_offer`. Each `kind.type` is `Spell` or `ActivatedAbility`, not `TriggeredAbility`, and the validator's `(Some(_), None) => {}` arm accepts a non-triggered resolving entry with no carrier. An earlier classifier of mine flagged these as UNDETERMINED; reading each entry's `kind.type` showed the flag was wrong. SECOND FIELD CLASS — the CR 603.7 delayed-trigger ALLOCATORS. Restoring the dumps exposed a further phase-rs#6842 change that the decode failures had been masking: `next_delayed_trigger_token` carries `#[serde(default)]`, so a pre-phase-rs#6842 dump that omits it restores as 0 through a bare `GameState` decode, while the production `PersistedGameState` path runs a load-time repair (`next = max(existing // 1, max_used_token + 1)`) and restores 1. The two decoders therefore disagree on a legacy dump, and 0 is invalid on its face: `validate_trigger_firing_coherence` rejects `next_delayed_trigger_token <= max_token`, which is 0 when there are no install roots. This surfaced as the R8 state-neutrality arm of `migrated_dump_decodes_through_both_decoders_and_unmigrated_through_neither` reporting `differing paths: ["state.next_delayed_trigger_token"]`. That arm is NOT relaxed. The fixtures are stamped with the repaired value instead, which makes them look like a modern capture, keeps both decoders in agreement, and survives the eventual deletion of the load-time shim. Measured, all six fixtures: field absent, `delayed_triggers: 0`, and ZERO `DelayedTriggerInstall` commands across 158/30/2/516/50/5094 journal entries. So both used-token sets are empty and the formula collapses to `max(1, 1) = 1`. The scan is not vacuous: injecting one synthetic install command into a fixture makes the same selector report 1 instead of 0. The GENERAL derivation of the used-token set is deliberately NOT reimplemented in jq — it walks `resolved_rules_journal` install commands and `delayed_triggers` provenance with reuse and nonzero checks, and re-deriving it here would repeat exactly the mistake this script's sibling refuses to make for `EffectKind`. Only the collapsed no-install-roots case is stamped; anything else ABORTS BY NAME, verified by probe (one injected install command => "UNDETERMINED delayed-trigger allocators: 1 install command(s) ...", nothing written, exit 1). MECHANISM. `scripts/lib/trigger-firing.jq` holds the SINGLE definition of the derivation; both the pristine-regeneration path (`migrate-dump-fixture.sh`) and the in-place path (`stamp-fixture-firing.sh`) load that one file, so neither can certify its own copy of the recipe. WHY IN PLACE RATHER THAN A PRISTINE REGENERATION for these six. Regeneration is the stronger provenance and is preferred where it applies, but it does not apply here: measured, `dina_conqueror_4p` and `witherbloom_sprout_lumaret_simple_4p` differ from their pristine regeneration in exactly one object each (Priest of Forgotten Gods' `abilities`/`base_abilities` AST), because the committed fixture carries a LATER parser state than the capture. Regenerating them would silently REVERT that. Stamping in place is additive and cannot revert anything. ALL THREE control arms are enforced, and the stamper refuses to write if any fails: arm 1 NO_COLLATERAL the stamped artifact minus the five stamped keys is BYTE-IDENTICAL to what was committed, so nothing but the carriers moved. arm 2 CARRIERS_ADDED got == need && got > 0, keyed on CARRIER COUNT rather than on byte difference. An earlier revision of this arm keyed on bytes and reported a false pass for zero-carrier fixtures, where gzip/jq re-serialization alone changes bytes without stamping anything. Fixtures needing zero carriers are now SKIPped outright. arm 3 ALLOCATORS_CANONICAL both allocators exist and are >= 1 — i.e. above the value the engine's own coherence validator rejects. Also fixes a pre-existing defect in `migrate-dump-fixture.sh` that the corpus sweep exposed: the `target_slots` stage used an unguarded `|=`, which aborts with "Cannot iterate over null" on a dump paused at a beat with no target prompt. The script was therefore only ever usable on 2 of the 6 dumps. The stage is now guarded, so ONE recipe covers the whole corpus. The `gameState`-envelope guard in the jq library is the same class of fix, caught by the control before any write: without it, `.gameState |= ...` would have CREATED a `gameState` key on the four `turn_number`-enveloped dumps, i.e. corrupted them. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
…nto phase-rs#6842 The census tripwire pins the exact source coordinates of every CR 603.5 prompt producer so that a SIXTH producer is a counted event rather than a silent one. Rebasing this branch onto upstream phase-rs#6842 (8121fd1) moved five of those coordinates, and the row fired. ADJUDICATED, NOT RELAXED. The row fired on COORDINATES, not on population. The producer count is still 5, not 6: game/effects/mod.rs :5896 :5973 :8927 => :5918 :5995 :8949 game/engine.rs :10500 => :10589 game/effects/scoped_library_search.rs :452 => :452 (UNMOVED) `mod.rs` is a uniform +22 shift and `engine.rs` a +89 shift — the signature of lines inserted ABOVE each site, not of a new producer. That `scoped_library_search.rs:452` did NOT move is itself evidence the SET did not change: a genuinely new producer would not leave an untouched file's coordinate fixed while shifting the others by a constant. The producer TEXT at all five sites was verified byte-identical between the pre-rebase tree (`chain3-prefold-backup`, dbc81821d) at the old coordinates and this tree at the new ones. So the expectation array is re-anchored to the new coordinates and the assertion keeps its full strength: a sixth producer still fails this row. Correcting my own earlier report for the record: I previously stated that upstream had ADDED a CR 603.5 producer. That was wrong — the count never left 5, and the failure was purely positional. The correction is carried in the tripwire's own doc block so the next reader of this row does not inherit the mistake. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
…firewall, and measured 4p rows (combo-fb phases 5a-5d, chain 3) (phase-rs#6886) * feat(engine): pin per-iteration decision slots for bounded-loop shortcut A bounded cycle can only be fast-forwarded when every player choice it repeats is forced. Publish the pinned slots for the one class where that provably holds — a "target opponent" triggered ability — and let the loop-window scope discharge exactly those, and only those. The published legal set is MOVED from the announcement authority (`build_target_slots`, CR 601.2c: the ability's controller announces), never re-derived from the head effect's target filter. Re-deriving it published [Player(1), Player(2)] where the builder said [Player(0), Player(1), Player(2)] on a chained "target player" sub-ability — measured, not hypothetical. The head filter is now a shape gate only, enforced by a `bool` return rather than by comment, so re-deriving it is unrepresentable. The relief returns a residual verdict instead of a blanket `continue`, so it discharges only what the pin specifies and re-arms the life guard; and the re-check delegates to the same authority that minted the value, so the two cannot drift. CR 601.2c / CR 603.3d / CR 115.2 (each grepped in docs/MagicCompRules.txt) Tests: 22036 passed / 0 failed / 15 ignored (`cargo test -p engine`; the 15 is 8 #[ignore] + 7 doc-test fences. `cargo nextest run -p engine` reports 22036 passed / 8 skipped — it does not run doc-tests, so cite the runner). clippy --workspace --all-targets -D warnings: clean. Assisted-by: ClaudeCode:claude-opus-4.8 * feat(engine): bounded CR 732.2a cycle fast-forward for a multiplayer drain A draining loop never returns the board to a prior state, so board-recurrence alone cannot certify it. Add a second, board-blind basis: a periodic resource signature derived from the retained ring, which certifies a period only after seeing it twice, and publish a bounded shortcut whose iteration cap is the CR 704.5a elimination minimum computed from the live board. The board-blind basis needs a discriminant the board-based one gets for free. Without one it accepts the game's own turn structure — two players each drawing once per turn is a genuine repeating resource signature, and it minted a shortcut offer on a board with no loop at all. CR 732.2a permits a shortcut to cross turns, so the fix is scoped to the board-blind basis only: its certifying window must be turn-position invariant, expressed through the same window_scope_from_cover_frames authority the growing-class firewall already uses. Zero new predicates, types, or fields. Basis attribution is by discriminating probe, never by frames_per_period: basis A publishes 1 unconditionally and the board-blind basis at k==1 publishes the identical value, so equality discriminates nothing. That inference had already mislabeled a real four-player drain dump as board-recurrent; it is in fact the board-blind basis, and it is this feature's real-dump control. On a growing cascade the two bases are separated by whether the board carries a fire-time condition reading a projected resource axis -- not by which resources move. Recorded at the dispatch site, since both shipped row docs had asserted a resource-purity rule that measurement refuted on their own fixtures. CR 732.2a / CR 704.5a / CR 703.1a / CR 601.2c (each grepped in docs/MagicCompRules.txt) Tests: 22046 passed / 0 failed / 15 ignored (cargo test -p engine); cargo nextest run -p engine reports 22046 passed / 8 skipped -- it does not run doc-tests, so cite the runner. clippy --workspace --all-targets -D warnings clean. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): a mandatory draw is not a resolution-time choice The bounded-shortcut offer requires every stack entry's resolution-time choices to be specified. That verdict came from an allow-list of exactly two Effect variants -- GainLife and LoseLife -- out of roughly two hundred, with everything else falling to a fail-closed MayPrompt. The exclusions were never assessed and found choice-bearing; they were simply never enumerated, so a draining loop whose stack held a draw trigger could never be fast-forwarded. Keyed on the MECHANISM, never the variant name: a mandatory draw whose only prompt route is its replacement environment. The same Effect::Draw is genuinely choice-bearing when its ability is optional (CR 603.5), and stays refused; a count of "up to" is a CR 608.2d resolution-time choice and is excluded fail-closed, recursively, so a future QuantityExpr variant cannot smuggle one past a wildcard. The verdict is parameterized rather than given a sibling: FreeUnlessReplacements carries the set of replacement classes its obligation covers, and chained abilities union them. A FreeUnlessDrawReplacements sibling would have forced all three consumers to re-derive that union. The parameterization is grounded, not assumed: all three prompt seams live in the event-agnostic pipeline_loop and none branches on event class. Also corrects a premise this code documented and a real fixture falsifies. The every-entry scan was justified by "in an exact-recurrence window every entry re-announces each cycle"; the dellian dump is a growing-cascade window over a frozen bottom prefix -- 107 entries with identical ids and indices surviving 220 beats while the stack grew above them. dellian is named there as the refutation only. No allow-list widening of any size unblocks it, and nothing here claims otherwise. CR 121.1 / CR 616.1 / CR 608.2d / CR 603.5 / CR 702.52a / CR 704.5b (each grepped in docs/MagicCompRules.txt; 702.52a is dredge -- 702.51a, written from memory first, is convoke) Acceptance: bloodloop at 3 and 4 players goes 0 offers -> 1, with UnspecifiedChoiceWindow 34 -> 0 attributing the flip to that gate; the 2p offer moves beat 31 -> 29. All three beats pinned. Five revert probes, each flipping its own assertion. Tests: 22048 passed / 0 failed / 15 ignored (cargo test -p engine); cargo nextest run -p engine reports 22048 passed / 8 skipped -- it does not run doc-tests, so cite the runner. clippy --workspace --all-targets -D warnings clean. Assisted-by: ClaudeCode:claude-opus-4.8 * fix(engine): delimit a bounded CR 732.2a cycle by its published per-period frame span A basis-B certificate — what `ring_delta_signature` mints, and the class the bounded offer widened to — certifies a periodic DELTA, not a recurring board. `materialize_fixed_shortcut`'s 'cycles loop only advanced on board recurrence, so neither recurrence predicate could ever fire and the declared `n` was structurally inert: the drive could only end at the beat cap (committing zero) or by crossing lethal. Measured on the production accept path at c6d834040, `Fixed(1)` and `Fixed(3)` were byte-identical — bloodloop 3p/4p wiped the whole table at either count, dina 4p committed nothing at either. `PeriodicDelta::frames_per_period` was written, cloned onto the proposal, and read by nothing. It is the missing delimiter, and its own doc already specified the check nobody implemented ('so a bounded drive can check that each committed cycle actually conformed'). Both now exist: * `drive_one_shortcut_cycle` takes the published span and completes a cycle once that many retained ring frames have been recorded. Frames are counted the same way they are minted — the engine's single `record_loop_detect_sample` call site is inside `pass_priority_once_with_pipeline`, the function the drive steps — and detected by Arc identity of the ring's back, since the ring evicts at its cap. * `materialize_fixed_shortcut` drops any committed cycle whose measured resource delta differs from the published one. CR 704.5a: `elimination_bounds` divided the headroom by that delta, so a divergent cycle invalidates the agreed bound. `per_cycle: None` (every pre-bounded offer) keeps board recurrence as the sole delimiter and skips the conformance check, so those drives are unchanged. Basis A's `frames_per_period` was a hardcoded `1` and is measured WRONG: the subset-lethal DRAIN_CLERIC/BLOOD_SIPPER fixture's repetition spans TWO frames. It is now derived from the certifying prior's ring index. Under the hardcode that fixture's accepted drive committed nothing at all (the new conformance check catching the half-period). The shipped `frames_per_period == 1` assertion was a self-ratifying oracle — it compared a literal against a constant no game state could influence — and is corrected to the measured 2 with that mechanism recorded. AFTER, same production path: bloodloop3 n=1 [20,17,17]->[20,16,16], n=3 ->[20,14,14], n=16 (the bound) ->[20,1,1]; dina 4p n=1 ->[50,34,30,35], n=30 (the bound) ->[79,5,1,6]. Zero eliminations at every n within bound. New rows: the n-scaling acceptance property on all three fixtures; the matched stop-short pair at the CR 704.5a boundary; the conformance drop; a named negative row for each of the three bounded-offer conjuncts that refused zero times; and the AI's bounded-declare candidate generated, applied and driven. Doc corrections, each re-measured rather than reasoned: the dina row's named step-(7) revert-probe genuinely does not flip (106/0 green under `1..=MAX`); the dellian frozen bottom prefix is 151 entries over 220 beats, not 140 or 107; the AI policy's 'every n within max_iterations eliminates nobody' premise was false when it shipped and is now true and re-measured. Assisted-by: ClaudeCode:claude-opus-4.8 * test(engine): cover the partial-crossing arm and the basis-A span at drive level Fix round 2 on the `review-impl` findings. No production behaviour changes: every `engine.rs` edit is a comment or lives inside `#[cfg(test)] mod bounded_offer_conjunct_tests`. MED-1. The required cross-lethal stop-short row was demonstrated only on bloodloop3, whose two opponents sit at equal life (17/17) and therefore cross 0 on the SAME cycle — a symmetric fixture cannot witness an asymmetric outcome. The arm real multiplayer boards take had no fixture, and the mirror's doc claim ("the eliminated set is exactly the seats the published period drains") is false on it. The two arms are asymmetric: total wipe every remaining opponent crosses together => WaitingFor::GameOver => CycleOutcome::CrossLethal: the crossing cycle COMMITS, game ends, eliminated == victims partial one seat crosses while >= 2 survive => no GameOver => CycleOutcome::Abort: the crossing cycle is ROLLED BACK WHOLE, eliminated == [] (by rollback, not because nobody crossed) Both are out of contract for a legitimately-derived bound — `elimination_bounds` reserves `life - 1` of CR 704.5a headroom — so each is reachable only under a doctored one. `bounded_fixed_drive_rolls_back_a_partial_crossing_cycle` asserts the measured partial behaviour on the dina 4p dump: honest bound 30, first crossings 31/35/36, so doctoring the bound to 31/34/40 commits exactly 30 periods, eliminates nobody and hands back `Priority{P0}`. The Abort is not "fixed": rolling the out-of-contract cycle back is conservative and correct, since the remaining repetitions were bounded by a delta the board stops moving once a drain target leaves. The mirror's claim is now scoped per arm and its arm-a doctoring is recorded — and asserted — as the no-op it is on that fixture. MED-2. Reverting `frames_per_period: span as u32` to the old hardcoded `1` flipped exactly one row, and that row asserts the PUBLISHED number, never accepting a `Fixed(n)` — so the basis-A half of the delimiter shipped on a single published-value assertion. `basis_a_bounded_fixed_count_commits_exactly_n_periods` declares and drives `Fixed(1)`, `Fixed(2)` and `Fixed(3)` through `apply()` on the subset-lethal (basis-A, k == 2) fixture and measures `n * delta` committed. Under the hardcode it commits `{0, 0, 0}` at every n — the conformance check dropping the half-periods a too-small k produces. The width tripwire is placed LAST on purpose: with it first, the hardcode probe failed there and the drive assertions were never reached, which would have made the row a second copy of the one it exists to back. Measured: the hardcode revert now flips 2 rows of 85, and the flip it reports is "the drive committed nothing". The `span >= 1` fail-closed guard gains `a_zero_span_certifying_pair_never_publishes_a_zero_width_period`, on a new `drain_ring` fixture. `mill_ring` could not serve: library size is BOARD, so its frames are board-unequal, basis A refuses them outright, and every mill-ring row in that module is really exercising basis B. Life is projected, so a drain ring's frames are board-equal and reach the basis-A walk. Reach-guards assert the span-0 pair satisfies both halves of the disjunct, so the guard is provably the refuser; deleting it publishes 0. Doc corrections, each re-measured. Six sites claimed "basis A publishes 1 unconditionally" — false since the previous commit made it a measured span, and one of them was carrying a basis attribution that inference no longer supports. Counts now name the runner AND the filter (`cargo test -p engine --test integration -- loop_shortcut::`, module filter, 4090 filtered out); the one bare count with neither recorded is deleted rather than re-dressed. Revert-probes run: delete the frame delimiter => 5 of 85 fail (the basis-A row does NOT, because its board recurs and delimits itself); delete the conformance check => 1 of 85 fails and the partial-crossing row stays green, which is what establishes its stop is the Abort rather than a conformance drop. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): the aborted cycle rolls back alone, not the whole drive Doc-only. Corrects the arm-asymmetry wording introduced in the previous commit, which read as if a partial crossing discarded the entire drive. It does not: `materialize_fixed_shortcut`'s `break 'cycles` falls through to `*state = committed`, the last WHOLE cycle — so the out-of-contract cycle is refused atomically while every prior conforming cycle stays committed. The measured shape already said so (dina 4p, honest bound 30: a doctored `n` at or past the first crossing commits 30 periods and refuses the 31st), and the row's assertions were already bound to `first_crossing - 1` rather than to zero. total wipe every remaining opponent crosses 0 on the same cycle => WaitingFor::GameOver => CycleOutcome::CrossLethal: the crossing cycle COMMITS, the game ends partial crossing one seat crosses 0 while >= 2 players survive => no GameOver => CycleOutcome::Abort: the crossing cycle rolls back WHOLE; prior conforming cycles STAY COMMITTED; priority handback The property this buys is named where it is implemented: no half-applied period, ever — refuse the out-of-contract cycle atomically, keep the conforming prefix. The per-arm eliminated-set split is unchanged and stays two distinct facts: on the GameOver arm the eliminated set is exactly the victims; on the Abort arm it is empty because the crossing cycle was refused, not because nobody crossed. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): correct four doc-accuracy claims and re-cite CR 101.2 in the bounded-offer rows Fix round 3 of the combo-detector player-feedback review loop. Doc-only: zero production or test-behaviour change; all five findings were LOW. - LOW-1: the flip-count sentence mixed epochs (numerator pre-commit, denominator post-commit). Now states 1-of-83 as measured on the pre-row tree and 2-of-85 on this one, with the runner and filter named. - LOW-2: CR 119.8 governs life exchanges, redistribution, and pay-life costs -- not a "Your life total can't change." override. Re-cited to CR 101.2, which is the engine's own convention and what the sibling fixture doc already names. - LOW-3: a bare "(4167)" suite count with no runner and no filter is deleted rather than re-dressed, mirroring the same removal in the unit module's doc. Swept both files; it was the last one. - LOW-4: the basis-A drive row now scopes itself as synthetic-fixture-only -- every real 4p dump in the file certifies on basis B. - LOW-5: "basis A refuses a mill ring outright" rested on evidence measuring only the equal disjunct. Measured the cover disjunct too: cover == false at all three ring indices, so the claim holds -- and the probe corrects the reason at the oldest frame, which is board-EQUAL and refused by net_progress_for instead. Verified: cargo fmt --all; cargo clippy --all-targets -- -D warnings (exit 0); cargo test -p engine --lib -- game::engine::bounded_offer_conjunct_tests:: (4 passed / 0 failed, 17870 filtered out); cargo test -p engine --test integration -- loop_shortcut:: (85 passed / 0 failed, 4090 filtered out). Assisted-by: ClaudeCode:claude-opus-4.8 * docs(engine): fix the CLAIM, not the site -- four sibling doc corrections Fix round 4 of the combo-detector player-feedback review loop. Doc-only: zero non-comment lines on either side of the diff; all four findings were LOW. Every one was a SIBLING SITE that round 3's fix did not reach, or self-contamination that round 3's fix introduced. - LOW-1: drain_ring's doc -- the DEFINITIONAL site a reader reaches first -- still carried the blanket "a mill ring's frames are board-UNEQUAL" that round 3 corrected 160 lines later. Now states the per-index truth: board-unequal at every index EXCEPT the oldest, which pops zero cards, is board-EQUAL, and is refused by net_progress_for on its zero delta. The conclusion (basis A certifies nothing on a mill ring) is unchanged and still measured. - LOW-2: "this is the only row that detects a back-door deletion of basis B" was measurably false and licensed exactly the overread the round-3 SCOPE note exists to prevent, from 870 lines earlier. Re-measured with the file's own prescribed attribution probe (ring_delta_signature -> None unconditionally), runner cargo test -p engine --test integration -- loop_shortcut:: : 74 passed / 11 failed / 4090 filtered, against a clean 85 passed / 0 failed. ELEVEN rows flip; all ten others are now named so the claim is checkable. - LOW-3: the file named three rules for one const -- CR 101.2 twice and a governing CR 119.8 once. The outlier survived the round that fixed its twin. Both life-loss-immune fixture docs now read "CR 101.2 ... cf. CR 119.8, which governs only life EXCHANGES, REDISTRIBUTION and pay-life COSTS". Swept both files: no governing 119.8 remains. - LOW-4: round 3's own correction became the file's first in-comment test attribute, so its stated counting recipe (grep the attribute over this file) returned 86 while the runner reported 85 -- the fix for a counting error broke the count by containing the thing being counted. The denominator is now anchored to the runner, which is where the number came from, and the quoted literal is gone (grep and runner agree at 85 again). Verified on the frozen final tree: cargo fmt --all; cargo clippy --all-targets -- -D warnings (exit 0, zero warnings) -- the gate the round-4 reviewer honestly did not run; cargo test -p engine --test integration -- loop_shortcut:: (85 passed / 0 failed, 4090 filtered out). Attribution probe reverted byte-identically (md5 match) before any edit landed. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): scope two universals in the basis-B control's doc to their measured sets Fix round 5 of the combo-detector player-feedback review loop. Doc-only: 26 added and 4 removed lines, all `///`; zero non-comment lines on either side. Both LOW findings were universal quantifiers that had never been checked against the set they quantify over -- one of them introduced by round 4's own replacement sentence. - LOW-1: "each of those ten fails for a reason its own doc does not name" is false for the first row it names. Now NINE of ten, with dina called out as the exception: its CERTIFICATION BASIS note already documents this very probe as measurement (ii), down to the 400-beat cap and the `expect` firing. Measured by scanning all ten doc blocks for `ring_delta_signature`/`basis B` (dina 6 hits, the other nine 0), plus a second pass for probe-adjacent wording. - LOW-2: annotating dina alone as "(the real 4p dump)" implied an exclusivity it does not have, and contradicted the plural "the file's real 4p dumps" 900 lines later. Dropped, and replaced by the measured split over the whole set: SIX of the eleven load the real dina_conqueror_4p capture, five are GameScenario builds, and bounded_fixed_count_commits_exactly_n_periods is the one MIXED row. Classified by resolving every fixture-loading call site to its enclosing test fn. The count basis is stated in the doc as call-site resolution, NOT grep hit count, because these sentences themselves add doc-comment hits for the names they list -- the self-contamination class this lane earned in round 4. Both measurement methods were re-run after the edit and returned the same numbers. Verification: cargo clippy --all-targets -- -D warnings, exit 0, zero warning or error lines. cargo fmt --all clean. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): point our repro commands at the renamed phase-engine package Upstream 2b204dff5 (#6739) renamed the engine package to phase-engine, so the six `cargo test -p engine` repro commands in these two files' doc comments no longer resolve. Comment-only; no non-doc lines changed. Upstream's own stale `-p engine` doc comments in eight other files are left alone -- provenance-checked against upstream/main, they are not ours. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): route player choice through one legality authority (5c) Every site that materializes a player choice or validates a player target now goes through a single authority instead of ad hoc per-site filters, so a seat that has left the game or phased out can no longer be offered or pinned. - CR 102.1: eliminated players are not choosable or targetable. - CR 702.26b: a phased-out permanent's controller is treated as absent for choice and target legality. - CR 608.2b: pinned player targets are re-validated mid-drive; an unresolvable pin withdraws the offer rather than publishing an undeclarable point. - CR 732.2a: a wire-encoded zero-repetition shortcut bound is rejected at the load seam, closing a window that let a zero-cycle proposal deserialize. Also repairs the save-compat path: dump loaders now decode through the production PersistedGameState decoder rather than bypassing it, and fixture migration is re-runnable from the read-only pristine root via scripts/migrate-dump-fixture.sh. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): split LoopDetectSample into normalized and live ring halves (5d U0) CR 104.4b: the normalized half stays the draw-detection comparand; the live half preserves what normalization erases so later 5d steps can evaluate period touches against real board state. Ring re-typed VecDeque<Arc<LoopDetectSample>>, serde(skip) unchanged (zero persistence surface), normalize_for_loop body untouched. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): event-derived resolution obligation and prompt-cause partition (5d U1) CR 616.1/614.1a: replacement-prompt causes derived from the proposed-event stream (ReplacementPromptCauses bit-set over find_applicable_replacements); 29-variant wildcard-free event_is_accounted partition; probe_resolution with the four Prompted arms; bind_resolution_scope extracted from resolve_top (CR 608.2k); UpTo guards on all six promoted quantity arms; forward-split verdict_memo::ProbeBudget (narrowed at U3). Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): shape-B mint conjuncts and declare-time owner firewall (5d U2) CR 603.5/732.2a: entry_publishes_pin_slots gains the recipient, stored-auto-choice, and mint-level cardinality conjuncts for both mint shapes; declare-time owner firewall validates against LoopShortcutOffer.proposer with apply_confirmed_shortcut re-validation; firewall placement covers the empty-schema route (R28 a-double-prime pins it). Known pre-existing defect disclosed, not fixed here: ShortcutProposal.per_cycle PlayerId-keyed map fails JSON key deserialization, making persisted bounded-shortcut saves unloadable. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): re-derive the loop-shortcut probe budget from the offering beat Measured through `try_offer_bounded_cycle_shortcut_metered`, three real 4p dumps driven through `apply()` at an unbounded cap: dina beat 19 spent=13 asks=13 skips=0 ring=3 stack=10 OFFER dina beat 45 spent=73 OFFER (post-fast-forward) dellian max spent=107 asks=5 ring=16 stack=177 no offer in 200 beats f4 max spent=0 no offer in 200 beats R16(ii-a): demand at the corpus's acceptance beat is 13, so the shipped cap of 12 starved it by exactly one charge. PROBE_BUDGET 12 -> 26 (2x the measured demand; the unexempted sweep measures 96-107, so an unexempted classification still exhausts and still refuses fail-closed). R16(ii-b), recorded: the max spend across ALL mintable beats is 107, at non-offering dellian beats where refusal is correct. BOTH AXES at the offering beat, per the R33 escalation: basis = B (`ResourceSignatureOnly`); the within-basis-A disjunct has NO VALUE — basis A certified 0 times across all three dumps (129 basis-B certifications). Named-dump caveat: dellian and F4 reached no offering beat in 200 beats under `dump_drive_one_beat`, so the witness is dina. Consequence for the cost table (§3 D4.3): the frozen exemption's speed-up applies at dellian-shaped beats, which exist in-corpus but never offer under the shipped driver. THE OFFERING BEAT PAYS THE FULL UNEXEMPTED SWEEP — `skips=0` there — so the 854x headline no longer describes the beat this class actually offers on. `MintMeter` gains `certification`, the only surface on which the certifying disjunct is observable: both bases publish `frames_per_period`, so the published `LoopCertificate` discriminates in neither direction. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): pin the frozen exemption to its certifying disjunct (5d U3, R33) Six arms on the CR 732.2a bounded-offer frozen exemption, all on real-dump or constructed-equality windows driven through `apply()`: - (a)/(b)/(a'1)/(c1)/(c2) on the real dellian window — beat SEARCHED by construction requirements, landing on beat 14 / 152 frozen, independently agreeing with the plan's RF13-BEAT row; (a) bounded by an independent longest-common-prefix scan, never f(x)==f(x). - (a'2) at the SELECTION site on a constructed equality-certified ring, with both reach-guards firing (non-empty BoardCovered frozen set; certification == Some(BoardEqualOnly)). - (d) landed previously (060d0cf64). Revert-probes, all run and restored byte-identical: 1. delete the pre-walk early return => (b) flips, 152 frozen leak back 2. basis-B call site -> BoardCovered => (d) flips 3. delete step 4b (round-39 shape) => (a'2) ALONE flips while a/b/a'1/c/(d) all still pass — the round-40-vs-round-39 discriminator this arm exists for. R33(c2) keys to the PROBE_BUDGET constant (12->26 needed no edit); the 96-107 unexempted band is recorded, not asserted. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): budget-refusal and warmup-meter rows for the bounded offer (5d U3, R15/R16(v)/R20) - R15: the real dina offer beat replayed at ProbeCap::Lowered(0) refuses with UnspecifiedChoiceWindow while Shipped offers; probe (delete probe_resolution's try_charge_one arm) FLIPPED — the zero-cap board offers at spent:0 asks:13, D=13 confirmed by a third independent instrument. Basis B certifies for free at any cap (no board predicate); the denial is conjunct (6)'s. - R16(v): a ring-starved dellian beat at priority (stack > 2) yields NoCertification with (spent, denied, asks, scans) == (0,false,0,0). DISCLOSED LIMITATION in the test doc: MintMeter is snapshotted below the ring gate's early return, so a hoisted eager pass is invisible to this instrument — the observable half is pinned; the structural half rests on construction order. - R20: 27 entries (PROBE_BUDGET + 1) with the last CHAINED so the bound is per-link; the same board offers at RaisedTwiceLinks, refuses at Shipped with denied at spent == PROBE_BUDGET, and a pre-charge NotAtPriority control leaves a clean meter. Probe run twice: run 1 masked by (i)'s spent >= entries clause (re-keyed to conjunct6_asks — what the gate examined, not what it charged); run 2 flips in the plan's direction (over-budget board OFFERS, asks:27 spent:0). - Shared fixtures extracted (dina_driven_to_bounded_offer, replay_at_priority, equality_ring_with_stack); R33 arm (a'2) rewired to the helper and re-run green. All probes restored byte-identical; resolution_prompt.rs carries zero diff. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): offer-writer census and carrying-frame rows (5d U3, R8/R14/R19/R29/R31/R32) Purely additive test rows on top of 93dc52c02; six revert-probes run, flipped, and restored byte-identical. Suite arithmetic pins the delta: lib 18144 -> 18149, integration 4311 -> 4313. Three plan contradictions disclosed, none silently absorbed: - R8's ROUND-7 pre-change check is comment-blind: U2's doc comment at engine.rs:4168 makes a bare anchor measure 23/12. The census excludes comment lines (a comment writes no offer and consumes none), restoring the plan's 22 production / 12 test and its per-file production multiset exactly. - R29's stated (c) probe had no flipping site as written ((c1)/(c2) sit on the predicate, the probe edits an argument they never traverse); ADDED arm (c3) driving the same crossing through stack_choices_are_all_specified — the probe flips (c3) alone. - R21 arm (a3) is FALSIFIED and not written: it demands frozen_skips > 0 at the offering beat, but the corpus's offering beat certifies through basis B with skips == 0 — the exact fact R33 arm (d) pins. A row contradicting a shipped green row does not ship. CR anchors: 115.2, 601.2c, 603.3c, 603.5, 608.1, 732.2a — all re-read against the code they describe. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): verdict-door totality, keying and proposer rows (5d U3, R22) All five R22 conjuncts across four tests so each conjunct's revert-probe lands on its own assertion: totality/frame/proposer (1)+(2)+(2'), the window-offset- zero vacuity boundary (green under all four probes, never counted as coverage), conjunct (3) foreign-frame refusal, conjunct (4) effective-key proposer. Four probes run, flipped, restored byte-identical (0 deleted lines in the diff): id-only memo key; unchecked frame_ix; hard-coded proposer; deleted relief agreement guard. The fourth flipped only on run 2 — run 1 exposed a VACUOUS relief negative: entry_publishes_pin_slots early-returns on entry.controller != proposer (engine.rs:1670), so the B-bound container answered None upstream of the guard under test. Fixed with a B-controlled entry plus a positive reach-guard (under B's own pins the entry IS relieved); the probe then flipped. CR 603.5 anchor re-verified. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): close the U3 remainder — R16, R17, R21(b)-family, R27(a2), Arc::as_ptr Nine test-only rows, +919/-0, no production line touched. MEASURED, through `try_offer_bounded_cycle_shortcut_metered` and through the predicates the mint calls, on both TRACKED 4p dumps driven via `apply()`: dina beat 19 demand=13 offer fires, cap does not bind (D searched, not copied) dina beat 10 stack=8 announced=3 frozen=5 asks=6 skips=5 sum=11 dellian beat 14 stack=154 announced=2 frozen=152 asks=4 skips=152 sum=156 dellian beat 14 mint: NoCertification, spent=26=cap, denied, scans=36, 1.79 s (debug) id freshness: dellian 227 announcements / 50 resolutions / 0 revivals; dina 88/17/0 R21(b)'s SUM IDENTITY is CORRECTED, not copied: the plan's "sum = current.stack.len() = 154-156" is HEAD-era. Post-U3 the domain is `announced ∪ (stack \ frozen)`, so the measured sum is `announced + stack` (156 and 11). The plan's asks/skips figures themselves reproduce. R21(b-placement-B)'s stated matched pair is FALSIFIED and re-keyed: the unmutated dellian beat-14 board does NOT offer (item (4) already trips at stack index 35, and that entry is ITSELF frozen), so no mutation is needed — item (4) scans 36 entries where only 2 are non-exempt, while conjunct (6) on the same touch skips all 152. R27(d) is NOT WRITTEN, measured vacuous four ways, including running the plan's own stated revert (comparand ring <- .live): 4/4 green, nothing flips. Basis A certifies 0 times corpus-wide; `frames_per_period`/`delta` come from `ring_delta_signature`, which reads `f.normalized` directly from the sample and never consults the mint's `ring` vec; `residual_board_delta` is byte-identical across the halves. R16(ii-a)'s `spent <= PROBE_BUDGET` is VACUOUS by construction (a budget cannot overspend), so the row ships the denial flag plus an exact-demand sweep across the seam's closed cap domain: `Lowered(13)` offers, every `Lowered(n < 13)` refuses. R16(iv)'s ceiling is DEBUG-scaled and says so. Eight revert-probes applied, seven flipped, all restored byte-identically. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): retained-sample .live-reader rows close U3 (5d, R27 a3/b/c/e) Four arms on a CONSTRUCTED board driven through the real mint, where ring frame 2 carries an entry neither frame 1 nor `current` holds — so the announced pair genuinely arrives from a retained sample and the shared carrier revert (ring_live <= .normalized) has a BEHAVIOURAL flipping site, defeating the round-7 blind-spot tripwire for (b)/(c): - (a3) retained sample's Effect::Token derivation equals the live board's; control shows the .normalized half derives a different set. Flips on P-A (clear_trigger_identity_recursive source_id scrub deleted): (940,940) vs (940,0). No mint-level flip — measured and disclosed in-code (both derivations are event_is_accounted). - (b) stored MayChoice auto-record survives the ring: no record => OFFER publishing exactly 1 MayChoice point; record seeded => UnspecifiedChoiceWindow. Flips on P-B (carrier revert): the negative OFFERS. - (c) intervening-if binds with the retained sample's trigger source; the "classifies identically to current" conjunct and a MayPrompt control on the normalized half ride the same row. Flips on P-B: the positive REFUSES. - (e) discharge reads the pair's carrying frame, not the live board — four boards (frame-only / both / neither / causeless) plus the frame_ix pointer-identity structural conjunct. Flips on P-C (resolution_events_are_discharged frame => state), both-boards arm green in the same run. R8 census tripwire fired benignly: (22,12) => (22,13) — production half unchanged, per-file multiset unchanged; the +1 is (b)'s cfg(test) read of the offer schema, adjudicated per the row's protocol in its doc and failure message. All probes restored byte-identically (sha-verified). CR gate: 12 numbers, 0 unverified. Assisted-by: ClaudeCode:claude-opus-5 * feat(engine): OptionalEffectChoice pin-injection arm with seat and beat guards (5d U4) inject_pinned_answer gains the CR 603.5 MayChoice consumption arm (plan section 3 D5 verbatim) with both total head guards in source order — seat (*player != template.owner => RecastAbort, CR 603.5) and beat (work.pending_trigger.is_some() => RecastAbort, CR 603.3c announcement-time question is never answerable by a resolution-time pin) — plus the two injector doc corrections the arm falsifies (Targets-pin/MayChoice-pin split; the _ arm's remainder is "(mode / unless / X)"). Rows: R23(4) seat matched pair; R23(5) beat matched pair on the SAME source_id (differing-source would be refused by the slot lookup and report the wrong guard); R28(b) re-keyed on measurement — the plan's "drive seam must still RecastAbort for a matching hostile owner" is FALSE and its own cell says why (the comparand is client input): (b1) asserts the breach loudly with the re-key instruction in-code, (b2) asserts the declare-seam refusal that makes the ingress unreachable, matched positive included. Fourth row added for the accept mapping (both MayChoiceOption directions — live inverted-mapping failure mode with no plan row). R23's (5-reach) ships with R2 in U5. Four revert-probes run, flipped, restored sha-identical. P3 (declare firewall deleted) flips (b2) printing the tampered proposal reaching APNAP; (b1) is a measured must-not-flip control; U2's r28_a/r28_a'' flip on it too, r28_c stays green. Duplicate-needle tripwire fired once on P4's first application (doc quoted the production expression verbatim); doc rewritten, needle re-measured 1, probe re-run and flipped. Census adjudications, never relaxed: CR 603.5 prompt census 34=>37 (producer half unchanged at 5); R8 offer-writer test half 13=>14 (production unchanged at 22, multiset byte-identical) — new sites named in doc + failure message. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): track the F4 4p dump and pin the bounded-offer behaviour it actually has Stages `crates/engine/tests/fixtures/fantastic_four_bounded_loop_4p.json.gz` for the first time, together with the integration module that loads it through the production chokepoint (`PersistedGameState::into_game_state`) and drives it beat-by-beat through the public `apply()` — no synthetic `GameScenario` stands in for the real board. Rows: R18 (name-resolver fails loud both ways), R1 + R1b (offer fires, bound re-derived independently of `elimination_bounds`, published point set), R2 (what an accepted declaration commits), R23 5-reach (no answered may-beat carries a construction cursor), R9 (refusal keyed to a derived replacement obligation, not a definition name), R16 (exact probe demand at the offering beat), R27 a1 (the recorded sample keeps a live half normalisation would erase), and R7 in `analysis/resource.rs` (the frozen prefix is an index-id identity, never a presence count). MEASURED CORRECTIONS, pinned rather than asserted-as-planned: * The bounded offer FIRES on the real dump (beat 43, P0, max_iterations = 35, basis B) but publishes ONE decision point, not the three §6 R1 predicted, and an accepted declaration therefore commits ZERO cycles at both n = 1 and n = 3 — it answers Sue's `may` from the pin and then aborts on Reed's unpinned `may`. Fail-CLOSED (rollback, CR 800.4a handback): nothing rules-wrong ships, but it is not a grant. R1/R1b are labelled PARTIAL and §6 R2a/R2b/R3/R4/R5 plus the interruptibility matched pair are deliberately NOT written — no row asserts a falsified prediction. * CONSEQUENCE-DISCLOSURE of the ratified `PROBE_BUDGET = 26`: §6 D1/D2 (dellian) are unreachable and are NOT written. dellian drives 309 beats through `apply()` to `GameOver` with no offer ever raised; every mintable beat refuses `UnspecifiedChoiceWindow` with `spent = 26 = PROBE_BUDGET, denied = true`. dellian's measured demand at that seam was 96–107, so this is budget exhaustion by design of the cap, not a detector fault. Remedy sizing for the follow-up is journaled with measurements (widening `announced` is insufficient alone; the sampler gate is not the seam — two relaxations left the frame census unchanged; the resolution-order sequence is empty at the offering beat). Assisted-by: ClaudeCode:claude-opus-5 * test(engine): pin what the AI can actually do at the real F4 bounded offer §5 U6's AI-verification step, keyed to the measured reality rather than to the plan's three-slot prediction. Drives the tracked 4p F4 dump to its real CR 732.2a offer through `apply()` and measures `engine::ai_support::legal_actions` there — the seam `phase-ai/src/search.rs` reaches via `WaitingFor::LoopShortcut { .. } => engine::ai_support::legal_actions(state)`. MEASURED: the generator emits exactly two candidates, `DeclareShortcut { count: UntilLethal, template: None }` and `DeclineShortcut`. Its `Fixed(max_iterations)` candidate — which exists precisely for bounded offers — is gated on `schema.points.is_empty()`, and this offer publishes one point, so it is excluded; the surviving declare candidate is refused outright by `handle_declare_shortcut` (`UntilLethal` against a narrowed bound). The AI's only effective action is therefore DECLINE, which the phase-ai policy independently reaches: the offer latches `predicted_winner: None`, routing `LoopShortcutPolicy` to its `(None, UntilLethal) => reject` arm. These rows supply the reachability that policy's own row cannot — a real captured board carrying exactly that pair. The whole declare option space is measured on one board: `UntilLethal` + None, `UntilLethal` + a conformant template, and `Fixed(max)` + None are all refused into the CR 800.4a handback; `Fixed(n)` + a template pinning every published point with `owner == proposer` is ACCEPTED and opens the CR 732.2b window — the anti-vacuity control, and the shape the generator never emits. A third row exercises the declare-time `template.owner` firewall on the real dump (the staged-offer arm lives in `loop_shortcut.rs`'s `r28_a`). THE GAP IS REPORTED, NOT CLOSED, for two measured reasons: an accepted declaration on this board commits ZERO cycles (the unannounced-`may` defect pinned by `r2`), so a candidate-generator fix rides the grant mechanism and inherits its hold; and building the template needs a new engine authority for the pin CONTENT, which is a design decision rather than executor-local plumbing. §6 R8's U6 invariance arm — the one arm §5 leaves open — PASSES: U6 touches no tree R8 walks (`git diff --stat HEAD -- crates/engine/src crates/phase-ai/src` is empty), the raw `WaitingFor::LoopShortcut {` census is unchanged at 37, and R8's own tests stay green at `(production, test) == (22, 14)` with the `ai_support/candidates.rs 1` multiset entry intact. No census update is owed. Assisted-by: ClaudeCode:claude-opus-5 * docs(engine): review-impl LOW fixes — board-not-prompt contract, real Braids text, census pin Three LOW findings from the independent full-diff review, applied without any logic change: - The three-field "pure over (stack, objects, proposer)" wording at the mint's three sites is replaced with the board-not-prompt contract the U2 doc already states (the mint reaches eleven GameState fields through optional_prompt_player; what callers rely on is PROMPT-independence). The CR 603.3c relief justification is restated on the stronger premise: the pending_trigger_entry cursor is prompt-coupled by every production writer. - The Braids, Conjurer Adept quotes at three sites now carry the card's real Oracle text (verified against Scryfall twice, executor and reviewer independently) with the fixture's Effect::PutCounter named as a synthetic stand-in for the class, sound because the branch is effect-agnostic. - .combofb-5d-cr-gate.md gains a committed-range reconciliation: 49 distinct CR numbers on added lines, 0 unresolved, +7 previously untabled, 2 stale rows superseded; two independent instruments agree and a fake-number positive control proves discrimination. One knock-on adjudicated per the census test's own protocol: the +7 net doc lines above engine.rs:10493 moved the CR 603.5 producer to :10500; the pinned string is re-adjudicated with the cause named (producer byte-identical; the total-37 and 5/7/25 partition asserts fired green in the same red run, proving the set never moved). Near-miss recorded in the module doc: an adjudication doc must never quote the census needle verbatim — it would self-count. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): derive CR 603.7 firing carriers for the 4p dump corpus Upstream #6842 (8121fd1c6) made a `TriggerFiring` carrier MANDATORY on every persisted triggered record and fails CLOSED without one. The six 4p dump fixtures this branch drives were captured before that commit, so on the rebased base they no longer decode at all: 41 of the 44 red rows were production-decoder rejections naming the three carriers (`active legacy pending trigger has no firing discriminator`, `triggered stack entry has no firing carrier`, `resolving triggered entry has no firing carrier`). The carrier cannot be RECOVERED, only DERIVED. The read-only pristine root (`combofb-dumps-pristine/`, captured 2026-07-22/25) predates #6842 too and records zero firing carriers, so re-deriving from pristine cannot mint a value that was never captured. `TriggerFiring::UnknownLegacy` is not an escape hatch, and this was measured rather than assumed: `validate_firing` (7 call sites, types/game_state.rs :7688/:7691/:7700/:7721/:7732/:7749/:7779) returns `Err("... 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. THE DISCRIMINANT (CR 603.1 ordinary vs CR 603.7a delayed), applied per record: Ordinary <= the fired trigger's definition is present on its SOURCE OBJECT's own `trigger_definitions`/`base_trigger_definitions`, matched by exact `description`. A printed or granted triggered ability of a permanent is an ordinary triggered ability. Delayed <= the trigger has an install receipt in `delayed_triggers`. Anything else ABORTS by name. There is deliberately no fallback stamp: a wrong carrier silently re-classifies a CR 603.7 firing identity, which is precisely the inference upstream refuses to make. Measured, per fixture — carriers stamped, and `delayed_triggers` length: | fixture | carriers | delayed_triggers | sha256 (stamped) | |---|---|---|---| | dellian_emblem_conqueror_4p | 154 | 0 | 3f8b06eb7986de10 | | dina_conqueror_4p | 7 | 0 | 773e902daccf7199 | | fantastic_four_bounded_loop_4p | 3 | 0 | 89ff377b53b7bd55 | | tenacity_exquisite_blood_4p | 6 | 0 | a296ab6e37129ca3 | | witherbloom_sprout_lumaret_4p | 1 | 0 | 71efd26867e9fc57 | | witherbloom_sprout_lumaret_simple_4p | 1 | 0 | 607fbe632e759692 | 172 carriers total, ALL `Ordinary`, ZERO undetermined. Every fixture records `delayed_triggers: []` and no install journal, so `Delayed(Some(prov))` could not have validated regardless — `validate_firing` demands a registered install root. Every one of the 172 was classified from its own record's source object, never defaulted; the per-record witness table (carrier, source id, card name) is in the PR body. Five resolving entries elsewhere in the corpus are NOT migration targets and are deliberately left unstamped: `combo_infinite_pile_4p_untapped_precast`, `kilo_freed_relic_pentad_4p`, `sprout_witherbloom_realistic_lands_4p`, `basalt_power_artifact_infinite_colorless`, `combo_infinite_pile_4p_offer`. Each `kind.type` is `Spell` or `ActivatedAbility`, not `TriggeredAbility`, and the validator's `(Some(_), None) => {}` arm accepts a non-triggered resolving entry with no carrier. An earlier classifier of mine flagged these as UNDETERMINED; reading each entry's `kind.type` showed the flag was wrong. SECOND FIELD CLASS — the CR 603.7 delayed-trigger ALLOCATORS. Restoring the dumps exposed a further #6842 change that the decode failures had been masking: `next_delayed_trigger_token` carries `#[serde(default)]`, so a pre-#6842 dump that omits it restores as 0 through a bare `GameState` decode, while the production `PersistedGameState` path runs a load-time repair (`next = max(existing // 1, max_used_token + 1)`) and restores 1. The two decoders therefore disagree on a legacy dump, and 0 is invalid on its face: `validate_trigger_firing_coherence` rejects `next_delayed_trigger_token <= max_token`, which is 0 when there are no install roots. This surfaced as the R8 state-neutrality arm of `migrated_dump_decodes_through_both_decoders_and_unmigrated_through_neither` reporting `differing paths: ["state.next_delayed_trigger_token"]`. That arm is NOT relaxed. The fixtures are stamped with the repaired value instead, which makes them look like a modern capture, keeps both decoders in agreement, and survives the eventual deletion of the load-time shim. Measured, all six fixtures: field absent, `delayed_triggers: 0`, and ZERO `DelayedTriggerInstall` commands across 158/30/2/516/50/5094 journal entries. So both used-token sets are empty and the formula collapses to `max(1, 1) = 1`. The scan is not vacuous: injecting one synthetic install command into a fixture makes the same selector report 1 instead of 0. The GENERAL derivation of the used-token set is deliberately NOT reimplemented in jq — it walks `resolved_rules_journal` install commands and `delayed_triggers` provenance with reuse and nonzero checks, and re-deriving it here would repeat exactly the mistake this script's sibling refuses to make for `EffectKind`. Only the collapsed no-install-roots case is stamped; anything else ABORTS BY NAME, verified by probe (one injected install command => "UNDETERMINED delayed-trigger allocators: 1 install command(s) ...", nothing written, exit 1). MECHANISM. `scripts/lib/trigger-firing.jq` holds the SINGLE definition of the derivation; both the pristine-regeneration path (`migrate-dump-fixture.sh`) and the in-place path (`stamp-fixture-firing.sh`) load that one file, so neither can certify its own copy of the recipe. WHY IN PLACE RATHER THAN A PRISTINE REGENERATION for these six. Regeneration is the stronger provenance and is preferred where it applies, but it does not apply here: measured, `dina_conqueror_4p` and `witherbloom_sprout_lumaret_simple_4p` differ from their pristine regeneration in exactly one object each (Priest of Forgotten Gods' `abilities`/`base_abilities` AST), because the committed fixture carries a LATER parser state than the capture. Regenerating them would silently REVERT that. Stamping in place is additive and cannot revert anything. ALL THREE control arms are enforced, and the stamper refuses to write if any fails: arm 1 NO_COLLATERAL the stamped artifact minus the five stamped keys is BYTE-IDENTICAL to what was committed, so nothing but the carriers moved. arm 2 CARRIERS_ADDED got == need && got > 0, keyed on CARRIER COUNT rather than on byte difference. An earlier revision of this arm keyed on bytes and reported a false pass for zero-carrier fixtures, where gzip/jq re-serialization alone changes bytes without stamping anything. Fixtures needing zero carriers are now SKIPped outright. arm 3 ALLOCATORS_CANONICAL both allocators exist and are >= 1 — i.e. above the value the engine's own coherence validator rejects. Also fixes a pre-existing defect in `migrate-dump-fixture.sh` that the corpus sweep exposed: the `target_slots` stage used an unguarded `|=`, which aborts with "Cannot iterate over null" on a dump paused at a beat with no target prompt. The script was therefore only ever usable on 2 of the 6 dumps. The stage is now guarded, so ONE recipe covers the whole corpus. The `gameState`-envelope guard in the jq library is the same class of fix, caught by the control before any write: without it, `.gameState |= ...` would have CREATED a `gameState` key on the four `turn_number`-enveloped dumps, i.e. corrupted them. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): re-anchor the CR 603.5 prompt census after the rebase onto #6842 The census tripwire pins the exact source coordinates of every CR 603.5 prompt producer so that a SIXTH producer is a counted event rather than a silent one. Rebasing this branch onto upstream #6842 (8121fd1c6) moved five of those coordinates, and the row fired. ADJUDICATED, NOT RELAXED. The row fired on COORDINATES, not on population. The producer count is still 5, not 6: game/effects/mod.rs :5896 :5973 :8927 => :5918 :5995 :8949 game/engine.rs :10500 => :10589 game/effects/scoped_library_search.rs :452 => :452 (UNMOVED) `mod.rs` is a uniform +22 shift and `engine.rs` a +89 shift — the signature of lines inserted ABOVE each site, not of a new producer. That `scoped_library_search.rs:452` did NOT move is itself evidence the SET did not change: a genuinely new producer would not leave an untouched file's coordinate fixed while shifting the others by a constant. The producer TEXT at all five sites was verified byte-identical between the pre-rebase tree (`chain3-prefold-backup`, dbc81821d) at the old coordinates and this tree at the new ones. So the expectation array is re-anchored to the new coordinates and the assertion keeps its full strength: a sixth producer still fails this row. Correcting my own earlier report for the record: I previously stated that upstream had ADDED a CR 603.5 producer. That was wrong — the count never left 5, and the failure was purely positional. The correction is carried in the tripwire's own doc block so the next reader of this row does not inherit the mistake. Assisted-by: ClaudeCode:claude-opus-5 * test(engine): re-anchor the CR 603.5 prompt census after the rebase onto #6851 CI built the MERGE ref (branch + main 96e41b3ab) while the branch was still based on e12447f4f, so the census row fired in CI but not locally. Re-anchored per the row's own protocol — re-derive the SET first, prove byte-identity, then move coordinates — never relaxed. MEASURED, three independent ways: 1. Producer SET is still 5 and the partition is still 5/7/25 (total 37). Only ONE coordinate moved: game/engine.rs:10589 => :10640. The other four (game/effects/mod.rs:5918/5995/8949, scoped_library_search.rs:452) are UNMOVED. 2. All five producers re-read at their new coordinates and diffed against the pre-rebase tree at their old ones: BYTE-IDENTICAL. Negative control confirms the diff instrument discriminates — the new tree at the OLD coordinate :10589 is a bare `}`, not the producer. 3. The +51 is fully accounted for by #6851's own insertions above this producer: `git diff -U0 e12447f4f 96e41b3ab` nets +51 above line 10589 and +51 across the whole file, so predicted 10589+51 = 10640 equals the observed coordinate exactly and #6851 adds nothing below it. A sixth producer remains a counted event. Assisted-by: ClaudeCode:claude-opus-5 * fix(engine): close two fail-open seams in the loop-shortcut analysis proofs Addresses maintainer Critical 1 and Medium 6 on #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 * fix(engine): refuse the resolution probe when a prompt already stands Maintainer Critical 2 on #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 * fix(engine,ai): offer only legal quantity choices on a bounded shortcut Maintainer Medium 5 on #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 * fix(scripts): read both trigger-definition shapes and stop overwriting carriers Maintainer Medium 3 on #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 t…
This was referenced Aug 5, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit