refactor: unify resolution-continuation drains onto a resolution_stack

[dripsmvcp]

Problem

GameState accumulated ~11 flat pending_* resolution-continuation fields drained by ~9 drain_pending_* functions, hand-sequenced inside drain_pending_continuation with load-bearing ordering and waits_for_resolution_choice cross-guards. Each new interactive iteration effect had to add a field, init it, add a PartialEq line, write a drain fn, and splice into the fragile ordering — an O(n²) coupling and a correctness surface for nested choices (the ordering was encoded implicitly in call order, not in data).

Solution

Introduce one GameState::resolution_stack: Vec<ResolutionFrame> driven by a single drive_resolution, replacing the per-effect flat fields with typed frames:

• Chain — a stashed ability chain (replay after a choice)
• Iterate { remaining, accumulate, ctx } — generic per-player/per-iteration loop (subsumes vote-ballot iteration and choose_one_of's remaining-players)
• ResumeChangeZone / ResumeCopyToken — saved mid-loop resolver contexts
• CounterMoves / CounterAdditions / BatchDeliveries — drain queues

The driver advances each frame at the same sequence position the legacy drain occupied, so the load-bearing interleaving (e.g. a vote ballot nesting a ChangeZone iteration) is preserved exactly. Every match is exhaustive (no wildcard).

Scope

Unifies 7 continuation types onto the stack. pending_continuation, pending_repeat_iteration, and pending_repeat_until are intentionally left on the legacy path as scoped follow-ups: the first carries intra-frame chain composition (append/prepend_to_pending_continuation, where append-tail is the opposite of LIFO), and repeat_until fires on a 3-way quiescence guard rather than a stack position — both need dedicated frame semantics (FrameGate::QuiescentAtPriority) and are safer as separate changes.

Delivered as 5 reviewable commits

Slice 0 scaffolding (byte-identical) → 1 Iterate frame → 2 resolver-resume/queue frames → 3 remove the 7 dead flat fields → 4 serde back-compat + round-trip tests.

Verification

• cargo test -p engine: 13,391 passed, 0 failed (incl. 3 new serde/exhaustiveness tests + the per-player Iterate end-to-end test)
• cargo clippy -p engine --all-targets -- -D warnings: 0 warnings
• cargo test -p phase-ai --lib: 885 ✓ · cargo test -p mtgish-import --lib: 125 ✓ · server-core builds

Multiplayer: resolution_stack carries the same payloads the (never-redacted) flat fields did, so it introduces no new hidden-information leak vs. main. Save back-compat: #[serde(default)] loads old saves (no resolution_stack key) to an empty stack — covered by a test.

[dripsmvcp]

@gemini-code-assist review

[gemini-code-assist]

Code Review

This pull request refactors the resolution-continuation machinery by introducing a unified LIFO resolution_stack on GameState to replace several flat pending_* fields, porting vote ballots and choose-one-of branches onto a generic Iterate frame. The changes update various effect handlers and engine drivers to use stack-based accessors, and add robust serialization and integration tests to ensure backward compatibility and round-trip safety. Review feedback recommends adding a mandatory CR 608.2c annotation to the ResolutionFrame::Chain variant to comply with the style guide, and suggests using VecDeque::from instead of .into_iter().collect() in choose_one_of.rs for a cleaner conversion.

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.

[gemini-code-assist]

[MEDIUM] Missing CR annotation on ResolutionFrame::Chain.\n\nWhy it matters: Under R6 (CR annotations are mandatory and verified), every rules-touching line or variant representing a rules-defined continuation must carry a CR <number> annotation. The Chain variant represents a stashed ability chain continuation (CR 608.2c).\n\nSuggested fix: Add the CR 608.2c annotation to the doc comment.

Suggested change

	/// A stashed ability chain to replay once the current choice resolves.
	Chain(PendingContinuation),
	/// CR 608.2c: A stashed ability chain to replay once the current choice resolves.
	Chain(PendingContinuation),

References

1. Every rules-touching line of engine code must carry a comment of the form CR : (regex CR \d{3}(.\d+[a-z]?)?). ^(link)

[gemini-code-assist]

[MEDIUM] Use VecDeque::from instead of .into_iter().collect() for cleaner conversion.\n\nWhy it matters: Converting a Vec directly to a VecDeque using VecDeque::from is more idiomatic and concise than mapping through an iterator.\n\nSuggested fix: Replace the iterator collection with VecDeque::from.

    let mut players = VecDeque::from(choosing_players(state, ability, chooser));

[gemini-code-assist]

Warning

Gemini encountered an error creating the review. You can try again by commenting /gemini review.

[matthewevans]

Maintainer review — refactor: unify resolution-continuation drains onto a resolution_stack

Verdict: hold (large-refactor value gate not cleared). This is a high-blast-radius mechanical refactor (25 files, +1307/−302) over the casting/resolution-continuation machinery — code with many callers and a history of subtle regressions (the Scry/resolution-choice park-in-deferred clobber). The genuinely valuable slice is entangled with a large amount of inert scaffolding, so I'm asking to split rather than land as-is. Detail and the explicit value-gate answers below.

What this PR actually delivers now

1. Real unification (valuable): the per-player Vote and ChooseOneOf drains are ported onto a new ResolutionFrame::Iterate frame (IterItem / IterCtx / IterKind). This collapses two near-identical hand-stashed Pending*Iteration structs + resume loops into one generic iteration frame. The end-to-end Expropriate vote test driving all three ballots through apply in APNAP order is a genuinely discriminating addition.
2. Storage relocation (not unification): the other five continuations — change_zone_iteration, copy_token_resolution, counter_moves, counter_additions, batch_deliveries — are moved into ResolutionFrame variants but accessed through Option-shaped accessors (take_* / set_* / has_* / *_ref) that linearly search the stack by variant (rposition). Behavior is deliberately byte-identical: drive_resolution is a literal 1:1 alias of drain_pending_continuation, and the fixed drain ordering in that function is unchanged. So for five of seven continuations this is Option<Pending*> field → Vec<ResolutionFrame> + per-kind accessor, with no semantic change and no LIFO nesting actually used.
3. Dead/reserved scaffolding: ResolutionFrame::Chain(PendingContinuation) is defined but never pushed or popped anywhere in production — pending_continuation remains a separate flat field that drain_pending_continuation still drains. Accumulator::TrackedSet and the non-Player IterItem variants are explicitly "reserved for a later slice." The PR is self-described as "Slice 1/5."

Value-gate answers

• Debt retired, at which seam: real reduction only at the Vote/ChooseOneOf iteration seam (two structs + two resume fns → one Iterate frame). The other five conversions retire no concept — the Option API is faithfully re-exposed over a Vec, and a new linear-search accessor layer is added.
• Cases cleaner / newly possible: Vote + ChooseOneOf iteration. No card class becomes newly possible in this PR.
• Hot-path bool→domain-type or cosmetic: mostly cosmetic relocation. The one new domain type that earns its keep (IterKind) serves only two call sites today.
• Fewer concepts / clearer boundaries: net more surface right now — one new enum family (ResolutionFrame/IterKind/IterItem/Accumulator) plus ~14 accessor methods alongside the retained pending_continuation flat field and an unused Chain variant. The promised simplification lands only after slices 2–5.
• Blast radius vs value: 25 files across effect handlers, the engine driver, replacement and resolution-choice handlers, and GameState PartialEq/serde. That is disproportionate to "ported two of seven drains"; the remaining churn is carrying future slices' cost into this one.
• Sibling raw fields still present: yes — pending_continuation, pending_repeat_iteration, pending_repeat_until, pending_coin_flip, etc. remain flat fields. The partial conversion leaves two parallel shapes (flat fields + stack frames) for the resolution machinery simultaneously, which is harder to reason about than either end state.

Regression risk

drive_resolution's exact equivalence to drain_pending_continuation is the only thing protecting the delicate drain ordering (change_zone → vote → repeat → choose_one_of, each gated on !waits_for_resolution_choice). The accessor take_* semantics (set_* silently evicts any existing same-kind frame to preserve "at most one") and the rposition search add a layer where an ordering or eviction bug would be invisible until a specific nested-choice card misbehaves. The new serde back-compat + round-trip tests are good and necessary, but they pin shape, not the drain interleaving.

Recommendation — split

1. Land now (its own PR): the ResolutionFrame::Iterate family + the Vote/ChooseOneOf port + the Expropriate end-to-end test and the resolution_stack_iterate integration test. That is objective, localized value with discriminating coverage.
2. Defer: the five-field relocation onto stack variants, the Chain variant, Accumulator::TrackedSet, and the reserved IterItem variants until the slice that actually consumes them — so the accessor churn lands together with the code that needs LIFO nesting, not ahead of it.

No correctness defect found in the code as written; this is a value/blast-radius hold, not a BLOCK. Also fold in Gemini's two nits when re-spinning: add // CR 608.2c to ResolutionFrame::Chain and use VecDeque::from(...) in choose_one_of.rs instead of .into_iter().collect().

[dripsmvcp]

/gemini review

[gemini-code-assist]

Code Review

This pull request introduces the first slice of the unified resolution_stack architecture, migrating the flat, hand-stashed pending_choose_one_of and pending_vote_ballot_iteration fields into a LIFO stack of ResolutionFrame::Iterate frames. It also encapsulates other pending continuation fields (such as counter additions, counter moves, and batch deliveries) into stack-based accessors on GameState to prepare for future migration slices while preserving existing behavior. The review feedback suggests several idiomatic Rust improvements, such as using VecDeque::from for cleaner collection conversions and simplifying queue-draining loops by directly leveraging the return value of VecDeque::pop_front.

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.

[gemini-code-assist]

Idiomatic Rust

Using .into_iter().collect() to convert a Vec to a VecDeque is less direct and idiomatic than using VecDeque::from. Simplifying this improves readability and aligns with standard library idioms.

    let mut players = VecDeque::from(choosing_players(state, ability, chooser));

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. ^(link)

[gemini-code-assist]

Idiomatic Rust

Using .iter().copied().collect() to convert a cloned Vec to a VecDeque is less direct than using VecDeque::from. Simplifying this improves readability and aligns with standard library idioms.

Suggested change

	let mut remaining_voters: VecDeque<PlayerId> = choice_ballots.iter().copied().collect();
	let mut remaining_voters = VecDeque::from(choice_ballots.clone());

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. ^(link)

[gemini-code-assist]

Idiomatic Rust

Using front().copied() followed by pop_front() is redundant and non-idiomatic. You can directly use pop_front() which returns the popped element as an Option.

Suggested change

	while let Some(voter) = remaining_voters.front().copied() {
	remaining_voters.pop_front();
	while let Some(voter) = remaining_voters.pop_front() {

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. ^(link)

[gemini-code-assist]

Idiomatic Rust

Using front().copied() followed by pop_front() is redundant and non-idiomatic. You can directly use pop_front() which returns the popped element as an Option.

Suggested change

	while let Some(voter) = remaining_voters.front().copied() {
	remaining_voters.pop_front();
	while let Some(voter) = remaining_voters.pop_front() {

References

1. Idiomatic Rust — uses the type system, ownership model, and standard library idioms to their fullest. ^(link)

[dripsmvcp]

Addressed the review feedback:

• Added the CR 608.2c annotation to ResolutionFrame::Chain.
• Switched Vec→VecDeque conversions to VecDeque::from(...) in choose_one_of and vote.
• Simplified the per-ballot loops to while let Some(voter) = remaining_voters.pop_front().

Full engine suite green (13,392 passing, 0 failed), clippy clean.