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

[andriypolanski]

Summary

Two cold runs of the same AI workload with identical card data, seed, and action cap could produce different game trajectories because std::collections::HashSet / HashMap with the default RandomState leaked allocation-order-dependent iteration into engine and AI paths. This change makes those paths use deterministic ordering so fixed-seed AI runs and perf-gate counters are reproducible.

Fixes #4878.

Root cause

RandomState::default() seeds each new HashSet/HashMap from a thread-local counter, so iteration order varies per allocation — both across processes and between sequential in-process runs. When that order influences which legal action is enumerated first, which layer recipients are processed, or how softmax breaks score ties, the macro game line diverges (observed via trajectory-coupled perf counters such as state_clone_for_legality and static_full_scans).

Changes

Engine — incremental layers (CR 613-sensitive)

• LayersDirty::EnteredObjects now stores BTreeSet<ObjectId> instead of HashSet<ObjectId>.
• ObjectId derives PartialOrd / Ord (transparent over u64).
• Incremental flush helpers in layers.rs and parity tests in stack.rs use BTreeSet for entered/recipient sets and restrict_to filters.

The incremental path was already only taken when coalescing order cannot change layer semantics relative to a full pass; this change fixes which deterministic order is used (ObjectId ascending), not whether incremental flush is allowed.

Engine — spell candidate enumeration

• spell_objects_available_to_cast: exile-permission object ids are collected into a BTreeSet before extending the castable spell list (previously extended from a HashSet in hash iteration order).

Engine — validated candidate pipeline

• validated_candidate_actions_with_probe sorts surviving candidates by Debug action key before returning, so downstream consumers (projection first()/last(), fallback paths, AI scoring input order) do not depend on upstream collection iteration order.

AI — search and selection

• score_candidates_core: always sorts gated and scored action lists with action_order_key (removed measurement-mode-only guards).
• choose_action: always sorts scored pairs before softmax selection.
• softmax_select_pairs: when falling back to max score, breaks ties with action_order_key.

Out of scope (follow-ups)

• Tightening ai-perf-gate from median-of-5 cross-process sampling back to K=1 once this lands (see comments in crates/phase-ai/src/duel_suite/perf.rs).
• Hunting any remaining HashSet/HashMap iteration sites outside these hot paths if jitter persists on the full red-mirror workload.

Test plan

• cargo fmt --all
• ./scripts/tilt-wait.sh clippy test-engine test-ai
• Run the perf-gate in-process diagnostic (red-mirror, fixed seed): two consecutive in-process runs should report zero counter jitter for trajectory-coupled counters (state_clone_for_legality, static_full_scans, legal_actions_spell_cost_sweeps, etc.).
• Optional: two cold-process runs of the same perf workload should match on stable counters once gate baselines are refreshed.
• Spot-check incremental layer parity tests still pass (stack.rs EnteredObjects vs Full reference paths).

Risk notes

• Layers: CR 613 conformance was previously audited (feat(engine): CR 613 layers conformance — face-down override, timestamp assignment, granted-ability timestamps #4620). This does not change when incremental flush is taken — only the iteration order within that path. Full flush behavior is unchanged.
• AI play style: Sorting candidates and tie-breaking may slightly change softmax sampling weights when many actions had near-equal scores under the old arbitrary order; behavior should be more stable, not more random.

[gemini-code-assist]

Code Review

This pull request addresses issue #4878 by replacing several instances of HashSet with BTreeSet (such as for ObjectId collections) and introducing deterministic sorting of candidate actions to prevent non-deterministic iteration order from leaking into AI tie-breaking. Feedback on the changes highlights a performance bottleneck in crates/engine/src/ai_support/mod.rs, where sorting candidate actions relies on expensive string formatting; it is recommended to use direct comparison instead.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

[matthewevans]

[MED] Avoid sorting hot candidate paths through Debug strings. Evidence: crates/engine/src/ai_support/mod.rs:55 and crates/phase-ai/src/search.rs:1831. Why it matters: legal_actions / AI scoring are hot paths, and this now allocates a formatted String for every candidate on every call, then repeats the same key construction at multiple scoring/selection boundaries; that risks regressing the decision-cost perf work this PR is trying to stabilize. Suggested fix: use a typed stable action comparator/key (variant rank plus relevant ids/payloads, or a centralized reusable key that does not rely on Debug formatting) and sort only at the boundaries that actually need a canonical order.

[MED] Add revert-discriminating coverage for the newly changed non-measurement ordering paths. Evidence: crates/phase-ai/src/search.rs:5388 tests measurement mode, while the diff shows the pre-existing code already sorted under config.execution_mode.is_measurement() at the modified scoring sites. Why it matters: the visible test would still pass if the newly removed measurement guards and the validated_candidate_actions_with_probe sort were reverted, so the actual fixed bug class is not pinned. Suggested fix: add a non-measurement equal-score action selection test and/or a validated_candidate_actions ordering test that fails when candidate order falls back to hash iteration.

[github-actions]

Parse changes introduced by this PR · 4 card(s), 5 signature(s) (baseline: main 57684e586cac)

2 card(s) · ability/ApplyPerpetual · removed: ApplyPerpetual

Examples: Chronicler of Worship, Lumbering Lightshield

2 card(s) · replacement/Moved · added: Moved

Examples: Famished Worldsire, Feasting Hobbit

2 card(s) · replacement/Moved · removed: Moved

Examples: Famished Worldsire, Feasting Hobbit

1 card(s) · ability/RaiseCost · added: RaiseCost (affects=any target, duration=until end of turn, grants=RaiseCost, target=any target)

Examples: Lumbering Lightshield

1 card(s) · ability/ReduceCost · added: ReduceCost (affects=any target, duration=until end of turn, grants=ReduceCost, target=any target)

Examples: Chronicler of Worship

2 card(s) had Oracle-text changes (errata/reprint) — excluded as non-parser.

[matthewevans]

[MED] Make cmp_payload exhaustively fail when GameAction grows. Evidence: crates/engine/src/types/action_stable_order.rs:34 and crates/engine/src/types/action_stable_order.rs:620. Why it matters: this module is now the single authority for stable action ordering, but the wildcard arm means a future GameAction variant can compile while all same-variant payloads compare Equal, reintroducing input-order tie behavior at the exact seam this PR is centralizing. Suggested fix: structure the comparator so matching on GameAction variants is exhaustive without a wildcard (for example, match on a first and then the same b variant, with mismatched variants unreachable), and do the same for the debug-action payload helper if it is intended to be maintained alongside the enum.

[matthewevans]

[MED] Explain or eliminate the parse-detail blast radius before enqueue. Evidence: the current <!-- coverage-parse-diff --> comment for this head reports 66 card changes / 31 signatures, including Discarded target/watch-scope changes, added/removed abilities, and Korvold/Tidus/Drix ability changes, while the PR body and changed files are scoped to deterministic action ordering. Why it matters: parser/card-data semantic changes that are not part of the claimed fix can silently change supported card behavior, and the review protocol requires unexplained gained/lost/changed cards to block queueing. Suggested fix: either rebase/regenerate until the parse diff is empty or reduce it to ordering-only noise, or document and test every intended card-level semantic change as part of this PR.

[matthewevans]

[MED] Explain or eliminate the remaining parse-detail blast radius before enqueue. Evidence: the current <!-- coverage-parse-diff --> comment for head cfb7513a112dce73be3595951b93ac67adfe7ab6 still reports 66 card changes / 31 signatures, including discard trigger scope changes and unrelated ability changes such as Korvold, Drix, and Tidus. Why it matters: this PR is scoped to deterministic AI/action ordering, so parser/card-data changes of this size are either unrelated scope contamination or need an explicit, reviewable causal explanation. Suggested fix: restore parser/card-data behavior for unrelated cards, or split and justify the parser changes with targeted tests and a PR scope that names them.

[andriypolanski]

Hi, @matthewevans

this pr is waiting your feedback during long time.

[matthewevans]

This is a PR that might require more time being spent on it @andriypolanski - I need to ensure there are not negative performance implications.

[matthewevans]

It also feels like apply_with_probe returning a BTreeSet would prevent this manual sort afterwards.

[matthewevans]

@andriypolanski if you're going to take on a ticket like this then we need performance metrics. I appreciate the effort towards this, but it needs more proof to ensure it's stable enough to ship.

Parser/engine related issues are far easier to review & get merged :)

[andriypolanski]

Hi, @matthewevans
I think that the architectural direction (typed cmp_stable total order replacing Debug string sorts) is sound, and the trajectory stabilizes at the macro level. But shipping without a passing perf gate + M15 validation would violate the project's own enqueue bar — especially for a change that touches AI hot paths and intentionally shifts decision order.

I'd treat this as needs another iteration: diagnose why state_clone_for_legality climbed (+421), run M15, I will regenerate baseline only if margin-validated, then attach the metrics table to the PR. :)