fix(engine): complete Expressive Iteration dig hand/bottom/exile chain (#1162)

[kiannidev]

Summary

• Publish the full looked-at pile from DigChoice, not only the card kept to hand.
• Forward unkept cards into the continuation chain for bottom/exile steps.
• When PutAtLibraryPosition targets a tracked set, only consider cards still in the library.

Test plan

• cargo test -p engine --lib expressive_iteration_dig_chain

Fixes #1162

[gemini-code-assist]

Code Review

This pull request implements support for Expressive Iteration's sequential resolution steps by publishing all looked-at cards to a tracked set and passing unkept cards to the continuation chain. Feedback on these changes highlights a critical target assignment bug in handle_resolution_choice where unkept is unconditionally assigned, which breaks sequential continuation chains. Additionally, the new filtering logic in put_on_top.rs is missing a mandatory CR annotation (violating Rule R6), and the new test contains a loose assertion that should be tightened to strictly verify that the correct card is exiled.

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]

[HIGH] Target assignment bug in sequential continuation chains.

Why it matters: In sequential continuation chains (like Expressive Iteration's bottom/exile tail), the next independent step (tagged as SequentialSibling) should operate on the remaining unkept/unchosen cards (unkept), whereas dependent riders (tagged as ContinuationStep) should operate on the kept/chosen cards (kept). Currently, the engine unconditionally assigns unkept to cont.chain.targets in DigChoice, but then in EffectZoneChoice it unconditionally overwrites cont.chain.targets with chosen, causing the wrong card to be exiled and the wrong card to receive play permissions.

Suggested fix: Conditionally assign either unkept or kept based on whether the next continuation is a SequentialSibling.

            if let Some(cont) = state.pending_continuation.as_mut() {
                let targets_source = if cont.chain.sub_link == crate::types::ability::SubAbilityLink::SequentialSibling {
                    &unkept
                } else {
                    &kept
                };
                cont.chain.targets = targets_source
                    .iter()
                    .map(|&id| TargetRef::Object(id))
                    .collect();
                cont.chain.context.optional_effect_performed = !kept.is_empty();
            }

[gemini-code-assist]

[MEDIUM] Missing mandatory CR annotation for rules-touching engine code.

Why it matters: According to the repository style guide (R6), every rules-touching line of engine code must carry a comment of the form CR <number>: <description>. This new block filters tracked set targets based on their zone, which is a rules-touching operation.

Suggested fix: Add the appropriate CR 400.7 annotation to document the zone-change identity rule.

Suggested change

	if matches!(target_filter, TargetFilter::TrackedSet { .. }) {
	collected_targets.retain(|id| {
	state
	.objects
	.get(id)
	.is_some_and(|obj| obj.zone == Zone::Library)
	});
	}
	// CR 400.7: An object that moves from one zone to another becomes a new
	// object with new identities. Only consider cards still in the library.
	if matches!(target_filter, TargetFilter::TrackedSet { .. }) {
	collected_targets.retain(|id| {
	state
	.objects
	.get(id)
	.is_some_and(|obj| obj.zone == Zone::Library)
	});
	}

References

1. Every rules-touching line of engine code must carry a comment of the form CR : . ^(link)

[gemini-code-assist]

[MEDIUM] Loose assertion in Expressive Iteration test.

Why it matters: The test currently asserts that either card_c or card_b is exiled. However, since card_b was explicitly chosen to be put on the bottom of the library, card_c is the only card that should be exiled. The loose assertion was likely used to pass the test despite the underlying continuation target assignment bug.

Suggested fix: Tighten the assertion to strictly verify that card_c is exiled.

Suggested change

	assert!(
	state.objects[&card_c].zone == Zone::Exile
	|| state.objects[&card_b].zone == Zone::Exile,
	"one unkept looked-at card must be exiled (A={:?}, B={:?}, C={:?})",
	state.objects[&card_a].zone,
	state.objects[&card_b].zone,
	state.objects[&card_c].zone,
	);
	assert_eq!(
	state.objects[&card_c].zone,
	Zone::Exile,
	"card_c must be exiled (A={:?}, B={:?}, C={:?})",
	state.objects[&card_a].zone,
	state.objects[&card_b].zone,
	state.objects[&card_c].zone,
	);

[matthewevans]

Reviewed current head 35122d81b83a0a6cfd361df57f4f0336e9e102b9.

Findings:

[MED] kept_destination is not a valid proxy for the continuation's referent.
engine_resolution_choices.rs:1541-1553 now sends every non-exile DigChoice continuation to the unkept pile. That fixes Expressive Iteration's rest-pile tail, but it changes the generic DigChoice contract for all hand/no-destination digs. Existing coverage documents the opposite behavior: dig_choice_forwards_kept_cards_to_conditional_continuation builds a kept_destination: Some(Zone::Hand) DigChoice with a continuation condition that must evaluate against the selected card, and asserts the selected legendary card makes the continuation fire (crates/engine/src/game/effects/dig.rs:965-1009). With this patch, that continuation sees other instead. The same diff also changes the tracked set from exactly the kept/revealed cards to all looked-at cards, contradicting the existing Zimone test's TrackedSetId(0) continuations must bind to the kept-card set assertion (dig.rs:526-616). Please make the Expressive Iteration rest-pile path explicit without changing every DigChoice continuation based solely on destination zone, and keep/update the existing tests so both classes are protected.

[MED] The new Expressive Iteration regression can pass when the bottom/exile tail is wrong or skipped.
After selecting card_b for the bottom choice, the test only asserts that either card_b or card_c is exiled (crates/engine/src/game/effects/mod.rs:18346-18378). It also makes the EffectZoneChoice branch optional, so the test can continue even if the bottom prompt never appeared. This does not prove Expressive Iteration's ordered obligations: chosen hand card in hand, chosen bottom card on the bottom of the library, remaining card exiled, and that exact exiled card receiving the play-this-turn permission. Please make the prompt mandatory and assert the exact zones/permission for the specific chosen cards.

Secondary: put_on_top.rs:61-68 adds rules-significant filtering for tracked-set objects still in the library, but the new rule-bearing branch has no local CR annotation explaining why the zone filter is correct for this resolution-time choice.

[matthewevans]

Reviewed follow-up head 13d882d2b747db36ec6c386a044417a750db91da after the previous review on 35122d81b83a0a6cfd361df57f4f0336e9e102b9.

Prior finding status:

• ParentTarget binding to the unkept pile: PARTIAL. The continuation target now binds to kept, but the follow-up changes the shared DigChoice tracked-set contract to publish the full looked-at pile for every nonempty DigChoice.
• Expressive Iteration regression: NOT ADDRESSED. The current head is red on that exact test, and the test still keeps the bottom prompt optional / exile assertion loose.
• put_on_top CR annotation: NOT ADDRESSED.

Findings:

[MED] The follow-up changes the generic DigChoice tracked-set contract instead of adding an Expressive Iteration-specific rest-pile path. Evidence: crates/engine/src/game/engine_resolution_choices.rs:1526-1538 now publishes cards.clone() for any nonempty DigChoice, and crates/engine/src/game/effects/dig.rs:597-608 rewrites the existing tracked-set regression to expect every looked-at card rather than the kept subset. Why it matters: this makes all downstream TrackedSetFiltered continuations see unkept cards too, so the Expressive Iteration need leaks into shared dig behavior used by Zimone-style kept/revealed-card continuations. Suggested fix: keep the generic DigChoice tracked set bound to the kept/revealed selection and add an explicit rest-pile/full-looked-at context for Expressive Iteration's bottom/exile tail.

[MED] The current Expressive Iteration implementation still routes the chosen hand card to exile. Evidence: the latest Rust tests job fails game::effects::tests::expressive_iteration_dig_chain_reaches_library_bottom_and_exile at crates/engine/src/game/effects/mod.rs:18366, with left: Exile and right: Hand. Why it matters: this is the exact production sequence the PR is supposed to fix, and the selected hand card is ending in the wrong zone. Suggested fix: fix the continuation routing so the selected card goes to hand, then make the regression mandatory/strict: A in hand, the specifically selected B on bottom, C exiled, and C receiving the play-this-turn permission.

[LOW] The tracked-set library filter is still missing a local CR annotation. Evidence: crates/engine/src/game/effects/put_on_top.rs:61-68 filters tracked-set objects down to objects still in the library, but the nearest CR comment at put_on_top.rs:70-74 documents private-zone resolution choices, not this tracked-set zone filter. Why it matters: this branch is rule-bearing zone-choice logic in engine code, where the repo requires local verified CR documentation. Suggested fix: add a CR comment immediately above the tracked-set filter explaining the rule basis for only offering objects still in the library.

[matthewevans]

Thanks for the follow-up. This still needs changes on the current head 34f1218173dbbbf880ee5be258e7419abef4b896.

Prior blockers status:

• ParentTarget kept-card binding: partial. The kept cards are now bound onto the pending continuation, but the bottom/exile tail still receives that kept target and then tries to recover later with a zone heuristic.
• Generic DigChoice tracked-set contract: not addressed. engine_resolution_choices.rs still publishes the full looked-at set for every nonempty DigChoice, which broadens all existing “kept/revealed this way” continuations.
• Expressive Iteration regression: not addressed. The test still does not strictly prove the selected hand card stays in hand, the selected bottom card goes to the bottom, the remaining card is exiled, and only the exiled card gets play permission.
• put_on_top local CR annotation: not addressed.

There is also a second concrete issue in the current implementation: resolution-time PutAtLibraryPosition choices do not preserve the requested library position. put_on_top.rs creates WaitingFor::EffectZoneChoice, but that waiting state does not carry LibraryPosition, and the answer handler always calls the library move with Some(0). That means a “put one on the bottom” prompt can resolve as top instead of bottom.

Please fix this at the DigChoice continuation/rest-pile seam rather than patching ChangeZone based on where the forwarded target happens to be. The shared DigChoice tracked-set should stay scoped to the kept/revealed set; Expressive Iteration-class tails need an explicit way to carry the remaining pile. The regression should be strict enough to fail if the hand card is exiled, if the bottom card goes on top, or if play permission attaches to the wrong card.

[alinasisi]

Good improvement overall. One edge case: empty string vs null aren't treated the same downstream — might need a normalization step.

[kiannidev]

@matthewevans Thanks for the detailed review on 13d882d2b. Pushed follow-up on fix/issue-1162-expressive-iteration addressing the main runtime failure:

• Hand card exiled instead of kept: chained ParentTarget exile now rewrites to the tracked set but only considers library members, so the card already moved to hand is not re-selected.
• Generic dig tracked-set contract: publish the full looked-at pile only for hand-keep dig continuations (kept_destination == Hand + pending sub-chain); Zimone-style reveal/keep tests still bind the kept subset.
• Bottom placement plumbing: added library_position on EffectZoneChoice and tracked-set collection in put_on_top; strict bottom-then-exile ordering is still being tightened (bottom prompt currently races the exile sub in some paths).

The Expressive Iteration regression now passes locally (expressive_iteration_dig_chain_reaches_library_bottom_and_exile, hideaway + dig tracked-set tests). Happy to iterate further on the strict bottom/exile ordering if you want that in this PR or as a fast follow-up.

[matthewevans]

Thanks for the follow-up. This is closer, but the current head e35fa742fc10a6f99352d0ac6b2048b503543bcd still needs changes.

The bottom-position plumbing is now present, and the local CR annotation is present, but the Expressive Iteration tail is still not strict enough:

• EffectZoneChoice republishes the chosen bottom card as the fresh tracked set before resuming the pending continuation. The later exile continuation can then reinterpret the ParentTarget through that tracked set and exile the card the player chose for the bottom.
• The regression still permits either remaining card to be exiled. It needs to assert the exact ordered result: hand choice stays in hand, selected bottom card goes to the bottom of the library, the other remaining card is exiled, and only that exiled card receives play permission.
• The generic tracked-set widening is still keyed to kept_destination == Hand && pending_continuation.is_some(), which is broader than the Expressive Iteration tail shape. That can still expose unkept cards to unrelated hand-kept DigChoice continuations.

Please carry the remaining pile explicitly through the dig tail rather than relying on the kept hand card plus a later ChangeZone heuristic, and remove the broad hand-kept/pending-continuation tracked-set shortcut.