Add Angel's Grace#6093
Conversation
…-floor lift, command-zone can't-lose
- sequence.rs: compose the plural-player subject x negation boundary axis
(players/your opponents x can't/cannot) in starts_bare_and_clause_lower so
"You can't lose the game this turn and your opponents can't win the game
this turn." splits into two subject-restriction clauses (CR 104.2b +
CR 104.3e); Everybody Lives! behavior preserved.
- oracle_effect/mod.rs: try_parse_global_life_floor_replacement lifts the
duration-stripped "damage that would reduce your life total to less than N
reduces it to N instead" clause (CR 614.1a) to
Effect::AddTargetReplacement { target: None }, delegating the sentence to
parse_replacement_line; also unlocks Angel of Grace's ETB trigger.
- replacement.rs: thread the install-time source_controller into the floating
damage_target_filter check instead of a hardcoded PlayerId(0), so
Controller/Opponent scopes bind to the actual installing player
(CR 109.4 + CR 614.1a).
- sba.rs: player_has_cant_lose scans game_active_statics (battlefield +
command zone, CR 114.4) so emblem-granted can't-lose (Gideon of the Trials)
is honored, matching player_has_cant_win's existing scope.
- tests: SHAPE test with zero-Unimplemented reach guard; command-zone emblem
SBA unit test; five integration tests casting the verbatim Oracle text from
P1 (discriminates the floating controller authority) covering can't-lose,
win-block, per-event life floor, scope split, and cleanup expiry.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…ace trigger form Final review-impl findings addressed: - resolve_lose now filters players through sba::player_has_cant_lose (CR 104.3e + CR 810.8a), mirroring resolve_win's player_has_cant_win gate (CR 104.2b) — an opponent's "target player loses the game" or the caster's own Pact-style loss no longer eliminates a player protected by Angel's Grace / Platinum Angel / the Gideon of the Trials emblem. Concede (CR 104.3a) and the SBA loop (which pre-filters) are unaffected. - unit test at the seam (lose_effect_blocked_by_cant_lose_static) plus an integration test casting Angel's Grace then resolving loss effects against both the protected caster and the unprotected opponent (live reach guard). - parser SHAPE test pinning Angel of Grace's ETB-trigger form of the life-floor lift (AddTargetReplacement + UntilEndOfTurn threaded onto the executing ability). - CR 614.9 citation corrected to CR 614.1 + CR 614.3; expiry test now also asserts the can't-lose lock ended at cleanup; win/loss helper spells use synthetic names. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
There was a problem hiding this comment.
Code Review
This pull request implements support for "can't lose the game" and "can't win the game" effects, specifically targeting cards like Angel's Grace and Gideon of the Trials' emblem. It updates the loss resolution path to respect "can't lose" continuous effects, expands state-based action checks to scan all active game statics (including command-zone emblems), and fixes a bug where damage target filters for replacement effects were evaluated against a hardcoded player ID instead of the source controller. Additionally, the parser was updated to handle plural-player restriction clauses, accompanied by comprehensive unit and integration tests. The reviewer suggests expanding sibling coverage for opponent-based restriction clauses in the parser using modular, reusable parsers to avoid fragile matches.
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.
| .or(preceded( | ||
| alt(( | ||
| tag::<_, _, OracleError<'_>>("players "), | ||
| tag("your opponents "), | ||
| )), | ||
| value((), alt((tag("can't "), tag("cannot ")))), | ||
| )) |
There was a problem hiding this comment.
[MEDIUM] Expand sibling coverage for opponent-based restriction clauses using modular parsers.
Why it matters: Ensuring that other common opponent-based subjects (like "each opponent", "an opponent", or "their opponents") are covered prevents future parsing failures. To prevent fragile matches and combinatorial explosion, avoid verbatim string equality for compound phrases. Instead, decompose them into modular, reusable parsers for constituent parts (e.g., determiners and subjects) and compose them using idiomatic combinators.
Suggested fix: Refactor the parser to decompose the opponent phrases into an optional determiner followed by the opponent noun.
.or(preceded(
alt((
tag::<_, _, OracleError<'_>>("players "),
preceded(
opt(alt((tag("your "), tag("each "), tag("an "), tag("their ")))),
alt((tag("opponents "), tag("opponent ")))
),
)),
value((), alt((tag("can't "), tag("cannot ")))),
))References
- L2. Sibling coverage: If a parser arm or string was extended, are plural / possessive / negated / 'an opponent's' / 'your' / 'their' / 'non-X' / 'another' variants covered? (link)
- Avoid verbatim string equality for parsing Oracle phrases as it bypasses the robust nom-based parser and creates fragile matches. Instead, decompose compound phrases into modular, reusable parsers for constituent parts and compose them using idiomatic combinator aggregates.
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — current parser surface and grammar boundary need reconciliation.
The current-head coverage artifact from CI run 29580637255 (baseline 4a0ffc) reports 4 cards / 8 signatures, while this PR claims Angel's Grace and Angel of Grace only. In addition to the intended additions, it changes the can't duration/signature for Courageous Resolve and the until signature for Celestine Reef. Please explain or discriminate every added/removed signature and restore a current-head <!-- coverage-parse-diff --> evidence comment; no sticky artifact is presently published.
Also, crates/engine/src/parser/oracle_effect/sequence.rs:2707 admits only players / your opponents in the new bare-and can't/cannot classifier. Establish and test the grammar/class boundary with sibling forms, or explain why no other subject form can reach this safe clause boundary. The existing core wiring review is otherwise plausible; CI lint remains in progress and is not the reason for this block.
Evidence: current head c8c3572afc4b54568b2422b6d2f483d497c1d779; CI coverage artifact above. No direct builds were run.
Parse changes introduced by this PR · 5 card(s), 9 signature(s) (baseline: main
|
…x Oxford-comma regression
matthewevans requested changes on two fronts: verify the coverage-parse-diff
signature deltas for Courageous Resolve and Celestine Reef, and establish the
grammar/class boundary at sequence.rs:2707 with sibling-form evidence.
Verified against fresh Scryfall Oracle text (never memory):
- Courageous Resolve's fateful-hour list and Celestine Reef's chaos trigger
both feed "your opponents can't win the game[ this turn]" through the same
bare-and boundary Angel's Grace exercises — a real, in-scope widening.
- Discriminating the Courageous Resolve delta surfaced an actual regression
the widening exposed: peeling the opponents-clause off an Oxford-comma list
("...this turn, you can't lose the game this turn, and your opponents
can't...") left the preceding list item with a dangling comma
("can't lose the game this turn,"), which broke
`parse_restriction_modes`'s `all_consuming` match and silently regressed it
to `Effect::Unimplemented`. Fixed at the shared `push_clause_chunk` (every
chunk boundary in the module routes through it), not the one call site —
it now trims a trailing list comma the same way it already trims the
sentence-final period. Courageous Resolve now parses fully
(supported=true, gap_count=0, previously left partially Unimplemented).
- Celestine Reef's delta is a partial, non-regressing improvement: the
opponents clause now resolves to CantWinTheGame; the "you can't lose the
game" clause stays Unimplemented for an unrelated, pre-existing,
out-of-scope reason (no support for "until a player planeswalks" as a
recognized duration/until-clause) — not something this PR's diff caused
or should fix.
Kept the dead-context agent's class-boundary documentation and boundary
test as-is after independently re-verifying every cited card and every
excluded sibling form via Scryfall search: the static-line "can't win/lose"
compounds (Platinum Angel, Abyssal Persecutor, Herald of Eternal Dawn,
Cloudsteel Kirin, Gideon of the Trials' emblem, The Book of Exalted Deeds,
The Bird Champion) are intercepted by `is_cant_win_lose_compound` before
reaching this splitter; "your opponent can't" (singular) exists only in
stripped reminder text (Adventurer Beguiler); "each opponent"/"an
opponent"/"their opponents" + "can't" appear in no printed Oracle text as a
bare-and conjunct (confirmed by direct search) — gemini's suggested
determiner-generic widen would admit speculative grammar with no card to
exercise or test it, so it is declined and documented instead.
Added crates/engine/src/parser/oracle_tests.rs::
courageous_resolve_fateful_hour_list_zero_unimplemented as the
discriminating regression test (drives the real oracle-text parse, asserts
zero Unimplemented and the exact three-restriction chain in order; fails
against the unfixed push_clause_chunk).
Model: claude-sonnet-5
Thinking: high
Tier: Standard
Discrimination of the 4-card / 8-signature parse-diff (head
|
Review response — ready for re-reviewAddressed both blocking points from @matthewevans's review (head at review time: 1. Coverage-parse-diff verification. Posted a full per-card discrimination as a 2. Class boundary at Verification at
PR body's Gate A / Final review-impl / Anchored-on / Claimed-parse-impact sections are updated to this head. This PR is ready for re-review. |
matthewevans
left a comment
There was a problem hiding this comment.
Blocked — the new can't-lose enforcement is not team-aware.
🔴 Blocker
[HIGH] CantLoseTheGame protects only the named player, not that player's Two-Headed Giant team. Evidence: crates/engine/src/game/sba.rs:439-461 checks the printed and transient grants only for player_id, and crates/engine/src/game/effects/win_lose.rs:66-80 uses that predicate for effect-stated loss; crates/engine/tests/integration/angels_grace.rs:56-150 exercises only two-player fixtures. CR 810.8a (verified in docs/MagicCompRules.txt:6733) makes a player's can't-lose effect apply to the entire team. Why it matters: after one teammate casts Angel's Grace, an SBA or Target player loses the game aimed at the other teammate can still eliminate the team. Suggested fix: make the shared player_has_cant_lose authority propagate through 2HG teammates, as the adjacent life-lock helpers do, and add runtime SBA and effect-loss fixtures where only one teammate receives the transient grant.
Recommendation: request changes; retain the current parser/coverage reconciliation, then add the team-aware authority and discriminating 2HG coverage before re-review.
… 810.8a) player_has_cant_lose (sba.rs) and player_has_cant_win (static_abilities.rs) only checked the named player, so an SBA or effect-stated loss/win aimed at the OTHER Two-Headed Giant teammate could still resolve even after Angel's Grace protected their partner. CR 810.8a: "If an effect says that a player can't win the game, that player's team can't win the game. If an effect says that a player can't lose the game, that player's team can't lose the game." Both predicates now fold in the 2HG teammate's grant, mirroring the existing player_has_cant_gain_life / player_has_cant_lose_life teammate fold (CR 810.9g/810.9h siblings) already in static_abilities.rs. This is the single authority point for both the SBA path (sba.rs) and the effect-stated-loss path (win_lose.rs's resolve_lose/resolve_win), so both inherit the fix without further changes. Adds crates/engine/tests/integration/angels_grace_2hg.rs: SBA-loss and effect-stated-loss/win fixtures where only one 2HG teammate holds the transient grant, plus negative-control reach guards proving the unprotected team still loses/wins normally. Addresses matthewevans's CHANGES_REQUESTED review on PR phase-rs#6093.
Review response — 2HG team propagation (CR 810.8a), head
|
matthewevans
left a comment
There was a problem hiding this comment.
Approved on current head 0724b2a7b360e31e04c64344dcf25b6774a0c59f: the Two-Headed Giant correction is at the shared player_has_cant_lose and player_has_cant_win authorities, and the registered team fixtures discriminate both SBA and effect-stated loss/win paths. The current parse artifact is refreshed and CI is green. Approved without the quality label because the PR does not supply an exact current claimed-impact card set matching the five-card artifact.
Summary
Adds engine support for Angel's Grace.
Built for the class, not the card:
players/your opponents×can't/cannot) in the bare-and splitter — the same seam and shape as the existing Everybody Lives! arms — and each half lowers through the existingparse_restriction_modes→CantLoseTheGame/CantWinTheGametransient-continuous-effect machinery (no new effects, no new enum variants).Effect::AddTargetReplacement { target: None }(Rankle and Torbran / Don't Blink seam), delegating the sentence parse toparse_replacement_line(the Worship/Fortune Thief life-floor authority). Angel of Grace (same sentence in an ETB trigger) flips to supported by the same arm.PlayerId(0)as the controller authority in itsdamage_target_filtercheck; it now threads the install-timesource_controller, soDamageTargetPlayerScope::Controller/Opponentbind to the actual installing player for the whole floating-replacement class.player_has_cant_losenow scansgame_active_statics(battlefield + command zone, CR 114.4) instead of battlefield-only, so a command-zone emblem's "you can't lose the game" (Gideon of the Trials' emblem) is honored — matchingplayer_has_cant_win, which already had command-zone scope.resolve_lose(CR 104.3e effect-stated losses — Pacts, Door to Nothingness) now gates each loss throughplayer_has_cant_lose, mirroringresolve_win's existingplayer_has_cant_wingate; previously "can't lose the game" only blocked SBA losses. Concede (CR 104.3a) and the SBA loop are unaffected, andresolve_win's opponent elimination deliberately stays ungated (CR 104.1: a win ends the game; opponents don't "lose" per CR 104.3 — which is why these cards print both halves).Known remaining sibling gaps (distinct root causes, intentionally out of scope, coverage stays honest/red for them): Gideon of the Trials' emblem parse (single-def emblem static path + short-name normalization of the "Gideon planeswalker" subtype adjective), Serra the Benevolent's emblem (replacement text inside an emblem).
Courageous Resolve (comma-list conjunct form)— fixed in the review-response commit below, see## Review responsesection.Review response (head
765c12b39)matthewevans requested changes on two points; both are addressed in
765c12b39, appended on top ofc8c3572af(not rebased — a sibling branch,card/gideon-of-the-trials, is based on this one).1. Coverage-parse-diff verification (4 cards / 8 signatures, not 2). Re-verified against fresh Scryfall Oracle text (not memory) for all four cards. Full discrimination posted as PR comment (
<!-- coverage-parse-diff -->). Summary:"...you can't lose the game this turn, and your opponents can't win the game this turn"), which is correct and in-scope. Discriminating it surfaced a real regression the widening exposed: peeling the opponents-clause off the list left the preceding item with a dangling list comma ("can't lose the game this turn,"), which brokeparse_restriction_modes'sall_consumingmatch and silently fell toEffect::Unimplemented. Fixed at the sharedpush_clause_chunk(every chunk boundary in the module routes through it, so this is the root-cause fix, not a one-arm patch) — it now trims a trailing list comma the same way it already trims the sentence-final period. Courageous Resolve now parses fully:supported=true, gap_count=0(previously left partiallyUnimplemented; the "known remaining sibling gap" note above is stale and struck through).CantWinTheGamevia the same arm. The "you can't lose the game" clause staysUnimplementedfor an unrelated, pre-existing, out-of-scope reason: this engine has no support for "until a player planeswalks" as a recognized duration/until-clause (a Planechase-specific event duration). Not caused by this PR's diff and not fixed here —supported=false, gap_count=1(down from a fully-unparsed trigger body before this PR).2. Class boundary at
sequence.rs:2707. Reviewed the prior agent's uncommitted documentation/test diff critically rather than trusting it: independently re-verified every cited card (Platinum Angel, Abyssal Persecutor, Herald of Eternal Dawn, Cloudsteel Kirin, Gideon of the Trials' emblem, The Book of Exalted Deeds, The Bird Champion, Adventurer Beguiler) via fresh Scryfall lookups, and re-ran gemini's suggested determiner-generic widen (each/an/their/your+opponent(s)) against Scryfall oracle-text search for "and each/an/their/your opponent(s) can't" — zero matches for every sibling form. The static-line "can't win/lose" compounds are intercepted byis_cant_win_lose_compound(oracle.rs, gated behindis_granted_static_line) before ever reaching this splitter — confirmed by reading both call sites, not assumed. Verdict: kept the documentation and boundary test as committed (both positive and negative cases hold up), and declined gemini's widen — it has no card to exercise or test, matching CLAUDE.md's evidence bar.Review response 2 (head
0724b2a7b) — 2HG team propagation, CR 810.8amatthewevans's second CHANGES_REQUESTED review blocked on:
CantLoseTheGameprotected only the named player, not that player's Two-Headed Giant team.Rule verified fresh against
docs/MagicCompRules.txt:6733(CR 810.8a), quoted verbatim: "Players win and lose the game only as a team, not as individuals. If either player on a team loses the game, the team loses the game. If either player on a team wins the game, the entire team wins the game. If an effect says that a player can't win the game, that player's team can't win the game. If an effect says that a player can't lose the game, that player's team can't lose the game." The rule's own Platinum Angel example is the exact clause Angel's Grace prints: "Neither that player nor their teammate can lose the game while Platinum Angel is on the battlefield, and neither player on the opposing team can win the game."Verdict on the can't-win side: the CR text states the identical team-propagation requirement for can't-win as for can't-lose (same sentence, parallel construction — not something I inferred), and the code evidence showed
player_has_cant_win(static_abilities.rs) had the identical single-player defect asplayer_has_cant_lose. So the can't-win side is fixed in this response too, not just documented as a gap — both sides of Angel's Grace's own clause needed it.Fix, single authority point.
player_has_cant_lose(crates/engine/src/game/sba.rs) andplayer_has_cant_win(crates/engine/src/game/static_abilities.rs) each now fold in the 2HG teammate's grant, mirroring the existingplayer_has_cant_gain_life/player_has_cant_lose_lifeteammate fold already instatic_abilities.rs(CR 810.9g/810.9h siblings — the reviewer's "adjacent life-lock helpers" reference): true forplayer_idif EITHERplayer_iditself or their teammate (topology::has_two_headed_giant_shared_resources+players::teammates) has the grant. Each single-player check was extracted into a small helper (cant_lose_active_for/cant_win_active_for) that the team-aware public function wraps — no duplicated scanning logic.Both
player_has_cant_loseandplayer_has_cant_winare the single shared authority already consulted by every caller (the SBA loop'scollect_life_losers/collect_draw_from_empty_losersinsba.rs,win_lose.rs'sresolve_lose/resolve_win, and the loop-shortcut firewall inanalysis/loop_check.rs), so the fix applies at one seam and every caller inherits it — no other file needed to change.Tests added (
crates/engine/tests/integration/angels_grace_2hg.rs, registered intests/integration/main.rs): install the transientCantLoseTheGame/CantWinTheGamegrant (the sameSpecificPlayer-scoped TCE shape Angel's Grace's own resolution installs) on only ONE 2HG teammate, then exercise the OTHER teammate through the real runtime entry points:sba_loss_blocked_for_2hg_team_when_only_one_teammate_granted— SBA path (check_state_based_actions), team life total ≤ 0, only one teammate granted → team survives.sba_loss_still_applies_to_ungranted_2hg_team— negative control: identical life shape, no grant anywhere → team is eliminated (reach guard proving the SBA path is live).effect_stated_loss_blocked_for_non_granted_2hg_teammate— effect-stated path (resolve_lose, "target player loses the game" aimed at the non-granted teammate) → team survives, with an in-test reach guard against the opposing (ungranted) team.effect_stated_win_blocked_for_non_granted_2hg_teammate— can't-win discrimination (resolve_win, effect-stated win attempted by the non-granted teammate of theCantWinTheGameholder) → no-op, with a reach guard proving an unprotected win still eliminates normally.The parser/coverage reconciliation from the first review response is untouched — no changes to
sequence.rs,mod.rs, ororacle_tests.rsin this commit.Files changed
CR references
CR 104.2b, CR 104.3a, CR 104.3b, CR 104.3e, CR 109.4, CR 114.4, CR 117.3d, CR 119.7, CR 119.8, CR 514.2, CR 604.1, CR 611.2c, CR 614.1, CR 614.1a, CR 614.3, CR 810.8a, CR 810.9g, CR 810.9h — all verified by grep against docs/MagicCompRules.txt (July 2026).
Implementation method (required)
Method: /engine-implementer
Track
Developer
LLM
Model: claude-sonnet-5
Thinking: high
Tier: Frontier
Verification
Required checks ran clean, or the exact CI-owned alternative is stated below.
Gate A output below is for the current committed head.
Final review-impl below is clean for the current committed head.
Both anchors cite existing analogous code at the same seam.
cargo fmt --all— clean, no changescargo clippy --all-targets -- -D warnings(workspace) — Finished, zero warnings (exit 0)cargo test -p engine --test integration -- angels_grace— 10 passed; 0 failed (6 prior + 4 new 2HG tests)cargo test -p engine(full suite, at0724b2a7b) — lib: 16857 passed; 0 failed; 7 ignored. Integration: 3301 passed; 0 failed; 2 ignored (3297 baseline + 4 new).cargo coverage— Angel's Grace:supported=true gap_count=0; Angel of Grace:supported=true gap_count=0; Courageous Resolve:supported=true gap_count=0; Celestine Reef:supported=false gap_count=1(unrelated pre-existing gap, see Review response section) — no regression, this response touches no parser code.cargo semantic-audit— zero findings for Angel's Grace / Angel of Grace (grep -i "angel's grace\|angel of grace" data/semantic-audit.md— no match)git status --short/git diff --stat 765c12b39...HEAD— clean tree, exactly the 4 files this response touchesReview-response-2 commit (
0724b2a7b), re-run at that head:cargo fmt --all— clean, no changescargo clippy --all-targets -- -D warnings(workspace) — Finished, zero warnings (exit 0)cargo test -p engine(full suite) — lib: 16857 passed; 0 failed; 7 ignored. Integration: 3301 passed; 0 failed; 2 ignored.cargo coverage— Angel's Grace / Angel of Grace unregressed (supported=true, gap_count=0both); no card regressed (no parser files touched by this commit)cargo semantic-audit— zero findings for Angel's Grace / Angel of GraceGate A
Gate A PASS head=0724b2a7b360e31e04c64344dcf25b6774a0c59f base=765c12b39b9c7e7149d9797296372e0588b29309
Anchored on
preceded(alt(..), value((), alt(..)))boundary axis (gendered-pronoun arm) in the samestarts_bare_and_clause_lowerfunction; the new plural-player × negation axis mirrors it and replaces the enumeratedplayers can't/cannottagsparse_enter_from_zone_redirect_replacement, the existing duration-stripped floating-replacement lift toEffect::AddTargetReplacement { target: None }at the same dispatch slot afterstrip_leading_duration;try_parse_global_life_floor_replacementmirrors its contract (expiry leftNone, derived from ability duration at install) and itsparse_replacement_linedelegation followstry_parse_global_damage_modification_replacement(mod.rs:1923)check_static_ability's CR 114.4 battlefield + command-zone scan (game_functioning_statics), the authorityplayer_has_cant_winalready uses;player_has_cant_losenow mirrors it viagame_active_staticspush_clause_chunk's pre-existing.trim_end_matches('.')sentence-final-period trim is the anchor pattern; the fix extends it to a char-class trim (['.', ','])) at the same call site, not a new suppression armplayer_has_cant_gain_life/player_has_cant_lose_life's existinghas_two_headed_giant_shared_resources(state) && teammates(state, player_id).into_iter().any(...)teammate fold (CR 810.9g/810.9h);player_has_cant_loseandplayer_has_cant_winnow apply the identical fold for CR 810.8aFinal review-impl
Final review-impl PASS head=0724b2a7b360e31e04c64344dcf25b6774a0c59f
Self-review against docs/AI-CONTRIBUTOR.md §5.3 checklist (no repo-local
/review-implskill available in this runtime; performed the checklist directly againstgit diff 765c12b39..0724b2a7b):sba.rs::player_has_cant_lose,static_abilities.rs::player_has_cant_win), not a symptom-patch at a caller.player_has_cant_gain_life/player_has_cant_lose_lifeteammate-fold shape verbatim (same helpers, same short-circuit structure).docs/MagicCompRules.txt:6733); no separate layering rule applies.topology::teammates/topology::has_two_headed_giant_shared_resources, no duplicated scan logic.check_state_based_actions,resolve_lose,resolve_win) and each protected-team assertion is paired with an in-test or sibling reach guard proving the unprotected path still fires — no bare negative assertions.Claimed parse impact
No new parse impact in review-response-2 — that commit touches only runtime authority functions and tests, not the parser.
Validation Failures
None.
CI Failures
None.