test(engine): pin Underworld Breach's grant to its own battlefield presence (#1033)#5350
Merged
matthewevans merged 5 commits intoJul 8, 2026
Conversation
…esence (phase-rs#1033) Investigated phase-rs#1033's second claim ("Breach recasts itself from the graveyard via its own granted escape"): already correct per CR 604.2 — a static ability's continuous effect only exists while its source remains on the battlefield. Verified locally: with Breach placed directly in the graveyard (simulating post-sacrifice), neither Breach itself nor another graveyard card shows escape availability derived from its grant. Adds permanent regression coverage alongside the existing granted_escape_requires_exile_cost_payment test, which covers the first (and only reproducing) half of the original report. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Contributor
|
Note Gemini is unable to generate a review for this pull request due to the file types involved not being currently supported. |
matthewevans
approved these changes
Jul 8, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Reviewed test-only Underworld Breach coverage after maintainer test hardening. Scryfall confirms the Oracle text and CR 604.2 was verified locally. The test now makes the granted escape cost payable if Breach incorrectly grants escape from the graveyard, so the negative assertions are load-bearing for both Breach itself and another graveyard card. CI is pending on the fixup commit; auto-merge will wait for required checks.
matthewevans
added
test
rykerwilliams
added a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 8, 2026
Audited all four "already fixed" GitHub comments posted tonight, found real gaps where claims rested on a sub-agent's deleted scratch test rather than executed local verification, and fixed each one with a real permanent test (phase-rs#5348, phase-rs#5350, phase-rs#5351) plus updated comments. Logged as standing lesson 11 so this doesn't need re-litigating. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
Parse changes introduced by this PR · 656 card(s), 462 signature(s) (baseline: main
|
matthewevans
added
bug
matthewevans
approved these changes
Jul 8, 2026
matthewevans
left a comment
matthewevans
left a comment
Member
There was a problem hiding this comment.
Re-approved current head after maintainer fixups. The CI failure was from the off-zone static gate suppressing command-zone emblem statics; the current head preserves CR 114.4 emblems while keeping ordinary off-zone grants gated. Auto-merge can wait for the remaining checks.
rykerwilliams
added a commit
to rykerwilliams/phase
that referenced
this pull request
Jul 8, 2026
…lesson Point 2 of phase-rs#1033 was wrongly concluded "not a bug" from source-tracing alone, then correctly re-flagged as a real bug once actually tested. Fixed by the maintainer's phase-rs#5350 plus a non-overlapping follow-up (phase-rs#5381) covering 5 sibling functions their narrower fix didn't reach. Logs two new standing lessons: confirmatory tests can refute instead of confirm (treat that as the system working), and always defer to a maintainer's own commits on a PR rather than pushing a competing one. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
glorydavid03023
pushed a commit
to glorydavid03023/phase
that referenced
this pull request
Jul 8, 2026
…up to phase-rs#5350) (phase-rs#5381) * fix(engine): migrate remaining zone-of-function statics gathers (phase-rs#1033 follow-up) phase-rs#5350 fixed Underworld Breach's specific self-recast bug by narrowing active_continuous_effects_from_base_static_source's admission via a new caller-side pre-filter. That fix is scoped to the one call path Underworld Breach hits; five other statics-gathering functions share the identical "no zone-of-function check outside the command zone" defect and were still live on main: - active_static_definitions, game_functioning_statics, and battlefield_functioning_statics (functioning_abilities.rs) had no active_zones gate at all outside the command-zone branch. - active_combat_assignment_rule_effects_from_static_definitions (layers.rs, distinct from the sibling phase-rs#5350 already fixed) skipped its gate entirely whenever active_zones was empty. - compute_combat_tax (combat.rs) had no zone gate at all. Fix: a single shared predicate, static_functions_in_zone, now backs all five (CR 113.6/113.6b general case, CR 114.3/114.4 emblem exemption for the command zone, CR 113.6g stack exception for self-referential CantBeCountered/CantBeCopied distinguished from grants-to-others via `affected`). Also deletes a redundant, independently buggy duplicate (graveyard_permission_functions_in_zone) that never exempted emblems, unlike every other command-zone consumer. Each migration is backed by a permanent regression test, several proven via live revert-and-rerun during review, including a matrix covering self-referential vs. granting CantBeCountered on the stack vs. battlefield with two co-resident definitions gated independently. active_continuous_effects_from_static_definitions and its active_continuous_effects_from_base_static_source caller (phase-rs#5350's territory) are deliberately left untouched — this PR does not second-guess that fix, only extends coverage to the siblings it didn't reach. See the module doc's "Zone scope asymmetry" note for where the two approaches currently diverge. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(PR-5381): address statics gate review cleanup * test(PR-5381): cover combat assignment zone gate * fix(PR-5381): preserve chalice policy uncounterable prediction * fix(engine): admit non-emblem command-zone opt-in sources in combat tax Addresses matthewevans's CHANGES_REQUESTED review on phase-rs#5381: the compute_combat_tax loop's outer command-zone gate was emblem-only (source_obj.zone == Command && !is_emblem => skip), which silently dropped legitimate non-emblem command-zone sources (planes, schemes, conspiracies) that opt into functioning from the command zone via active_zones — the exact CR 113.6b admission rule every other command-zone-consuming gather already applies via object_sources_static_from_command_zone. Their CantAttack/CantBlock definitions never reached the per-def zone check. Fix: replace the emblem-only outer gate with object_sources_static_from_command_zone, matching the admission rule used everywhere else. Two new tests, one proving the opt-in source is now admitted (live revert-and-rerun confirmed it fails without the fix) and a negative sibling proving a non-opted-in non-emblem source still correctly taxes nothing. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> * fix(phase-ai): use public iter_unchecked instead of private iter_all CI broke the workspace build (E0624) after the chalice-avoidance uncounterable prediction started calling Definitions::iter_all, which is pub(crate) to the engine crate and not visible from phase-ai. iter_unchecked is the existing public escape hatch with identical semantics (raw slice iteration, no CR gating) intended for exactly this kind of classification/prediction use outside the engine crate. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01XbgwGxbU9NHN9kou9isp8K --------- Co-authored-by: Claude Sonnet 5 <noreply@anthropic.com> 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
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
main, no behavior change in this PR.for_each_static_effect_sourcesources these grants exclusively frombattlefield_sources(game/layers.rs), so once Breach is sacrificed and sits in the graveyard, its own grant is inert — including toward itself.granted_escape_requires_exile_cost_paymenttest, which covers the first (and only actually-reproducing) half of the original report — the exile-cost enforcement bug, already fixed byaa4ee3455.Test plan
escape_grant_from_graveyard_source_does_not_apply_to_itselfpasses locally.🤖 Generated with Claude Code