ship/canonicalize delayed trigger lifecycle - #6933
Merged
Merged
Conversation
Contributor
|
@matthewevans can we hold this one so #6886 goes in first - better to refactor after big changes go in. |
4 tasks
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
…lper Two seams the phase-rs#6933 fold analysis flagged that the merge resolution left open. Both are additive here; nothing in the merge is rewritten. 1. CR 732.2a bound guard, second ingress. phase-rs#6933 split the decode surface: the free `decode_persisted_resolution_state` became a delegator, and the two authorities are now `GameStateDecode::decode_persisted_resolution_state` and `GameStateDecode::decode`. They are genuinely separate — the former deserializes `ResolutionStateWire` itself and never calls the latter — and `impl Deserialize for GameState` routes through `decode` with `GameStateDecodeMode::DirectCurrentRaw`. With the guard on one site only, a persisted `LoopShortcut` offer whose wire bound is 0 was refused through the persisted path and revived through the bare-`GameState` path. A discriminating row lands with it (see loop_shortcut.rs): bare-`GameState` decode of `with_bound(0)` must `Err`, with a `with_bound(5)` control and a reach-guard that the UNMUTATED fixture decodes bare at all — `DirectCurrentRaw` skips the legacy migrations, so without that guard an `Err` would not be attributable to the bound. REVERT-PROBE: delete only the new call in `decode` and every pre-existing arm still passes while that row flips to `Ok`. 2. `delayed_trigger_payload_matches` is dead. It is base code from the v1 provenance backfill that phase-rs#6933 replaced; upstream deleted the definition AND its only caller together, and this branch adds no caller of its own (`git diff 9b97019..ea1b0ac -- game_state.rs` contains no added reference). Kept, it is a private fn with zero callers under `-D warnings`. Post-removal grep for the name returns 0 hits repo-wide. Also documents, at the `bind_resolution_scope` caller in stack.rs, why the CR 603.4 settlement must stay at the caller and never move into the helper: `analysis/resource.rs` calls that helper on CLONED PROBE BOARDS at five sites, where running terminal delayed-trigger disposition would mutate lifecycle state for a board that is only being measured. Comment only; the code is unchanged. ON THE CR ANNOTATION AT THAT BLOCK, since it is easy to read as drift: the trigger-event-context block keeps `CR 608.2k`, not the `CR 603.7c` inherited from before this branch. CR 608.2k — "If an ability's effect refers to a specific untargeted object that has been previously referred to by that ability's cost or trigger condition, it still affects that object even if the object has changed characteristics" — is what the block implements: it stores `current_trigger_event` so `TriggeringSpellController`/`TriggeringSource` resolve at resolution time. CR 603.7c is scoped to delayed triggered abilities and zone changes and does not describe this code. `resolution_prompt.rs` already cross-references CR 608.2k for this helper. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
…ted payload Two independent changes to the same file, both consequences of phase-rs#6933. 1. The second-ingress row for the CR 732.2a bound guard added in the previous commit. Arms: bare-`GameState` decode of `with_bound(0)` must `Err` naming `max_iterations 0`; `with_bound(5)` must load; and a REACH-GUARD that the unmutated fixture decodes bare at all, because `DirectCurrentRaw` skips the legacy migrations and an `Err` from a fixture that cannot decode at all would say nothing about the bound. The pre-existing arms above it cannot stand in for this one: they reach `decode_persisted_resolution_state`, and a revert of the guard call in `decode` ALONE leaves every one of them green. The orphaned `PersistedGameState` import goes with it — the branch's loader uses the fully-qualified path, so the merge left the short import unused (`-D warnings`). 2. r28_c's TRUSTED arm hand-built `{"state": <bare GameState>}`. phase-rs#6933 made `resolution_state_version` a required discriminator and gave only the PersistedRaw ingress permission to stamp v1 onto a legacy payload; the TrustedEnvelope ingress deliberately stamps nothing, because a trusted snapshot is WRITTEN as a versioned envelope and must retain its declared compatibility mode. `GameState`'s derived `Serialize` emits no such field, so the hand-built value was a shape production never writes and the trusted path was right to refuse it. Measured before the fix: `Error("resolution state wire is missing a numeric resolution_state_version")`. Fixed at the BUILDER, not the assertion: the payload is now constructed through `ResolutionStateWire::from_game_state`, which is exactly what `TrustedGameStateEnvelope`'s own `Serialize` does, so the arm round-trips the shape production writes. No assertion was weakened. The decode `.expect` also becomes a labelled panic. The row drives four arms (hostile x trusted) and the original message carried no `{label}`, so the failure above could not say which arm broke — it had to be re-derived by hand. Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
…er controls
Arms 5 and 6 asserted against `{"Delayed":null}`, a wire shape phase-rs#6933 removed.
The live `TriggerFiring` variants (`types/identifiers.rs`) are `"Ordinary"`,
`"LegacyDelayed"`, `{"ReceiptEligible":{token,instance,source_id}}` and
`"UnknownLegacy"`.
Those arms stayed GREEN across the removal, which is the point worth recording:
jq is untyped, so the controls fed themselves a shape the engine can no longer
emit and got it back unchanged. A control that passes on input its subject cannot
produce is not evidence about the subject. The arms were not wrong — they had
stopped being about the engine.
Adds a `ReceiptEligible` sub-arm alongside the unit-variant one. That variant
carries a payload, and a preservation rule written against the unit variants
alone could drop its `DelayedTriggerOrigin` and still satisfy every other arm —
so this asserts the VALUE survives, not just the discriminant.
Measured after the change: DEFINITION_SHAPES, CARRIER_PRESERVED (with
`receipt={"ReceiptEligible":{"token":3,"instance":4,"source_id":7}}`),
STALE_CARRIER_PRUNED, NON_GAMESTATE_SKIPPED and STAGE_BESIDE_DEST all true;
12 fixtures, 0 arm failures.
Assisted-by: ClaudeCode:claude-opus-5
lgray
added a commit
to lgray/phase
that referenced
this pull request
Aug 3, 2026
…fold The census pins each producer by `file:line`, and folding upstream phase-rs#6933 grew engine.rs around one of them: `game/engine.rs:10640 => :11427`, +787, while the file's whole-file delta over the same range is +1134 — so 787 lands above this producer and 347 below, which is what a file growing AROUND a mint looks like rather than one gaining a mint. Identity re-established at the new coordinate, not assumed. The line at :11427 is byte-identical by sha256 to `ea1b0ac19:game/engine.rs:10640`, and it is still inside `begin_pending_trigger_target_selection` (fn opens at :11278) — the producer this row names. The old coordinate now holds copy-target-slot code that mints nothing. The other four entries did not move at all, which is the same set-preservation evidence the earlier rebases in this comment relied on: a census that had gained or lost a producer could not leave four entries byte-identical AND in place. Nothing is weakened. The total stays 37 and the partition stays 5/7/25; only the coordinate of an unchanged producer is updated. This is the failure mode the row is DESIGNED to have — it is line-pinned precisely so that a moved or added mint cannot pass silently — so the red was the instrument working, not a defect in it. Surfaced by `cargo test -p phase-engine --lib` at 596bdfa; every other gate stage at that tip was green (fmt, parser gate, clippy -D warnings, phase-ai, integration, fixture corpus). Assisted-by: ClaudeCode:claude-opus-5
JacobWoodson
added a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 3, 2026
…provenance type (phase-rs#6884) The lifecycle-port merge (phase-rs#6933) canonicalized DelayedTrigger.provenance from Option<..> to the DelayedInstallIdentity enum. The maintainer's port updated every production construction but not the building-block regression test added by this PR, so `provenance: None` in `multi_fire_combat_damage_consumes_raw_aggregate_not_synthetic` failed to compile (E0308), which cascaded to red Rust lint + both test shards. Set it to `DelayedInstallIdentity::LegacyDelayed` (a normal, non-command delayed trigger — the same value the production install path uses in effects/delayed_trigger.rs). Test-only change. Verified against the ported head: engine tests compile clean; the PR's added tests pass at runtime under the new canonical lifecycle (no behavioral reconciliation needed) — full lib suite 18323 passed, combat/trigger/delayed integration sweep 800 passed; cargo fmt + engine clippy (-D warnings) clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JacobWoodson
added a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 3, 2026
…ase-rs#6884) Follow-up to the provenance test fix: the comment I added cited CR 603.7c on a test-only DelayedInstallIdentity::LegacyDelayed assignment. 603.7c governs a delayed ability's object reference across characteristic/zone changes, not this internal lifecycle/test provenance representation. Removed the citation (engine test scaffolding is not a rule implementation); kept the phase-rs#6933 context note. Comment-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
JacobWoodson
added a commit
to JacobWoodson/phase
that referenced
this pull request
Aug 3, 2026
…t" trigger (CreateDelayedTrigger + WheneverEvent) with tracked-set/self source binding mechanic (phase-rs#6884) * Partial: Add Delayed/created "deals combat damage to a player/opponent" trigger (CreateDelayedTrigger + WheneverEvent) with tracked-set/self source binding mechanic * fix(engine): address delayed combat-damage trigger review blockers (phase-rs#6884) Resolve the two runtime over-fire paths and test-evidence gaps raised in review of the delayed "deals combat damage" WheneverEvent mechanic. Blocker 1 — double-fire (triggers.rs): collect_matching_delayed_triggers recorded the synthetic per-source DamageDealt event as the consumed identity while computing its occurrence from the raw aggregate CombatDamageDealtToPlayer at event_index. Because filter_consumed_trigger_events_from compares both event equality and occurrence, the aggregate never matched the consumed set and survived into a later priority scan, re-firing the trigger. Record events[event_index] (the raw aggregate) as the consumed identity and keep the synthetic event as per-firing trigger context only. New building-block regression multi_fire_combat_damage_consumes_raw_aggregate_not_synthetic fails on the old code and passes on the fix. Blocker 2 — empty ParentTarget over-fire (delayed_trigger.rs): bind_contextual_filter_to_condition rewrites all three WheneverEvent filter slots, so an "up to N target" parent that chose zero could turn a bare ParentTarget in valid_card/valid_target (not just valid_source) into TargetFilter::Any and install an over-firing trigger. The install guard now inspects all three slots; added sibling fixtures for valid_card and valid_target. CodeRabbit maintainability + test evidence: - Replace the in_delayed_trigger bool with a typed TriggerConditionScope enum (Printed/Delayed) per the codebase's typed-enum convention. - Compute the delayed-trigger one_shot boolean once and reuse it; gate the TriggeringSource origin-stamp to one-shot triggers (a multi-fire WheneverEvent re-resolves TriggeringSource per firing, so freezing the creation-event zone would skip a later firing's zone move). - Assert the WheneverEventExpiry fields in the three spell-temporal parser tests instead of discarding them. - Strengthen negative tests with positive reach guards: human_torch now pins the coverage-honest Unknown parse shape (the possessive subject is not a recognized anaphor) and love_on_the_battlefield's three-attacker test asserts the defending player's life dropped (attack actually resolved). Verification: cargo fmt; cargo clippy -p phase-engine --all-targets (clean); cargo test -p phase-engine --lib (18272 passed); combat/damage/trigger/delayed integration sweep (798 passed). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(engine): scope-evidence class guards for delayed-trigger duration relocation (phase-rs#6884) Reconcile the parse blast radius the maintainer flagged (14 cards / 4 signatures) with permanent class-level regression coverage rather than one-off card tests. Signatures 1-2 (12 cards, "until your next turn, whenever ..." delayed triggers): the CreateDelayedTrigger duration field change is the intended Gap-C fix. "until your next turn" relocates from the (previously misattached) creator-ability duration to the WheneverEvent expiry (UntilControllersNextTurn). This is a correctness win: before, these triggers had no expiry, defaulted to EndOfTurn, and were purged at the creating turn's cleanup, so they never fired on opponents' turns (CR 603.7b). Sig 1 (plain inner effect) leaves the creator ability with no residual duration; Sig 2 (inner "... until end of turn" buff, e.g. Jace/Tamiyo) surfaces the residual UntilEndOfTurn on the creator ability. until_next_turn_delayed_trigger_relocates_duration_to_expiry_across_class pins both. None of these 12 cards is claimed as newly-supported; they remain coverage-red multi-ability cards (strict failure retained). Signatures 3-4 (Alluring Suitor + Love on the Battlefield): the "attack with exactly two creatures" recognition (AttackersDeclaredCount EQ 2 + you target) is a class fix. Love has full runtime coverage; added parse_shape_alluring_suitor_exactly_two_attack_constraint for the sibling so the class cannot regress independently. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix(PR-6884): remove accidental scratch artifact * style(engine): fix clippy doc_lazy_continuation in scope-evidence test (phase-rs#6884) The scope-evidence class-guard test's doc comment had a bulleted list immediately followed by a paragraph; add the blank /// separator so clippy::doc_lazy_continuation (workspace -D warnings) is satisfied. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(engine)+docs: address second-round review on delayed-trigger PR (phase-rs#6884) [MED] The "until your next turn" duration test only proved persistence (delayed_triggers.len) and never exercised firing. Added until_next_turn_parent_target_rider_fires_on_intervening_turn_then_expires: installs a ParentTarget rider bound to a real chosen creature (Kang's "any of those creatures"), drives that creature through unblocked combat against the controller on the intervening (opponent's) turn via the production pipeline, asserts the controller drew and took the combat damage, then crosses into the controller's next turn and asserts the rider was purged at untap. A revert of the UntilControllersNextTurn expiry purges the rider at the creating turn's cleanup so it never fires (draw assertion fails); a revert of the untap purge leaks it (expiry assertion fails). This firing path also exercises the aggregate->per-source expansion + consumed-identity fix (blocker 1) at runtime. The existing survives-then-purges persistence test is retained. [LOW] Removed the false-provenance CR annotations (CR 603.7c / CR 201.5) from the parser-scope TriggerConditionScope enum and its ParseContext field. CR 603.7c governs a delayed ability's object reference across zone/characteristic changes and CR 201.5 governs name-based self-reference — neither describes parser scope or pronoun classification. Per the annotation convention this is parser scaffolding, not a rule implementation, so it carries no CR citation. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * test(engine): drive Kang firing test through the parsed chapter, not a synthetic def (phase-rs#6884) [HIGH] The intervening-turn firing test constructed a fresh CreateDelayedTrigger AbilityDefinition and resolved that, so it could stay green even if the parsed chain lost its UntilControllersNextTurn expiry, failed to carry the parent target through the preceding tap/goad clauses, or attached the delayed sub-ability at the wrong point. Replace it with kang_parsed_chapter_rider_fires_on_intervening_turn_ then_expires, which resolves Kang's ACTUAL parsed chapter via parse_effect_chain(KANG_CHAPTER) + build_resolved_from_def, supplying a real chosen creature as the tap target. This exercises the full production seam the PR changes: parse -> SetTapState/Goad clauses -> parent-target propagation of the chosen creature into the rider's ParentTarget source (asserted: valid_source == SpecificObject{chosen}, tapped == true) -> the delayed-trigger resolver's expiry stamping. It then drives that goaded creature through unblocked combat against the controller on the intervening turn (asserts the controller drew and took combat damage) and crosses into the controller's next turn (asserts the rider was purged). The synthetic install helper is removed. Second signature (Jace/Tamiyo class): its delayed-trigger install/expiry/purge seam is identical (pinned by the parse-shape class-guard test). A runtime firing drive was probed and intentionally NOT added: its inner "it gets -X/-0 until end of turn" parses "it" to Pump{target: SelfRef} (the source), a pre-existing inner-effect misparse on that unclaimed card that this PR neither introduces nor touches — so a Sig-2 firing assertion would test pre-existing behavior, not the Gap-C expiry seam. Tracked as a separate follow-up. Verification: cargo fmt --all; cargo clippy --workspace --exclude phase-tauri --all-targets --features engine/proptest -- -D warnings (clean); Kang test file passes (4/4). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(parser): remove unsupported CR citations from delayed-anaphora comments (phase-rs#6884) [MED] Follow-through on the earlier context.rs annotation fix: the same false-provenance CR citations remained on the sibling parser-scope/anaphora comments this PR added. Removed CR 603.7c / CR 201.5 (and the CR 109.4 + 608.2c lead-in) from the four PR-introduced comments: - oracle_effect/mod.rs: the `Delayed` scope set site. - oracle_trigger.rs: the anaphoric-subjects gate, the gendered-pronoun→SelfRef arm, and the plural-set→ParentTarget arm. CR 603.7c governs a delayed ability's object identity across characteristic/zone changes and CR 201.5 governs name-based self-reference; neither describes these Oracle-text parsing heuristics (mode selection, pronoun/anaphora recognition). Per the reviewer's instruction the citations are removed rather than swapped for a nearby delayed-trigger rule that does not demonstrably support the behavior; the explanatory prose is retained and each is labeled parser scaffolding. Scope confirmed via `git diff <merge-base>...HEAD`: these four are the only CR 603.7c/201.5 lines this PR added to the parser; all other such citations are pre-existing and left untouched. Comment-only change. Verified: cargo fmt --all clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * fix: retain delayed triggers without moving condition * test(engine): fix delayed-trigger test construction for phase-rs#6933 provenance type (phase-rs#6884) The lifecycle-port merge (phase-rs#6933) canonicalized DelayedTrigger.provenance from Option<..> to the DelayedInstallIdentity enum. The maintainer's port updated every production construction but not the building-block regression test added by this PR, so `provenance: None` in `multi_fire_combat_damage_consumes_raw_aggregate_not_synthetic` failed to compile (E0308), which cascaded to red Rust lint + both test shards. Set it to `DelayedInstallIdentity::LegacyDelayed` (a normal, non-command delayed trigger — the same value the production install path uses in effects/delayed_trigger.rs). Test-only change. Verified against the ported head: engine tests compile clean; the PR's added tests pass at runtime under the new canonical lifecycle (no behavioral reconciliation needed) — full lib suite 18323 passed, combat/trigger/delayed integration sweep 800 passed; cargo fmt + engine clippy (-D warnings) clean. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> * docs(engine): drop stray CR citation from provenance test fixture (phase-rs#6884) Follow-up to the provenance test fix: the comment I added cited CR 603.7c on a test-only DelayedInstallIdentity::LegacyDelayed assignment. 603.7c governs a delayed ability's object reference across characteristic/zone changes, not this internal lifecycle/test provenance representation. Removed the citation (engine test scaffolding is not a rule implementation); kept the phase-rs#6933 context note. Comment-only change. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Matt Evans <1388610+matthewevans@users.noreply.github.com> Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
This was referenced Aug 7, 2026
Merged
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit