ship/t125 stack grants - #5803
Merged
Merged
Conversation
matthewevans
commented
Jul 14, 2026
Member
- fix(engine): apply continuous effects to stack objects (CR 613.1)
- fix(engine): scope the identity-filter scan to zones the layer pass owns
The layer pass computed the zones to scan for a continuous effect's affected population as a pure function of the affected filter. TargetFilter::SpecificObject is an identity reference and carries no zone marker, so extract_in_zone() answered None, the zone set came back empty, and the scan fell back to [Zone::Battlefield]. A keyword granted to a spell ON THE STACK was therefore looked for in a population that could not contain it, and the grant was silently dropped -- Taigam, Ojutai Master's 'that spell gains rebound' parsed, fired, resolved, and did nothing (CR 702.88a: rebound functions while the spell is on the stack). CR 613.1 computes the characteristics of an OBJECT, not only of a permanent, so a SpecificObject leaf now contributes the zone the object is actually in. CR 400.7a follows for free: ObjectId is stable across the zone change, so a permanent spell keeps the grant as it becomes a permanent. Stack objects' keywords are now also reset to base each layers pass, mirroring the existing hand-zone reset. The grant policy explicitly relies on that reset/apply invariant; without it a summing keyword (CR 702.164b Toxic) would accumulate one instance per evaluation and a grant would outlive its transient continuous effect. This also closes the same pre-existing hole for StackSpell-filtered statics, which already scanned Zone::Stack with no reset.
Following a SpecificObject filter into ANY zone made the layer pass a second
writer of keyword state that off_zone_characteristics already owns on demand
(keywords::object_has_effective_keyword_kind routes every non-battlefield object
to it). Measured consequence: granted-Suspend-in-exile stopped ticking its time
counter, and the meld entry replacement stopped being consulted -- 5 tests that
pass on origin/main.
The layer pass may only materialize a characteristic into a zone where it also
RESETS it at the top of the pass, or the write is a leak nothing reclaims. That
set is exactly {Battlefield, Hand, Stack}; library/graveyard/exile belong to
off_zone_characteristics. layer_pass_materializes_keywords() names that invariant
in one place so the reset set and the scan set cannot drift apart.
The stack-grant fix is unchanged: Taigam's rebound and Waystone's mobilize still
land on the stack object, and Ogre Battledriver's battlefield grant is untouched.
matthewevans
enabled auto-merge
July 14, 2026 12:20
Contributor
There was a problem hiding this comment.
Code Review
This pull request resolves a defect where continuous effects granting keywords to objects on the stack (such as Taigam, Ojutai Master granting rebound) were silently dropped because the layer pass fell back to scanning only the battlefield. The fix updates continuous_effect_scan_zones and collect_scan_zones to accept the game state, allowing SpecificObject identity filters to resolve to the object's actual zone (restricted to zones owned by the pass: Battlefield, Hand, and Stack). It also ensures stack-zone objects' keywords are reset to their base set each layers pass to prevent unbounded accumulation. Comprehensive unit and integration tests have been added to verify these changes and prevent regressions. There are no review comments, so no additional feedback is provided.
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.
Parse changes introduced by this PR✓ No card-parse changes detected. |
parthmishra
pushed a commit
to parthmishra/phase
that referenced
this pull request
Jul 14, 2026
…gn invariants (phase-rs#5816) The add-* skills lagged five invariants that have landed on main, and one of them (add-static-ability) stated the opposite of what the engine now does. - add-static-ability: continuous effects apply to OBJECTS (CR 613.1), not only battlefield permanents. New "Zone Boundary" section: the layer pass materializes keywords ONLY into the zones it also resets ({Battlefield, Hand, Stack} — layer_pass_materializes_keywords); library / graveyard / exile are owned by off_zone_characteristics and computed on demand. A grant into an unowned zone is a silent no-op that still reports as supported (the PR phase-rs#5803 defect class). - add-keyword: document the strict keyword router (PR phase-rs#5813 / task phase-rs#123). Router slots and routing classifiers must parse through parse_router_keyword_line / _list / _fragment (all-consuming); the permissive parse_granted_keyword_fragment / extract_granted_keyword_list are for EMBEDDED GRANT contexts only. A line with an unconsumed semantic tail must DECLINE, not commit. Gate G in scripts/check-parser-combinators.sh is the whole-file invariant. New router-registry checklist item (is_keyword_cost_line + ROUTER_KEYWORD_CASES set-equality test). - add-trigger: new "Purged Source" section (CR 608.2h). LKI look-back via subject_filter_matches_with_lki / source_controller_or_lki; attachments do NOT survive on the live object (CR 704.5m/n sever them — only LKISnapshot::attachments holds the pre-sever set) while attack/block history DOES (durable id-keyed ledgers on GameState). Names the non-vacuity vector: a ceased-to-exist token (CR 111.7), not a printed creature. - casting-stack-conditions + add-engine-effect: the three X carriers (GameObject::cost_x_paid = CR 107.3m cast-X, GameState::announced_source_x = CR 107.3a/d announce-X, ResolvedAbility::chosen_x = the published channel, CR 107.3i) and publish_announced_x as the single publish authority. Re-measuring X at resolution is rules-wrong (CR 107.3c), not merely late. - add-replacement-effect: cross-line "instead" lowers to a BRANCH (ClauseDisposition::ReplaceMeaning → else_ability), never two independent effects — CR 614.1a + CR 614.6, the phase-rs#44/phase-rs#79 double-execution class. - project-reference: card-export measurement hazards — export-cards rewrites the tracked parser input oracle-subtypes.json (non-idempotent), gen-card-data dirties known-tokens.toml, and the export is nondeterministic on ~20 faces (the noise floor of any whole-pool ledger). - card-test: gen-test-fixture.py --check verifies COVERAGE only (a key-set comparison), never the stored parse VALUES — a parser change can leave the integration fixture stale while the gate stays green. Also fixes stale anchors found while verifying the self-check blocks (all five now pass; two were red before this change): - add-static-ability / add-keyword cited crates/engine/src/parser/oracle_static.rs, which is a directory now (oracle_static/mod.rs, oracle_static/grammar.rs, oracle_static/keyword_grant.rs). - casting-stack-conditions cited pay_and_push and pay_ability_cost in casting.rs; they live in casting_costs.rs and costs.rs. - add-replacement-effect cited "MTG Rule 614.16" for as-enters choices; 614.16 is the token/counter-creation replacement rule. The correct citations are CR 614.1c + CR 614.12a. Every symbol grep-verified against the tree at 5f4fbcd; every CR number grep-verified against docs/MagicCompRules.txt. scripts/check-skill-doc.sh and Gate G both pass. Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
parthmishra
pushed a commit
to parthmishra/phase
that referenced
this pull request
Jul 14, 2026
* fix(filter): count the source's attachments from LKI once it leaves the battlefield (CR 608.2h) Whiplash, Vengeful Engineer — "Whenever Whiplash attacks, if he's equipped, each opponent loses X life and you gain X life, where X is the number of Equipment attached to him" — resolved X to 0 whenever its source left the battlefield with the trigger on the stack. PR phase-rs#5792 opened the GATE for this class: the `SourceMatchesFilter` intervening-if ("if he's equipped") now answers from `LKISnapshot::attachments`. But the EFFECT then asks a second, independent question at the quantity layer: X = ObjectCount { Typed[Subtype(Equipment)] + FilterProp::AttachedToSource } `AttachedToSource` enumerated the LIVE battlefield and asked each Equipment `attached_to == Some(source)`. SBA unattaches every Equipment the instant its host leaves the battlefield (CR 704.5n) and puts every Aura into its owner's graveyard (CR 704.5m), so the back-reference was already cleared on every candidate and the count could only ever answer 0. The gate opened onto an effect that did nothing. CR 608.2h governs and is explicit: an effect that requires information from a specific object — INCLUDING THE SOURCE OF THE ABILITY ITSELF — uses that object's current information while it is in the public zone it was expected to be in, and its LAST KNOWN INFORMATION once it is not. The referent's expected zone is the battlefield, so the attachment set is read live while it is there and from `state.lki_cache` once it is not. NOT KEYED ON THE CR 111.7 PURGE. Unlike the gate half, this half was broken for a merely-dead NONTOKEN source too: SBA unattaches on ANY battlefield exit, so a Whiplash in the graveyard — still present in `state.objects` — read 0 exactly like a purged token. Both legs are witnessed; the zone, not the object's continued existence, is the discriminating axis. `AttachedToSource` and `AttachedToRecipient` ask the same question and differ only in the referent, so both now route through one `attached_to_referent` authority rather than each re-deriving the lookup. The live path is byte-identical: a referent on the battlefield still reads the candidate's own back-reference. Witnesses (crates/engine/tests/integration/purged_source_attachment_count_lki.rs), all driving the real pipeline (oracle synthesis -> attach::attach_to -> declare_attackers -> move_to_zone -> SBA -> stack resolution): * premise_living_equipped_whiplash_drains_for_its_equipment_count — GREEN before and after. A living Whiplash with 2 Equipment (and a 3rd unattached on the battlefield) drains exactly 2, proving the probe works and the count filters. * unequipped_whiplash_never_triggers — GREEN before and after (CR 603.4). * purged_token_source_counts_its_attachments_from_lki — RED (0, want 2). * nontoken_dead_source_counts_its_attachments_from_lki — RED (0, want 2). * purged_token_source_counts_only_its_own_attachments_not_every_equipment — RED (0, want 1). 1 Equipment attached, 2 unattached decoys: kills both the fail-closed 0 and a fabricating "count every Equipment" fix, which would read 3. CR 608.2h / 704.5m / 704.5n / 301.5 / 303.4 / 111.7 all grep-verified against docs/MagicCompRules.txt. * fix(filter): answer the AttackedThisTurn family from the surviving ledger on the LKI path (CR 608.2h) Taigam, Ojutai Master — "Whenever you cast an instant or sorcery spell from your hand, if Taigam attacked this turn, that spell gains rebound" — silently granted nothing when a token copy of Taigam attacked and then died with his trigger on the stack. The spell resolved into the graveyard instead of being exiled. `FilterProp::AttackedThisTurn` and its `BlockedThisTurn` / `AttackedOrBlockedThisTurn` siblings sat in the explicit FAIL-CLOSED group of `zone_change_record_matches_property`. A purged token source is absent from `state.objects`, so `subject_filter_matches_with_lki` falls back to the LKI snapshot, which synthesizes a `ZoneChangeRecord` and lands in exactly that group. The CR 603.4 re-check at resolution read FALSE and the trigger was removed from the stack. WHY FAIL-CLOSED WAS WRONG. The old rationale cited CR 400.7 — "a permanent that changes zones becomes a new object with no memory of its previous existence, so the zone-change snapshot captures no attack history." That conflates two objects. CR 400.7 governs the object that ARRIVES in the new zone, and it stays true: nothing here gives the graveyard card an attack history of its own. But the object this ability asks about is the one that ATTACKED, and CR 608.2h names it exactly: "the effect uses the object's LAST KNOWN INFORMATION ... If an ability states that an object does something, it's the object as it exists — OR AS IT MOST RECENTLY EXISTED — that does it." And the game still holds that record. `creatures_attacked_this_turn` (and the per-defender `creature_attacked_defenders_this_turn`, and `creatures_blocked_this_turn`) are keyed by the battlefield ObjectId, written at declaration (combat.rs) and cleared ONLY at turn cleanup (turns.rs) — never on a zone change. The ledger already outlived the source; the filter simply refused to read it, even though the record carries the very id it is keyed by. The `WasDealtDamageThisTurn` arm immediately above already reads its turn-scoped ledger by `record.object_id` for precisely this reason. These three arms now mirror it, and the live arm. ASYMMETRY WITH THE ATTACHMENT HALF (previous commit, Whiplash). There the underlying FACT is destroyed on exit — SBA unattaches every Equipment the instant its host leaves (CR 704.5n) — so both a purged token and a merely-dead nontoken were broken. Here the fact SURVIVES in a live ledger, so a merely-dead nontoken is already answered correctly by the live path; only the leg where the subject cannot be SEEN at all (the CR 111.7 purged token) was broken. Both directions asserted. SEQUENCING. Taigam is the only observable carrier of this condition in the pool, and his rebound grant was itself inert until PR phase-rs#5803 taught the layer pass to reach STACK objects. Landing this before that would have been unwitnessable. This builds on the harness that PR established. Witnesses (crates/engine/tests/integration/purged_source_attacked_this_turn_lki.rs), driving the real pipeline (oracle synthesis -> declare_attackers -> cast -> move_to_zone -> SBA -> stack resolution). Observable: rebound EXILES the spell as it resolves (CR 702.88a) where it would otherwise hit the graveyard (CR 608.2n). * premise_living_taigam_who_attacked_grants_rebound — GREEN before and after. * living_taigam_who_did_not_attack_grants_nothing — GREEN before and after (CR 603.4). * purged_token_taigam_still_answers_attacked_this_turn_via_lki — RED (Graveyard, want Exile). * nontoken_dead_taigam_was_already_correct_via_the_live_path — GREEN before and after. Pins the asymmetry above rather than assuming it. * purged_token_taigam_that_did_not_attack_grants_nothing — GREEN. No fabrication. * purged_token_attack_lookback_is_keyed_by_the_source_not_the_board — RED (false, want true). Interrogates the seam with Taigam's OWN parsed filter, with a DECOY creature attacking in the arm where Taigam does not, so the ledger is non-empty in both arms. A fix reading `!ledger.is_empty()` instead of keying on `record.object_id` would fabricate rebound here. CR 608.2h / 508.1a / 508.1b / 508.6 / 509.1a / 400.7 / 603.4 / 702.88a / 608.2n / 603.7a / 111.7 / 704.5d all grep-verified against docs/MagicCompRules.txt. --------- Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
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
1 participant
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.