fix(engine): stop Jinnie Fay replacement from re-prompting (#4886) - #4938
Conversation
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
matthewevans
left a comment
There was a problem hiding this comment.
Thanks. I found one blocker in the current approach.
The inherited replacement-applied set is only seeded as transient game state and then consumed by the next token proposal. That handles the direct Jinnie Fay branch in the test, but it does not cover the full replacement-continuation class: nested ChooseOneOf branches are not detected by ability_tree_creates_tokens, and a branch that emits a second substitute token would lose the inherited applied set after the first token event. That lets the same replacement become eligible again inside its own substitute effect, which is exactly the loop this is trying to prevent.
Please bind the inherited applied set to the replacement continuation / choice context and apply it to every token proposal emitted by that continuation, including nested ChooseOneOf branches, then add coverage for a nested or multi-token branch so this cannot regress.
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The inherited replacement-applied set can still be dropped across an intervening nested replacement choice inside the paused branch chain. Evidence: branch token proposals read state.post_replacement_token_choice_applied at crates/engine/src/game/effects/token.rs:450, but continue_replacement_impl overwrites that field to None for any nested replacement whose own post-effect is not another token-choice shape at crates/engine/src/game/replacement.rs:6750. ReplacementChoice is one of the resolution-pausing states (crates/engine/src/game/effects/mod.rs:1812), and sub-abilities are stashed across such pauses (crates/engine/src/game/effects/mod.rs:7101), so a selected Jinnie branch that creates a token, hits another optional replacement choice, then resumes a later token sub-ability loses the original replacement id and can re-prompt the same Jinnie replacement. Why it matters: this is the same replacement-continuation loop class as the earlier blocker, just through an intervening ReplacementChoice instead of only nested ChooseOneOfBranch. Suggested fix: carry the inherited applied seed on the replacement continuation / pending resolution context and clear it only when the originating token-choice continuation fully drains, then add a regression with a branch token that pauses on another replacement choice before a later token sub-ability.
Parse changes introduced by this PR✓ No card-parse changes detected. |
|
Warning You have reached your daily quota limit. Please wait up to 24 hours and I will start processing your requests again! |
93b4a4f to
750853e
Compare
…eOneOf (phase-rs#4886) Address [HIGH] review finding on phase-rs#4938: the inherited replacement-applied seed could still be dropped across an intervening nested replacement choice inside the paused branch chain. Root cause: the seed lived as a single transient GameState field cleared at TWO sites that cannot distinguish an outer (originating) token-choice drain from a nested replacement's drain: - continue_replacement_impl's `_ => None` arm wiped it for every non-token-choice nested replacement (replacement.rs) - apply_pending_post_replacement_effect cleared it whenever waiting_for was not ChooseOneOfBranch (engine_replacement.rs) So a Jinnie branch that created a token, paused on another optional ReplacementChoice, then resumed a later token sub-ability lost the originating replacement id and re-prompted the same Jinnie replacement — the exact loop this is trying to prevent. Fix: make the seed OWNED by the originating token-choice ChooseOneOf continuation. Seed it only when a CreateToken event is replaced by a token-choice continuation; preserve it across all other replacement activity; clear it only when that originating ChooseOneOf fully drains back to priority (effects/choose_one_of.rs). Remove both clobbering sites. Document the ownership on the field. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14679 tests) pass. New regression pins the invariant: a pre-seeded applied set survives an intervening non-token-choice continuation drain.
matthewevans
left a comment
There was a problem hiding this comment.
[HIGH] The inherited applied seed can still be cleared by an intervening nested choice before the later token sub-ability drains. Evidence: the seed is global on GameState at crates/engine/src/types/game_state.rs:5967, and the originating replacement seeds it at crates/engine/src/game/replacement.rs:6760 when a token replacement parks a token-choice ChooseOneOf. But ChooseOneOf completion clears that global seed whenever waiting_for is back to priority at crates/engine/src/game/effects/choose_one_of.rs:161; then the ChooseBranch action handler drains any stashed continuation only afterward at crates/engine/src/game/engine_resolution_choices.rs:2388. A branch shaped like ChooseOneOf(non-token choice) -> sub_ability Token is production-reachable because resolve_ability_chain stashes the sub-ability while the nested choice is waiting (crates/engine/src/game/effects/mod.rs:7101). When the nested choice resolves, line 161 clears the Jinnie seed before line 2388 drains the token continuation, so the later token proposal loses the already-applied replacement id and can re-prompt the same replacement. Suggested fix: carry the seed on the originating pending choice/continuation frame, or otherwise clear only after that frame and its stashed continuation have drained; add a regression where a Jinnie-class branch pauses on a non-token ChooseOneOf before a later token sub-ability.
…eOneOf (phase-rs#4886) Address [HIGH] review finding on phase-rs#4938: the inherited replacement-applied seed could still be dropped across an intervening nested replacement choice inside the paused branch chain. Root cause: the seed lived as a single transient GameState field cleared at TWO sites that cannot distinguish an outer (originating) token-choice drain from a nested replacement's drain: - continue_replacement_impl's `_ => None` arm wiped it for every non-token-choice nested replacement (replacement.rs) - apply_pending_post_replacement_effect cleared it whenever waiting_for was not ChooseOneOfBranch (engine_replacement.rs) So a Jinnie branch that created a token, paused on another optional ReplacementChoice, then resumed a later token sub-ability lost the originating replacement id and re-prompted the same Jinnie replacement — the exact loop this is trying to prevent. Fix: make the seed OWNED by the originating token-choice ChooseOneOf continuation. Seed it only when a CreateToken event is replaced by a token-choice continuation; preserve it across all other replacement activity; clear it only when that originating ChooseOneOf fully drains back to priority (effects/choose_one_of.rs). Remove both clobbering sites. Document the ownership on the field. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14679 tests) pass. New regression pins the invariant: a pre-seeded applied set survives an intervening non-token-choice continuation drain.
…ompletion (phase-rs#4886) Address [HIGH] review finding phase-rs#3 on phase-rs#4938: the seed could still be cleared by an intervening nested choice before a later token sub-ability drained. Root cause: choose_one_of.rs cleared the global seed the moment its branch resolved back to priority. But a branch shaped `ChooseOneOf(non-token) -> sub_ability Token` stashes the token sub-ability into pending_continuation (effects/mod.rs), which the ChooseBranch handler drains only afterward (engine_resolution_choices.rs::drain_pending_continuation). Clearing at ChooseOneOf completion wiped the seed in the gap between the nested choice resolving and the stashed token sub-ability proposing, so the later token lost the inherited replacement id and re-prompted the same Jinnie replacement — the loop, reached via a non-token ChooseOneOf instead of a ReplacementChoice. Fix (per review's 'or otherwise clear only after that frame and its stashed continuation have drained'): remove the choose_one_of.rs clear entirely; clear the seed at the true full-drain point in drain_pending_continuation — Priority + no pending_continuation + no pending_repeat_iteration. By then the originating token-choice frame and every stashed sub-ability have completed, so no token proposal can still need the seed. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14828 tests) pass. New regression pins the exact shape: a Jinnie branch that parks on a non-token ChooseOneOf before a token sub-ability.
750853e to
8383173
Compare
matthewevans
left a comment
There was a problem hiding this comment.
[MED] The token-choice applied seed still clears before a paused repeat this process continuation can re-enter and emit later token proposals. Evidence: crates/engine/src/game/effects/mod.rs:673 gates the full-drain cleanup only on pending_continuation and pending_repeat_iteration, then clears post_replacement_token_choice_applied at crates/engine/src/game/effects/mod.rs:685 before draining pending_repeat_until at crates/engine/src/game/effects/mod.rs:686; that drain can immediately re-run the stashed ability through resolve_ability_chain at crates/engine/src/game/effects/mod.rs:721 and crates/engine/src/game/effects/mod.rs:744. Why it matters: a Jinnie-class token-choice branch that pauses inside a repeat-until process can lose the inherited replacement id before a later repeated token proposal, reopening the same self-replacement loop class. Suggested fix: treat pending_repeat_until as part of the originating continuation frame and clear the seed only after that repeat continuation has fully drained or stopped; add a regression with a token-choice branch that pauses inside a repeat-until loop before a later token proposal.
…eOneOf (phase-rs#4886) Address [HIGH] review finding on phase-rs#4938: the inherited replacement-applied seed could still be dropped across an intervening nested replacement choice inside the paused branch chain. Root cause: the seed lived as a single transient GameState field cleared at TWO sites that cannot distinguish an outer (originating) token-choice drain from a nested replacement's drain: - continue_replacement_impl's `_ => None` arm wiped it for every non-token-choice nested replacement (replacement.rs) - apply_pending_post_replacement_effect cleared it whenever waiting_for was not ChooseOneOfBranch (engine_replacement.rs) So a Jinnie branch that created a token, paused on another optional ReplacementChoice, then resumed a later token sub-ability lost the originating replacement id and re-prompted the same Jinnie replacement — the exact loop this is trying to prevent. Fix: make the seed OWNED by the originating token-choice ChooseOneOf continuation. Seed it only when a CreateToken event is replaced by a token-choice continuation; preserve it across all other replacement activity; clear it only when that originating ChooseOneOf fully drains back to priority (effects/choose_one_of.rs). Remove both clobbering sites. Document the ownership on the field. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14679 tests) pass. New regression pins the invariant: a pre-seeded applied set survives an intervening non-token-choice continuation drain.
…ompletion (phase-rs#4886) Address [HIGH] review finding phase-rs#3 on phase-rs#4938: the seed could still be cleared by an intervening nested choice before a later token sub-ability drained. Root cause: choose_one_of.rs cleared the global seed the moment its branch resolved back to priority. But a branch shaped `ChooseOneOf(non-token) -> sub_ability Token` stashes the token sub-ability into pending_continuation (effects/mod.rs), which the ChooseBranch handler drains only afterward (engine_resolution_choices.rs::drain_pending_continuation). Clearing at ChooseOneOf completion wiped the seed in the gap between the nested choice resolving and the stashed token sub-ability proposing, so the later token lost the inherited replacement id and re-prompted the same Jinnie replacement — the loop, reached via a non-token ChooseOneOf instead of a ReplacementChoice. Fix (per review's 'or otherwise clear only after that frame and its stashed continuation have drained'): remove the choose_one_of.rs clear entirely; clear the seed at the true full-drain point in drain_pending_continuation — Priority + no pending_continuation + no pending_repeat_iteration. By then the originating token-choice frame and every stashed sub-ability have completed, so no token proposal can still need the seed. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14828 tests) pass. New regression pins the exact shape: a Jinnie branch that parks on a non-token ChooseOneOf before a token sub-ability.
…-rs#4886) Address [MED] review finding phase-rs#4 on phase-rs#4938: the seed could still be cleared before a paused repeat-this-process continuation re-entered and emitted later token proposals. Root cause: drain_pending_continuation cleared post_replacement_token_choice_applied BEFORE calling drain_pending_repeat_until; that drain re-enters resolve_ability_chain (effects/mod.rs:721 / :744) and can propose further tokens, which then lost the inherited replacement id and re-prompted the same Jinnie replacement — the loop, reached via a repeat-until pause. Fix: treat pending_repeat_until as part of the originating continuation frame. Drain it first, then clear the seed only at true full-drain — Priority + no pending_continuation + no pending_repeat_iteration + no pending_repeat_until. The seed now outlives every drain in the function. cargo fmt, cargo clippy -p engine --all-targets -- -D warnings, and the full engine lib suite (14873 tests) pass. New regression pins the invariant: a pre-seeded applied set survives a pending_repeat_until drain that re-prompts the controller.
8383173 to
5876a94
Compare
matthewevans
left a comment
There was a problem hiding this comment.
[MED] This PR still carries unrelated parser-backlog doc changes. Evidence: docs/parser-misparse-backlog.md now changes the global totals at lines 5-7 and root-cause 31 at line 5221, but #4938 is a Jinnie Fay replacement-loop engine fix; those backlog edits remove the Rush of Dread / The Goose Mother entries that were handled separately in the parser backlog work. Why it matters: it contaminates the engine fix with unrelated ledger churn, which makes the PR harder to review and risks duplicate/conflicting doc accounting. Suggested fix: drop docs/parser-misparse-backlog.md from this PR and keep this diff scoped to the replacement-loop files.
I did check the current repeat-until seed-clearing change as well. The prior pending_repeat_until concern appears addressed by draining repeat-until before clearing post_replacement_token_choice_applied in crates/engine/src/game/effects/mod.rs lines 677-697, but the PR still has a queue-safety proof gap because the AI contributor template proof sections are missing. Please keep the behavioral proof on the PR current after removing the unrelated doc edit.
|
@matthewevans — thanks for confirming the repeat-until fix. On the What happened: this branch's merge-base ( I've merged current Files touched by this PR (post merge-base):
Ready for re-review on the current head. |
matthewevans
left a comment
There was a problem hiding this comment.
Thanks for the correction on the parser-backlog doc finding. I rechecked the merge-base diff for this PR and agree that docs/parser-misparse-backlog.md is not in origin/main...HEAD; that prior objection should not block this PR.
I found two remaining lifecycle blockers in the current replacement-frame approach:
[MED] Token-choice seeding misses a ChooseOneOf whose token is in the choice's own tail. Evidence: crates/engine/src/game/replacement.rs:300 only classifies ChooseOneOf by scanning its branches, while crates/engine/src/game/replacement.rs:6840 only seeds through that predicate; crates/engine/src/game/effects/mod.rs:7208 can stash a paused effect's sub_ability, and crates/engine/src/game/engine_resolution_choices.rs:2550 drains that tail after ChooseBranch. Why it matters: a CreateToken -> ChooseOneOf(non-token branches).sub_ability(Token) replacement will not seed the applied set, so the tail token can re-prompt/reapply the originating token replacement. Suggested fix: classify the whole root AbilityDefinition for token creation, not only the branches, or carry the applied set on the owned continuation frame itself; add a test for this tail-token shape.
[MED] The new global seed is not cleared when replacement/choice teardown abandons the normal drain path. Evidence: crates/engine/src/types/game_state.rs:6032 adds post_replacement_token_choice_applied, normal clearing only happens in crates/engine/src/game/effects/mod.rs:692, and the elimination teardown clears adjacent replacement continuation state but not this field at crates/engine/src/game/elimination.rs:487. Why it matters: if a live token-choice replacement frame is abandoned before normal full-drain, later token proposals inherit a stale applied set via crates/engine/src/game/effects/token.rs:450, suppressing replacements that should apply. Suggested fix: clear the seed anywhere the owning replacement/choice continuation is discarded, or move the seed into the owned pending continuation frame so teardown cannot leave a global latch behind.
…-rs#4886) Addresses review phase-rs#6 on PR phase-rs#4938: - is_token_replacement_choice only scanned a ChooseOneOf's branches for token creation, missing a token created by a sub_ability tail chained after the whole choice resolves. Reuse the existing recursive ability_tree_creates_tokens classifier (which already walks sub_ability/else_ability) instead of a narrower branches-only check. - The elimination teardown path abandoned a live post-replacement continuation without clearing post_replacement_token_choice_applied, the one field missing from its established sibling-clearing bundle. Extract that bundle into a single abandon_post_replacement_continuation helper so a future field can't be missed the same way again. Both fixes are pinned by discriminating regression tests (verified to fail without their corresponding fix, pass with it).
|
Re-reviewed the latest head. The two most recent requested-change items look addressed in the code: the token-choice classifier now walks the full ability tree including tail I am holding approval until the current CI evidence is complete for this head. At review time the current card-data/parse-diff artifact was not available yet and Rust/card-data checks were still pending. |
|
I updated the branch to refresh the stale merge-base evidence: the previous parse-diff was comparing against an old base and included unrelated parser/card-data changes despite this PR's current diff being engine-only. The substantive head reviewed clean for the latest lifecycle issues, but I am holding approval on the new merge head until the refreshed CI and parse-diff/card-data evidence complete. |
|
Saw the new merge-only head |
matthewevans
left a comment
There was a problem hiding this comment.
Maintainer sign-off after re-review: current head is merge-only from the previously reviewed clean implementation, parse-diff reports no card-parse changes, and the current required checks are green.
Summary
CreateToken -> ChooseOneOf(Token, Token)replacements as full substitutions so the original token event does not also resolveappliedset into the chosen substitute token event so the same replacement cannot immediately re-prompt on its own branchFiles changed
crates/engine/src/game/effects/choose_one_of.rs— stopChooseOneOfcompletion from clearing the inherited token-choice applied seedcrates/engine/src/game/effects/mod.rs— clear the applied seed only at true full-drain (no pending continuation, repeat iteration, or repeat-until), after all of those have drainedcrates/engine/src/game/effects/token.rs— read the inherited applied seed on every token proposal emitted by the replacement continuationcrates/engine/src/game/engine_replacement.rs— replacement-continuation plumbing for the token-choice substitution pathcrates/engine/src/game/replacement.rs— seed the applied set on the originating token-choice continuation (Jinnie Fay-class)crates/engine/src/types/game_state.rs—post_replacement_token_choice_appliedstate field + ownership/lifetime documentationTrack
Developer
LLM
Model: GLM-5.2
Thinking: high
Verification
cargo test -p engine create_token_choice_replacement_does_not_reprompt_or_create_original_tokens -- --nocapturecargo test -p engine create_token_replacement_accepted_applies_full_spec -- --nocapturecargo test -p engine replacement_instead_accepts_untap_and_token_choice_replacements -- --nocapturecargo clippy --all-targets -- -D warnings