fix(engine, ai): deterministic ordering for AI decision trajectories (#4878)

.github/workflows/ci.yml

@@ -250,9 +250,10 @@ jobs:
         # PR's parse changes, with no second coverage build and no cross-PR
         # contamination — even when the PR is behind main, or has main merged
         # into it (both sides move in lockstep because the base is read off the
-        # merge commit, not the frozen webhook base.sha). Path filter: identical
-        # engine-source hash => no parse
-        # change is possible => skip before the bin build. --features cli keeps
+        # merge commit, not the frozen webhook base.sha). Skip gate: identical
+        # parser-source hash => no Oracle/parser code changed => parse-diff
+        # cannot reflect real PR parse work (runtime/AI-only diffs are noise).
+        # Baseline fetch still keys off engine-source-hash (R2 object names).
         # the same [tool, cli] engine fingerprint as the steps above (lib cache
         # hit; only the new bin links).
         id: parsediff
@@ -289,6 +290,13 @@ jobs:
           git fetch --no-tags --depth=1 origin "$BASE_SHA"
           BASE_HASH="$(./scripts/engine-source-hash.sh "$BASE_SHA")"
           HEAD_HASH="$(./scripts/engine-source-hash.sh HEAD)"
+          BASE_PARSER_HASH="$(./scripts/parser-source-hash.sh "$BASE_SHA")"
+          HEAD_PARSER_HASH="$(./scripts/parser-source-hash.sh HEAD)"
+          if [ "$BASE_PARSER_HASH" = "$HEAD_PARSER_HASH" ]; then
+            echo "Parser surface unchanged vs base ($BASE_PARSER_HASH) — no parse change possible; skipping."
+            echo "produced=false" >> "$GITHUB_OUTPUT"
+            exit 0
+          fi
           if [ "$BASE_HASH" = "$HEAD_HASH" ]; then
             echo "Engine source unchanged vs base ($BASE_HASH) — no parse change possible; skipping."
             echo "produced=false" >> "$GITHUB_OUTPUT"

crates/engine/src/ai_support/mod.rs

@@ -56,7 +56,13 @@ pub fn validated_candidate_actions_with_probe(
     probe: Option<&crate::game::casting::PriorityCastProbe>,
 ) -> Vec<CandidateAction> {
     let pipeline = FilterPipeline::default_pipeline();
-    pipeline.apply_with_probe(state, candidate_actions_with_probe(state, probe), probe)
+    let mut actions =
+        pipeline.apply_with_probe(state, candidate_actions_with_probe(state, probe), probe);
+    // Issue #4878: candidate enumeration must not depend on HashSet/HashMap
+    // iteration order leaking into AI tie-breaking downstream. Ordered via the
+    // allocation-free `GameAction::cmp_stable` total order (not `Debug` strings).
+    actions.sort_by(|a, b| a.action.cmp_stable(&b.action));
+    actions
 }
 
 /// CR 702.51a / 702.66a / 702.126a: During `ManaPayment`, every structurally
@@ -5061,6 +5067,66 @@ mod tests {
         );
     }
 
+    /// Issue #4878: `validated_candidate_actions` must return candidates in the
+    /// canonical `GameAction::cmp_stable` order, independent of the upstream
+    /// enumeration order (which walks `state.objects`, an `im::HashMap`, in
+    /// hash order — not sorted for this id set). Reverting the
+    /// `sort_by(cmp_stable)` guard returns the candidates in that hash order,
+    /// which differs from the canonical order below, flipping this assertion.
+    #[test]
+    fn validated_candidates_are_cmp_stable_sorted() {
+        let mut state = setup_priority();
+        // CR 305.2 + CR 505: land drop available in the precombat main phase so
+        // each land in hand survives the simulation filter as a `PlayLand`.
+        state.phase = Phase::PreCombatMain;
+        state.lands_played_this_turn = 0;
+        for _ in 0..10 {
+            let id = create_object(
+                &mut state,
+                CardId(1),
+                PlayerId(0),
+                "Forest".to_string(),
+                Zone::Hand,
+            );
+            state
+                .objects
+                .get_mut(&id)
+                .unwrap()
+                .card_types
+                .core_types
+                .push(CoreType::Land);
+        }
+
+        let actions: Vec<GameAction> = validated_candidate_actions(&state)
+            .iter()
+            .map(|c| c.action.clone())
+            .collect();
+
+        // Reach guard: the scenario must actually offer several distinct actions
+        // (multiple `PlayLand` + `PassPriority`), otherwise a single-element list
+        // would be trivially "sorted" and the assertion vacuous.
+        assert!(
+            actions.len() >= 3,
+            "expected several candidate actions to canonicalize, got {}",
+            actions.len()
+        );
+
+        // Canonical order: identical to an independent `cmp_stable` sort.
+        let mut expected = actions.clone();
+        expected.sort_by(|a, b| a.cmp_stable(b));
+        assert_eq!(
+            actions, expected,
+            "validated_candidate_actions must emit cmp_stable-canonical order"
+        );
+
+        // Determinism: a second call over the same state yields the same order.
+        let again: Vec<GameAction> = validated_candidate_actions(&state)
+            .iter()
+            .map(|c| c.action.clone())
+            .collect();
+        assert_eq!(actions, again, "candidate order must be deterministic");
+    }
+
     /// CR 117.3d: a matching priority yield for the top-of-stack trigger makes
     /// `auto_pass_recommended` return `true`, overriding the meaningful-action
     /// hold that would otherwise keep the window open. Reverting the yield short-

crates/engine/src/game/casting.rs

@@ -28,7 +28,7 @@ use crate::types::statics::{
 };
 use crate::types::zones::{ExileCostSourceZone, Zone};
 
-use std::collections::HashSet;
+use std::collections::{BTreeSet, HashSet};
 
 use super::ability_utils::{
     ability_target_legality_needs_chosen_x, assign_targets_in_chain, auto_select_targets,
@@ -867,7 +867,7 @@ pub fn spell_objects_available_to_cast(state: &GameState, player: PlayerId) -> V
     // CR 117.1c: per-turn frequency is enforced inside the helper, not by
     // active-player gating, so the same logic covers the rare case of an
     // `Unlimited` printing on either player's turn.
-    let exile_permission_ids: HashSet<ObjectId> =
+    let exile_permission_ids: BTreeSet<ObjectId> =
         exile_objects_castable_by_permission(state, player)
             .iter()
             .map(|(obj_id, _source_id, _freq)| *obj_id)

crates/engine/src/game/combat.rs

@@ -90,7 +90,7 @@ pub enum TrampleKind {
 }
 
 /// Represents who a creature is attacking: a player, planeswalker, or battle (CR 506.3).
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 #[serde(tag = "type", content = "data")]
 pub enum AttackTarget {
     Player(PlayerId),

crates/engine/src/game/game_object.rs

@@ -219,7 +219,7 @@ pub struct CaseState {
 /// truth and lets exhaustive `match` arms force every consumer to handle both
 /// variants. Equipment-only call sites use `as_object()` with a CR-cited
 /// `expect` to assert the rules invariant.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 #[serde(tag = "type", content = "data")]
 pub enum AttachTarget {
     /// CR 301.5 / CR 303.4f: attached to a permanent.
@@ -260,7 +260,7 @@ impl From<ObjectId> for AttachTarget {
 
 /// CR 709.5c: Which half, or door, of a shared-type-line split permanent is
 /// being locked or unlocked.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 pub enum RoomDoor {
     Left,
     Right,

crates/engine/src/game/layers.rs

@@ -1,4 +1,4 @@
-use std::collections::{HashMap, HashSet};
+use std::collections::{BTreeSet, HashMap, HashSet};
 use std::sync::Arc;
 
 use crate::database::synthesis::KeywordTriggerInstaller;
@@ -73,7 +73,7 @@ impl LayerZoneObjectCache {
 }
 
 struct PreparedIncrementalFlush {
-    recipient_ids: HashSet<ObjectId>,
+    recipient_ids: BTreeSet<ObjectId>,
     active_effects: Vec<ActiveContinuousEffect>,
 }
 
@@ -2258,7 +2258,7 @@ pub fn flush_layers(state: &mut GameState) {
 
 fn prepare_incremental_flush(
     state: &mut GameState,
-    entered_ids: &HashSet<ObjectId>,
+    entered_ids: &BTreeSet<ObjectId>,
 ) -> Option<PreparedIncrementalFlush> {
     for &id in entered_ids {
         let obj = state.objects.get(&id)?;
@@ -2329,7 +2329,7 @@ fn prepare_incremental_flush(
 #[cfg(test)]
 pub(crate) fn incremental_flush_must_escalate(
     state: &GameState,
-    entered_ids: &HashSet<ObjectId>,
+    entered_ids: &BTreeSet<ObjectId>,
 ) -> bool {
     // Axis 1 — per-entered preconditions.
     for &id in entered_ids {
@@ -2384,7 +2384,7 @@ pub(crate) fn incremental_flush_must_escalate(
 
 fn active_effects_force_incremental_escalation(
     state: &GameState,
-    entered_ids: &HashSet<ObjectId>,
+    entered_ids: &BTreeSet<ObjectId>,
     active_effects: &[ActiveContinuousEffect],
 ) -> bool {
     active_effects.iter().any(|e| {
@@ -2453,7 +2453,7 @@ fn active_effects_force_incremental_escalation(
 /// (invariant 5), so the cached BEFORE truth aligns with the consulted def.
 fn any_active_static_condition_perturbed_by_entry(
     state: &GameState,
-    entered_ids: &HashSet<ObjectId>,
+    entered_ids: &BTreeSet<ObjectId>,
 ) -> bool {
     let mut found = false;
     for_each_static_effect_source(state, |state, obj| {
@@ -2617,8 +2617,8 @@ fn entered_object_blocks_incremental(
 /// continuous static applies to the enchanted permanent, not itself).
 fn incremental_recipient_ids(
     state: &GameState,
-    entered_ids: &HashSet<ObjectId>,
-) -> HashSet<ObjectId> {
+    entered_ids: &BTreeSet<ObjectId>,
+) -> BTreeSet<ObjectId> {
     let mut recipients = entered_ids.clone();
     for &id in entered_ids {
         if let Some(host) = state
@@ -2635,7 +2635,7 @@ fn incremental_recipient_ids(
 
 fn effect_is_restricted_to_incremental_recipients(
     effect: &ActiveContinuousEffect,
-    recipient_ids: &HashSet<ObjectId>,
+    recipient_ids: &BTreeSet<ObjectId>,
 ) -> bool {
     match &effect.affected_filter {
         TargetFilter::SelfRef => recipient_ids.contains(&effect.source_id),
@@ -3716,7 +3716,7 @@ fn apply_combat_assignment_rule_effects(state: &mut GameState) {
 /// object-affecting continuous effects.
 fn apply_combat_assignment_rule_effects_filtered(
     state: &mut GameState,
-    restrict_to: Option<&HashSet<ObjectId>>,
+    restrict_to: Option<&BTreeSet<ObjectId>>,
 ) {
     let mut effects = collect_active_combat_assignment_rule_effects(state);
     effects.sort_by_key(|effect| (effect.timestamp, effect.controller.0, effect.source_id.0));
@@ -3771,7 +3771,10 @@ fn apply_combat_assignment_rule_effects_filtered(
 /// affected objects. This is run AFTER all keyword grants/removals are applied,
 /// so the denial wins regardless of grant timestamp — the rules-correct "can't
 /// have" outcome (a concurrent anthem can't restore a denied keyword).
-fn apply_cant_have_keyword_denials(state: &mut GameState, restrict_to: Option<&HashSet<ObjectId>>) {
+fn apply_cant_have_keyword_denials(
+    state: &mut GameState,
+    restrict_to: Option<&BTreeSet<ObjectId>>,
+) {
     // Collect (affected object, denied keyword) pairs under an immutable borrow,
     // then strip — avoids a borrow conflict with the per-object mutation.
     let mut denials: Vec<(ObjectId, Keyword)> = Vec::new();
@@ -4394,7 +4397,7 @@ fn apply_continuous_effect(
 fn apply_continuous_effect_to(
     state: &mut GameState,
     effect: &ActiveContinuousEffect,
-    restrict_to: &HashSet<ObjectId>,
+    restrict_to: &BTreeSet<ObjectId>,
     abilities_suppressed: &mut HashSet<ObjectId>,
     zone_cache: &mut LayerZoneObjectCache,
 ) {
@@ -4410,7 +4413,7 @@ fn apply_continuous_effect_to(
 fn apply_continuous_effect_filtered(
     state: &mut GameState,
     effect: &ActiveContinuousEffect,
-    restrict_to: Option<&HashSet<ObjectId>>,
+    restrict_to: Option<&BTreeSet<ObjectId>>,
     abilities_suppressed: &mut HashSet<ObjectId>,
     zone_cache: &mut LayerZoneObjectCache,
 ) {

crates/engine/src/game/merge.rs

@@ -65,7 +65,9 @@ use crate::types::zones::Zone;
 /// Serializes as the plain variant string ("Top" / "Bottom") so the frontend
 /// `GameAction::ChooseMutateMergeSide` payload is `{ side: "Top" | "Bottom" }`,
 /// parallel to the sibling `ChooseTopOrBottom { top: bool }`.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, serde::Serialize, serde::Deserialize)]
+#[derive(
+    Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, serde::Serialize, serde::Deserialize,
+)]
 pub enum MergeSide {
     /// The mutating spell is placed on TOP of the target creature — the spell's
     /// card/token supplies the copiable characteristics.

crates/engine/src/game/stack.rs

@@ -7889,7 +7889,7 @@ mod tests {
             let mut normal = setup_board();
             flush_layers(&mut normal);
             add_entry(&mut normal);
-            let entered_ids: std::collections::HashSet<ObjectId> = match &normal.layers_dirty {
+            let entered_ids: std::collections::BTreeSet<ObjectId> = match &normal.layers_dirty {
                 crate::types::game_state::LayersDirty::EnteredObjects(ids) => ids.clone(),
                 other => panic!("expected EnteredObjects dirty state, got {other:?}"),
             };
@@ -8662,7 +8662,7 @@ mod tests {
             flush_layers(&mut state);
             // A green entry perturbs the < 7 gate (would flip 6 → 7).
             add_green_devotion_entry(&mut state, 322);
-            let entered_ids: std::collections::HashSet<ObjectId> = match &state.layers_dirty {
+            let entered_ids: std::collections::BTreeSet<ObjectId> = match &state.layers_dirty {
                 crate::types::game_state::LayersDirty::EnteredObjects(ids) => ids.clone(),
                 other => panic!("expected EnteredObjects, got {other:?}"),
             };

crates/engine/src/types/ability.rs

@@ -4556,7 +4556,7 @@ pub enum TapStateChange {
 /// inexpressible boolean combination. Parameterizes the lock/unlock axis — both
 /// live within CR rule section 709.5 — into one effect variant instead of two
 /// sibling effects.
-#[derive(Debug, Clone, Copy, PartialEq, Eq, Serialize, Deserialize)]
+#[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]
 #[serde(tag = "type")]
 pub enum DoorLockOp {
     /// CR 709.5f: choose a locked half and give it the unlocked designation.
@@ -19085,7 +19085,7 @@ pub enum ContinuousModification {
 // ---------------------------------------------------------------------------
 
 /// Unified target reference for creatures and players.
-#[derive(Debug, Clone, PartialEq, Eq, Hash, Serialize, Deserialize)]
+#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Hash, Serialize, Deserialize)]
 pub enum TargetRef {
     Object(ObjectId),
     Player(PlayerId),