Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion client/src/adapter/generated/interaction/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ export type InteractionSlotKind = "single" | "mulligan" | "openingBottom";

export type ActiveInteractionSlot = { semanticOwner: number, slotKind: InteractionSlotKind, interactionId: InteractionId, };

export type SimultaneousDecisionKind = "mulligan" | "openingBottom";
export type SimultaneousDecisionKind = "mulligan" | "openingBottom" | "resolveAllConsent";

export type InteractionWaitingForCode = "terminal" | "mulligan" | "openingBottom" | "choose" | "select" | "sequence" | "relations" | "manaGroups" | "text" | "deckPartition" | "number" | "shortcut" | "assignAmounts" | "assignDamage";

Expand Down
9 changes: 4 additions & 5 deletions client/src/game/__tests__/dispatchResolveAll.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -181,7 +181,7 @@ describe("dispatchResolveAll progress", () => {
expect(submitAction).not.toHaveBeenCalled();
});

it("begins consent before the batch drain and retains its AI seats until Ready", async () => {
it("does not retain AI seats across the consent and Ready calls", async () => {
const seats = [{ playerId: 1, difficulty: "Medium" }];
const submitAction = vi.fn().mockResolvedValue({ events: [] });
const consent = buildGameState({
Expand Down Expand Up @@ -222,13 +222,12 @@ describe("dispatchResolveAll progress", () => {

await dispatchResolveAll(0, []);

expect(resolveAll).toHaveBeenCalledWith(0, seats, 5);
expect(resolveAll).toHaveBeenCalledWith(0, [], 5);
});

it("uses an empty AI-seat list when the adapter delegates native AI ownership to its server", async () => {
it("consumes Ready consent with an empty AI-seat list when the server owns native AI", async () => {
const resolveAll = vi.fn<EngineResolveAll>().mockResolvedValue(chunk(0, 2));
const getState = vi.fn().mockResolvedValue(stateWithStack(0));
const submitAction = vi.fn();
const submitAction = vi.fn().mockResolvedValue({ events: [] });
useGameStore.setState({
gameState: readyStateWithStack(2),
adapter: {
Expand Down
5 changes: 2 additions & 3 deletions client/src/game/controllers/aiController.ts
Original file line number Diff line number Diff line change
Expand Up @@ -156,9 +156,8 @@ export function createAIController(config: AIControllerConfig): AIController {
return null;
}
if (waitingFor.type === "ResolveAllConsent") {
return waitingFor.data.representative === PLAYER_ID
? null
: waitingFor.data.representative;
const { representative } = waitingFor.data;
return aiPlayerIds.has(representative) ? representative : null;
}
if (
!("data" in waitingFor) ||
Expand Down
7 changes: 1 addition & 6 deletions client/src/game/dispatch.ts
Original file line number Diff line number Diff line change
Expand Up @@ -967,7 +967,6 @@ const BATCH_CHUNK_SIZE = 5;
// pathological stacks.
const BATCH_CHUNK_INSTANT = 5_000;
let batchResolveInProgress = false;
let pendingResolveAllSeats: { playerId: number; difficulty: string }[] | null = null;

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1: Module-level pendingResolveAllSeats is unprotected mutable shared state that races between async invocations

Module-level let pendingResolveAllSeats is written in one async branch and read later without synchronization.

Store pending seats inside the game store keyed by gameId instead of a module-level global.

AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="client/src/game/dispatch.ts">
<violation number="1" location="client/src/game/dispatch.ts:971">
<priority>P1</priority>
<title>Module-level `pendingResolveAllSeats` is unprotected mutable shared state that races between async invocations</title>
<evidence>`let pendingResolveAllSeats: { playerId: number; difficulty: string }[] | null = null;` is declared at module scope. In `dispatchResolveAll`, the non-Ready branch writes `pendingResolveAllSeats = aiSeats;` (line ~1018), awaits an async dispatch, and returns. A later call on the Ready branch reads `const resolvedSeats = pendingResolveAllSeats ?? aiSeats;` (line ~1025). If two calls interleave—e.g., a rapid double-click or an AI proposal racing a human click—the second write overwrites the first before the consent flow reaches Ready, so the wrong seat list is used for resolution.</evidence>
<recommendation>Move `pendingResolveAllSeats` into `gameStore` keyed by `gameId` (or by the current consent `epoch`) so it is scoped to a specific game instance and cannot be overwritten by a second concurrent call.</recommendation>
</violation>
</file>

export async function dispatchResolveAll(
requester: number,
Expand Down Expand Up @@ -1009,7 +1008,6 @@ export async function dispatchResolveAll(
// after Ready consumes that already-issued authorization; it never starts a
// second run or asks a future AI decision speculatively.
if (waitingFor?.type !== "ResolveAllReady") {
pendingResolveAllSeats = aiSeats;
const stackLen = useGameStore.getState().gameState?.stack.length ?? 0;
const maxResolutions =
stackPressureFromLength(stackLen) === "Instant"
Expand All @@ -1022,8 +1020,6 @@ export async function dispatchResolveAll(
return;
}

const resolvedSeats = pendingResolveAllSeats ?? aiSeats;

batchResolveInProgress = true;
const { setIsResolvingAll, setResolutionProgress } = useGameStore.getState();
setIsResolvingAll(true);
Expand All @@ -1041,7 +1037,7 @@ export async function dispatchResolveAll(
? BATCH_CHUNK_INSTANT
: BATCH_CHUNK_SIZE;
const batchResult: BatchResolveResult = await batchAdapter.resolveAll(
requester, resolvedSeats, maxResolutions,
requester, aiSeats, maxResolutions,
);

if (latchedTotal === 0) latchedTotal = batchResult.total;
Expand Down Expand Up @@ -1072,6 +1068,5 @@ export async function dispatchResolveAll(
batchResolveInProgress = false;
setIsResolvingAll(false);
setResolutionProgress(null);
pendingResolveAllSeats = null;
}

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2: pendingResolveAllSeats leaks across consent flows when the run exits via Decline or Revoke

pendingResolveAllSeats is only nulled in the Ready-path finally block, leaking on all other exits.

Reactively clear seats when waitingFor leaves ResolveAll states.

AI prompt
Check if this security scanner issue is valid. If so, understand the root cause and fix it. If appropriate, update or add tests. Keep the change focused and preserve intended behavior.

<file name="client/src/game/dispatch.ts">
<violation number="1" location="client/src/game/dispatch.ts:1076">
<priority>P2</priority>
<title>`pendingResolveAllSeats` leaks across consent flows when the run exits via Decline or Revoke</title>
<evidence>`pendingResolveAllSeats` is assigned at line ~1018 in the non-Ready branch and is only cleared inside the `finally` block of the Ready resolution path (`pendingResolveAllSeats = null;`). If a player clicks Decline, if another player revokes consent, if the game disconnects, or if any error short-circuits the flow before Ready, the module-level variable retains the stale seat list until the next Resolve All call.</evidence>
<recommendation>Reactively clear `pendingResolveAllSeats` whenever the store's `waitingFor` transitions away from any `ResolveAll*` state, or move the variable into store-managed state that is automatically reset per game session.</recommendation>
</violation>
</file>

}
5 changes: 4 additions & 1 deletion client/src/game/waitingForRegistry.ts
Original file line number Diff line number Diff line change
Expand Up @@ -33,8 +33,11 @@ export const HANDLED_WAITING_FOR_TYPES: ReadonlySet<WaitingFor["type"]> =
new Set<WaitingFor["type"]>([
// Active priority — passes via PassButton / mana payment / cast.
"Priority",
// ResolveAllConsentModal presents the engine-issued Grant/Decline prompt.
// Resolve All's explicit standing-pass authorization. The consent modal
// gathers each representative's response; its final Grant consumes Ready
// through the engine adapter before ordinary priority resumes.
"ResolveAllConsent",
"ResolveAllReady",
// CR 701.42 / CR 508.4: meld pair and attacking-entry destination dialogs.
"MeldPairChoice",
"MeldAttackTargetChoice",
Expand Down
19 changes: 14 additions & 5 deletions client/src/wasm/engine_wasm.d.ts
Original file line number Diff line number Diff line change
Expand Up @@ -429,11 +429,20 @@ export function restore_game_state(json_str: string): void;
*
* Differs from `restore_game_state` in two load-bearing ways:
*
* 1. **Fresh RNG seed.** `restore_game_state` re-seeds from the saved
* `rng_seed`, which rewinds the ChaCha20 stream to position 0 —
* correct for undo (replay from origin) but wrong for resume
* (subsequent draws would replay the pre-save sequence). This
* function stamps a fresh seed so continued play diverges.
* 1. **Fresh RNG seed.** `restore_game_state` re-seeds from the SAVED
* `rng_seed` and fast-forwards to the saved `rng_word_pos`, so the
* restored game continues the very stream the snapshot was taken on —
* correct for undo, wrong for resume, where continued play must not
* re-draw the values the pre-save timeline already committed to. This
* function stamps a FRESH seed and resets `rng_word_pos` to 0 so the
* resumed host diverges instead.
*
* It does NOT rewind to position 0: that was true only before issue
* #5466 taught the restore path to carry the offset, and it survives
* today just for snapshots written back then, which carry
* `rng_word_pos == 0`. Both the shared decode chokepoint
* (`PersistedGameState::into_game_state`) and `restore_game_state`'s
* own repeat call `rehydrate_rng`.
* 2. **Atomic multiplayer-flag flip.** Sets `MULTIPLAYER_MODE` in the
* same call that loads state, so there's no window where a stray
* `restore_game_state` (undo) would be accepted on the resumed
Expand Down
55 changes: 30 additions & 25 deletions crates/engine-wasm/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,9 @@ use engine::ai_support::{
};
use engine::database::legality::{any_ai_difficulty_is_cedh, validate_cedh_bracket};
use engine::database::{CardDatabase, CardSearchQuery};
use engine::game::engine::{apply, apply_for_simulation};
use engine::game::engine_resolve_batch::resolve_all_ready_prefix;
use engine::game::engine::{
apply, apply_for_simulation, resolve_all_ready_is_authorized, resolve_all_ready_prefix,
};
use engine::game::interaction::{bind_interaction_authority, submit_interaction};
use engine::game::preview::{compute_preview_diff, preview_auto_payment_sources};
use engine::game::{
Expand Down Expand Up @@ -3025,32 +3026,13 @@ pub fn submit_ai_action_proposal(token: &str, actor: u8, action: JsValue) -> JsV
/// events, so the WASM boundary intentionally returns empty event/log arrays
/// instead of serializing thousands of records for pathological stacks.
///
/// Stop conditions (all CR-compliant):
/// - Stack empties
/// - Stack grows beyond the chunk-origin depth
/// - An interactive `WaitingFor` appears (target selection, scry, etc.)
/// - An unknown/non-requester human actor receives priority
/// - AI declines to pass priority
/// - Game ends
/// - Safety cap reached (prevents infinite loops from cascading triggers)
#[expect(
dead_code,
reason = "the legacy Resolve All wire payload remains validated for compatibility, but unanimous engine consent now owns seat decisions"
)]
#[derive(serde::Deserialize)]
#[serde(rename_all = "camelCase")]
struct AiSeatConfig {
player_id: u8,
difficulty: String,
}

#[wasm_bindgen]
pub fn resolve_all(
requester: u8,
ai_seats_json: &str,
max_resolutions: u32,
) -> Result<JsValue, JsValue> {
let ai_seats: Vec<AiSeatConfig> = serde_json::from_str(ai_seats_json)
let _: serde_json::Value = serde_json::from_str(ai_seats_json)
.map_err(|e| JsValue::from_str(&format!("Failed to deserialize AI seats: {e}")))?;

let requester = PlayerId(requester);
Expand All @@ -3061,8 +3043,8 @@ pub fn resolve_all(
// call; Resolve All must never ask an AI about a speculative future
// priority window. Keep the legacy payload parse as a wire-compatible
// boundary while the consent action owns the authoritative cap.
let _ = (ai_seats, max_resolutions);
if !matches!(&state.waiting_for, WaitingFor::ResolveAllReady { .. }) {
let _ = max_resolutions;
if !resolve_all_ready_is_authorized(state, requester) {
return Err(JsValue::from_str("Resolve All consent is not ready"));
}
let mut result = resolve_all_ready_prefix(state, requester);
Expand Down Expand Up @@ -3232,13 +3214,15 @@ mod tests {
use std::sync::Arc;

use engine::game::deck_loading::create_object_from_card_face;
use engine::game::engine::ResolveAllFastForwardResult as BatchResolveResult;
use engine::game::scenario::{GameScenario, P0, P1};
use engine::game::zones::create_object;
use engine::types::ability::{
AbilityCost, AbilityDefinition, AbilityKind, ChoiceType, ChosenAttribute,
ContinuousModification, Duration, Effect, QuantityExpr, QuantityRef, ResolvedAbility,
TargetFilter, TargetRef,
};
use engine::types::actions::ResolveAllConsentDecision;
use engine::types::card::CardFace;
use engine::types::card_type::{CardType, CoreType};
use engine::types::counter::{CounterMatch, CounterType};
Expand Down Expand Up @@ -4494,7 +4478,6 @@ mod tests {
state.active_player = PlayerId(1);
state.turn_decision_controller = Some(PlayerId(0));
state.priority_player = PlayerId(0);
state.priority_passes.insert(PlayerId(0));
state.stack.push_back(no_op_stack_entry(1, PlayerId(1)));
apply(
&mut state,
Expand Down Expand Up @@ -4523,6 +4506,28 @@ mod tests {
));
GAME_STATE.with(|cell| cell.set(Some(state)));

with_state_mut(|state| {
apply(
state,
PlayerId(0),
GameAction::BeginResolveAll { max_resolutions: 0 },
)
.expect("turn controller may begin the consent run");
let WaitingFor::ResolveAllConsent { epoch, .. } = &state.waiting_for else {
panic!("controlled priority should queue Resolve All consent");
};
apply(
state,
PlayerId(0),
GameAction::RespondResolveAllConsent {
epoch: *epoch,
decision: ResolveAllConsentDecision::Grant,
},
)
.expect("frozen turn controller may grant for the queued representative");
})
.expect("test state remains installed");

let value = resolve_all(0, "[]", 0).unwrap();
let result: BatchResolveResult = serde_wasm_bindgen::from_value(value).unwrap();

Expand Down
13 changes: 6 additions & 7 deletions crates/engine/src/ai_support/candidates.rs
Original file line number Diff line number Diff line change
Expand Up @@ -3579,7 +3579,7 @@ fn semantic_candidate_actions_with_probe(
// for broad-only callers, so composing both here would expose every
// Grant, Decline, and Revoke choice twice.
if !matches!(
&state.waiting_for,
state.waiting_for,
WaitingFor::ResolveAllConsent { .. } | WaitingFor::ResolveAllReady { .. }
) {
actions.extend(candidate_actions_broad_with_probe(state, probe));
Expand Down Expand Up @@ -3614,12 +3614,11 @@ fn authorize_candidate_actors(state: &GameState, actions: &mut [CandidateAction]
WaitingFor::ResolveAllConsent {
epoch: active_epoch,
representative,
} if *epoch == *active_epoch => {
Some(crate::game::turn_control::authorized_submitter_for_player(
state,
*representative,
))
}
} if *epoch == *active_epoch => state
.resolve_all_consent_run
.as_ref()
.filter(|run| run.epoch == *active_epoch)
.and_then(|run| run.authorized_submitter_for(*representative)),
_ => None,
},
GameAction::RevokeResolveAllConsent {
Expand Down
64 changes: 43 additions & 21 deletions crates/engine/src/game/engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,8 @@ use super::zone_pipeline::{self, ZoneMoveRequest, ZoneMoveResult};
use super::zones;

pub use super::engine_resolve_batch::{
resolve_all_fast_forward, ResolveAllCallbackDecision, ResolveAllFastForwardResult,
resolve_all_fast_forward, resolve_all_ready_is_authorized, resolve_all_ready_prefix,
ResolveAllCallbackDecision, ResolveAllFastForwardResult,
};

#[derive(Debug, Clone, Error)]
Expand Down Expand Up @@ -7612,6 +7613,9 @@ fn begin_resolve_all_consent(
EngineError::ActionNotAllowed("Resolve All consent epoch space exhausted".to_string())
})?;
state.next_resolve_all_consent_epoch = next_epoch;
// CR 117.4: a stack object resolves only after every player passes in
// succession. Preserve the exact current pass cycle if consent is declined
// or revoked before its authorized one-entry materialization begins.
state.resolve_all_consent_run = Some(ResolveAllConsentRun {
epoch,
max_resolutions,
Expand Down Expand Up @@ -7689,26 +7693,34 @@ fn respond_resolve_all_consent(
"Resolve All consent response is no longer pending".to_string(),
));
}
match decision {
ResolveAllConsentDecision::Grant => {
let participant = run
.participants
.iter_mut()
.find(|participant| participant.representative == representative)
.expect("pending Resolve All representative must be a participant");
participant.granted = true;
}
ResolveAllConsentDecision::Decline => {}
if matches!(decision, ResolveAllConsentDecision::Grant) {
let participant = run
.participants
.iter_mut()
.find(|participant| participant.representative == representative)
.expect("pending Resolve All representative must be a participant");
participant.granted = true;
}
}
match decision {
ResolveAllConsentDecision::Decline => restore_resolve_all_priority_snapshot(state),
ResolveAllConsentDecision::Grant => {
resolve_all_consent_waiting_for(state).ok_or_else(|| {
EngineError::InvalidAction("Resolve All consent is not active".to_string())
})
}
if matches!(decision, ResolveAllConsentDecision::Decline) {
return restore_resolve_all_priority_snapshot(state);
}
let waiting_for = resolve_all_consent_waiting_for(state).ok_or_else(|| {
EngineError::InvalidAction("Resolve All consent is not active".to_string())
})?;
// ResolveAllReady has no current actor, so the ordinary waiting-state sync
// deliberately leaves `priority_player` alone. Restore the saved priority
// cursor now; the Ready consumer validates this exact snapshot before it
// begins its first materialized CR 117.4 pass cycle.
if matches!(waiting_for, WaitingFor::ResolveAllReady { .. }) {
state.priority_player = state
.resolve_all_consent_run
.as_ref()
.expect("an active consent run produced ResolveAllReady")
.priority_snapshot
.priority_player;
}
Ok(waiting_for)
}

fn revoke_resolve_all_consent(
Expand Down Expand Up @@ -8089,7 +8101,11 @@ fn apply_action(
let stack_len_before_action = state.stack.len();
if !matches!(
action,
GameAction::PassPriority | GameAction::OrderTriggers { .. }
GameAction::PassPriority
| GameAction::OrderTriggers { .. }
| GameAction::BeginResolveAll { .. }
| GameAction::RespondResolveAllConsent { .. }
| GameAction::RevokeResolveAllConsent { .. }
) && !answering_forced_window
{
state.loop_detect_ring.clear();
Expand All @@ -8110,7 +8126,10 @@ fn apply_action(
match &action {
GameAction::SetAutoPass { .. }
| GameAction::PassPriority
| GameAction::ReorderHand { .. } => {}
| GameAction::ReorderHand { .. }
| GameAction::BeginResolveAll { .. }
| GameAction::RespondResolveAllConsent { .. }
| GameAction::RevokeResolveAllConsent { .. } => {}
_ => {
state.auto_pass.remove(&actor);
}
Expand Down Expand Up @@ -19763,7 +19782,10 @@ mod stage2_injector_tests {
// `begin_pending_trigger_target_selection` is STILL 134 — the function opens
// `:12722 ⇒ :12778`, moving by the same `+56` as the pin, so the control
// that caught this row's one historical silent drift is intact.
"game/engine.rs:13094".to_string(),
// Resolve All consent adds its frozen-authority protocol above this producer:
// `:12912 ⇒ :13113`. It does not create a CR 603.5 prompt, and the pinned
// line remains the same `OptionalEffectChoice` construction.
"game/engine.rs:13113".to_string(),
],
"the five production producers, NAMED: the CR 603.5 gate in `resolve_chain_body` \
plus the two repeated-optional-payment drivers, the per-player acceptance cursor \
Expand Down
Loading
Loading