fix(engine): honor dynamically granted Sunburst at battlefield entry (Solar Array #5337)#5802
Conversation
superagent-security
Bot
added
the
contributor:flagged
|
🚨 Contributor flagged. Click here for more info: Superagent Dashboard |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request implements support for granted sunburst instances (e.g., from cards like Solar Array or Lux Artillery) in accordance with MTG Comprehensive Rules (CR 702.44a/b/d). It extracts the sunburst replacement definition into a shared helper, introduces a virtual replacement candidate for granted sunburst instances, ensures multiple sunburst instances coexist on the keyword list without deduplication, and adds comprehensive integration tests. The review feedback suggests refactoring the defensive check in apply_granted_sunburst_replacement to avoid manually rebuilding the ProposedEvent::ZoneChange struct, which reduces boilerplate and prevents future compilation breakages when new fields are added.
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.
| let ProposedEvent::ZoneChange { | ||
| object_id, | ||
| from, | ||
| to, | ||
| cause, | ||
| attach_to, | ||
| enter_tapped, | ||
| mut enter_with_counters, | ||
| controller_override, | ||
| enter_transformed, | ||
| face_down_profile, | ||
| applied, | ||
| } = event | ||
| else { | ||
| return event; | ||
| }; | ||
| if object_id != rid.source { | ||
| // Rebuild the event unchanged if the ids diverged (defensive; the | ||
| // candidate is keyed on the entering object so this never fires). | ||
| return ProposedEvent::ZoneChange { | ||
| object_id, | ||
| from, | ||
| to, | ||
| cause, | ||
| attach_to, | ||
| enter_tapped, | ||
| enter_with_counters, | ||
| controller_override, | ||
| enter_transformed, | ||
| face_down_profile, | ||
| applied, | ||
| }; | ||
| } |
There was a problem hiding this comment.
[MEDIUM] Avoid manual rebuild of ProposedEvent::ZoneChange in defensive check.
Why it matters: Manually rebuilding the ProposedEvent::ZoneChange struct with all its fields creates a maintenance burden, as any future addition of new fields to ZoneChange will break this compilation. Checking the object_id via a reference borrow before moving/destructuring the event allows returning the original event directly on mismatch, eliminating the boilerplate and future-proofing the code.
let ProposedEvent::ZoneChange { object_id, .. } = &event else {
return event;
};
if *object_id != rid.source {
return event;
}
let ProposedEvent::ZoneChange {
object_id,
from,
to,
cause,
attach_to,
enter_tapped,
mut enter_with_counters,
controller_override,
enter_transformed,
face_down_profile,
applied,
} = event
else {
unreachable!();
};
Parse changes introduced by this PR✓ No card-parse changes detected. |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — granted Sunburst is incorrectly declared order-independent from counter-doubling replacements.
🔴 Blocker
candidate_materialityreturnsDisjointfor the virtual Sunburst candidate (crates/engine/src/game/replacement.rs:7050-7056), while its applier appendsenter_with_countersto the sameZoneChangeevent (crates/engine/src/game/replacement.rs:334-440). A counter doubler can therefore be applied first to an empty payload, after which Sunburst appends counters that are no longer doubled; the reverse ordering doubles them. These outcomes differ, so CR 616 ordering must remain available. The new doubled-Sunburst test does not exercise both ordering choices.
✅ Clean
- The virtual candidate uses the existing replacement pipeline and shares the printed-Sunburst definition authority, which is the right seam for the granted-instance gap.
Recommendation: classify the candidate as a counter-payload write (or conservatively order-sensitive) and add an integration regression proving both legal orderings against a counter doubler before re-review.
… write (CR 616.1e) Review on phase-rs#5802: the virtual granted-sunburst candidate was classified `Disjoint` in `candidate_materiality`, but its applier appends to the same event counter payload a co-firing Count writer modifies — an append does not commute with a doubler ((0+N)*2 vs 0*2+N), so `Disjoint` silently suppressed the CR 616.1e affected-controller ordering choice. Reclassify as `Writes { field: Count, commute: Additive }`: two appenders still commute (no degenerate prompt), while an appender + any non-additive Count writer on one event now surfaces the ordering prompt. Integration regression (both fail on revert to `Disjoint` with "the CR 616.1e ordering prompt must surface"): granted sunburst + a same-event Moved-keyed `quantity_modification(DOUBLE)` writer co-fire on the entering spell's ZoneChange; the ordering prompt lists both candidates, and BOTH legal orderings are driven end-to-end to a clean entry with the granted payload intact. Empirical note recorded in the test doc: the two orderings currently converge (2 counters each) because a bare `quantity_modification` on a Moved-keyed definition has no ZoneChange counter-payload applier yet — the functioning Doubling Season path scales the downstream AddCounter placement instead (asserted at 4 by `granted_sunburst_participates_in_counter_ doubling`). The reclassification pins the ordering machinery so that if a ZoneChange payload applier lands later, only the expected totals change (4 sunburst-first vs 2 writer-first), not the choice surface. Full lib: 16549 passed. Integration: 3040 passed. Clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Pushed Regression added, both orderings driven end-to-end: One empirical finding to flag while writing the regression (recorded in the test doc): the two orderings currently CONVERGE (2 counters each) rather than diverge (4 vs 2) — a bare Happy to add the ZoneChange counter-payload applier as a follow-up if you'd like the divergent totals realized now rather than pinned for later. Full lib 16549 / integration 3040 green, clippy clean. |
matthewevans
removed
the
contributor:flagged
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Request changes — generalize virtual ETB keyword handling beyond Sunburst.
🟡 Finding
[MED] The virtual ETB path handles only dynamically granted Sunburst and ignores the existing granted Bloodthirst class. Why it matters: the fix creates a new special case while leaving an equivalent keyword path incorrect. Suggested fix: generalize the existing granted-keyword handling and add a Bloodlord regression.
Recommendation: request-changes.
…(Solar Array phase-rs#5337) "That spell gains sunburst" (Solar Array) and "it gains sunburst" (Lux Artillery) placed ZERO as-enters counters. Three stacked defects, fixed per layer: 1. Parser — the grant never landed (Solar Array). In a "when you next cast a <spell> this turn" delayed trigger, the subject-position "that spell" anaphor lowered to `affected: ParentTarget`; a `WhenNextEvent` delayed trigger has no parent target, so `register_transient_effect` bound the grant against the empty chain-tracked set and silently dropped it. A new lift (`lift_generic_effect_parent_target_to_triggering_source_in_ability`, mirroring the oracle_trigger.rs lift family) rebinds GenericEffect grants in a WhenNextEvent body to `TriggeringSource` — the just-cast spell is the event source (CR 608.2k). 2. Engine — a landed grant still placed no counters (the class bug, also breaking Lux Artillery). Printed sunburst is realized as an object-carried ETB `ReplacementDefinition` pre-synthesized from the card face's PRINTED keywords (`synthesize_sunburst`); a granted keyword adds no replacement and nothing consulted live keywords at entry. A granted-instance VIRTUAL replacement candidate now surfaces in `find_applicable_replacements` (alongside the shield/finality-counter virtual candidates), built from the same shared `sunburst_replacement_definition` authority the printed synthesis uses, so it participates in normal CR 616 replacement ordering (Doubling Season doubles it). Granted instances are counted as EFFECTIVE minus BASE keywords via `effective_off_zone_keywords` — the entering spell is still on the STACK when its entry pipeline runs, and a granted keyword exists only as a continuous effect at that moment (the materialized keyword list is empty), so the off-zone authority is the only correct read. The base subtraction keeps printed instances on their carried definitions — printed + granted apply separately, exactly CR 702.44d. 3. Engine — cast-trigger resolution wiped the color provenance. Resolving an intervening cast trigger (Lux Artillery's own grant trigger) cleared `colors_spent_to_cast` on objects still on the STACK, erasing the color count before the granted spell entered. The clear now preserves live cast provenance for stack objects (CR 601.2h), mirroring how cast_from_zone is preserved. Supporting changes: `Keyword` instance-coexistence predicate extended so a granted Sunburst survives next to an identical printed instance (CR 702.44d "each one works separately"; previously only Toxic's summation kept duplicates); `synthesize_sunburst`'s per-instance definition extracted into the shared `sunburst_replacement_definition` builder used by both the printed synthesis and the virtual candidate. Tests (integration, real activate/cast/trigger/replacement pipeline, verbatim Oracle texts): - Solar Array: creature cast for 3 colors -> 3 +1/+1; noncreature for 2 -> 2 charge; zero colored mana -> 0 counters (CR 702.44b). - Lux Artillery: 2 colors -> 2 +1/+1 (gap 2+3 canary). - Printed control: 3 colors -> 3 charge (carried-definition path untouched). - Printed + granted: 2 colors -> 4 charge (CR 702.44d separateness). - Counter doubling: granted sunburst under an AddCounter doubler -> 4 (CR 616 ordering through the virtual candidate). - Parser shape: the delayed grant lowers `affected: TriggeringSource` with the Sunburst AddKeyword (gap 1 canary). Full lib suite: 16549 passed, 0 failed. Integration: 3038 passed, 0 failed. Fixes phase-rs#5337. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…py empty-line-after-doc) The phase-rs#5337 parser-shape test was inserted between an existing doc comment and its function; reattach the comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
… write (CR 616.1e) Review on phase-rs#5802: the virtual granted-sunburst candidate was classified `Disjoint` in `candidate_materiality`, but its applier appends to the same event counter payload a co-firing Count writer modifies — an append does not commute with a doubler ((0+N)*2 vs 0*2+N), so `Disjoint` silently suppressed the CR 616.1e affected-controller ordering choice. Reclassify as `Writes { field: Count, commute: Additive }`: two appenders still commute (no degenerate prompt), while an appender + any non-additive Count writer on one event now surfaces the ordering prompt. Integration regression (both fail on revert to `Disjoint` with "the CR 616.1e ordering prompt must surface"): granted sunburst + a same-event Moved-keyed `quantity_modification(DOUBLE)` writer co-fire on the entering spell's ZoneChange; the ordering prompt lists both candidates, and BOTH legal orderings are driven end-to-end to a clean entry with the granted payload intact. Empirical note recorded in the test doc: the two orderings currently converge (2 counters each) because a bare `quantity_modification` on a Moved-keyed definition has no ZoneChange counter-payload applier yet — the functioning Doubling Season path scales the downstream AddCounter placement instead (asserted at 4 by `granted_sunburst_participates_in_counter_ doubling`). The reclassification pins the ordering machinery so that if a ZoneChange payload applier lands later, only the expected totals change (4 sunburst-first vs 2 writer-first), not the choice surface. Full lib: 16549 passed. Integration: 3040 passed. Clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…(Bloodlord of Vaasgoth, phase-rs#5802 review) matthewevans's [MED] review: the virtual ETB replacement handled only granted Sunburst, leaving the equivalent granted Bloodthirst path (Bloodlord of Vaasgoth: "Whenever you cast a Vampire creature spell, it gains bloodthirst 3") incorrect — a new special case beside an equally-broken keyword. Generalize the machinery into a keyword-agnostic granted-ETB-keyword path covering both Sunburst and Bloodthirst (and any future as-enters keyword): - `GrantedEtbKeyword { Sunburst, Bloodthirst }` with per-keyword reserved virtual-candidate ids (`GRANTED_SUNBURST_INDEX = MAX-7`, `GRANTED_BLOODTHIRST_INDEX = MAX-8`) feeding a shared count/apply core; `rid.index` recovers the keyword. - `granted_keyword_etb_instances` factors out the EFFECTIVE-minus-BASE keyword count (via `effective_off_zone_keywords`, since the entering spell is still on the stack); `granted_sunburst_instances` delegates to it, `granted_bloodthirst_instances` counts per distinct `BloodthirstValue` (mirroring `synthesize_bloodthirst`, CR 702.54c). - `bloodthirst_replacement_definition` extracted in synthesis.rs (mirroring `sunburst_replacement_definition`); both the printed synthesizer and the virtual applier build per-instance definitions from the same authority. - `apply_granted_keyword_etb_replacement` is keyword-agnostic and honors each definition's carried `condition` via `evaluate_replacement_condition` — Bloodthirst fixed-N only places counters when an opponent was dealt damage this turn (CR 702.54a); Sunburst has `condition: None` and always applies. `granted_etb_keyword_candidate_applies` re-checks the condition at registration so a condition-unmet grant raises no spurious CR 616.1e prompt. - `find_applicable_replacements` registers both keywords on `ZoneChange`→Battlefield; materiality stays `Writes { Count, Additive }`. - `keywords.rs`: Bloodthirst added to the instance-coexistence predicate so a granted instance survives beside an identical printed one (CR 702.54c). Also addresses Gemini's nit: the applier no longer rebuilds `ProposedEvent::ZoneChange` field-by-field — it mutates `enter_with_counters` in place via `if let ProposedEvent::ZoneChange { enter_with_counters, .. } = &mut event`, immune to new event fields. Rebased onto current main (resolved the `usize::MAX - 6` index collision with the new commander-return virtual candidate; `GRANTED_SUNBURST_INDEX` moved to MAX-7). Tests (integration, real cast/trigger/replacement pipeline): granted Bloodthirst 3 with an opponent damaged this turn → 3 +1/+1 counters; NO opponent damaged → 0 counters (condition revert-canary); end-to-end via Bloodlord's real "it gains bloodthirst 3" trigger → 3 counters. All 9 existing sunburst tests stay green. Full lib: 16959 passed. Integration: 3349 passed. Clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
23135a6 to
af975c2
Compare
superagent-security
Bot
added
the
contributor:flagged
|
Pushed [MED, matthewevans] Generalized beyond Sunburst to granted BloodthirstThe virtual ETB machinery is now keyword-agnostic, covering both granted Sunburst and granted Bloodthirst (Bloodlord of Vaasgoth: "Whenever you cast a Vampire creature spell, it gains bloodthirst 3"):
New tests (revert-probed): granted Bloodthirst 3 + opponent damaged → 3 counters; no opponent damaged → 0 counters (condition canary — disabling the condition check makes this place 3, and the test fails); end-to-end through Bloodlord's real trigger → 3 counters. All 9 sunburst tests stay green. [Gemini] ProposedEvent::ZoneChange rebuildThe applier no longer reconstructs the struct field-by-field — it mutates [HIGH, prior round] materialityAlready resolved on the current head — the candidate is Rebased onto current main (resolved a |
|
Current follow-up reviewed — prior request-changes findings are resolved on .\n\n## ✅ Clean\n\n- The virtual granted-ETB candidate is now an additive Count writer, so it remains order-material with non-additive counter writers; both ordering paths are exercised.\n- The virtual path is parameterized over granted Sunburst and Bloodthirst, with Bloodthirst's opponent-damaged condition covered through the real Bloodlord trigger.\n- Gemini's ZoneChange reconstruction concern is resolved by the current in-place payload mutation.\n\nRecommendation: hold pending the current required CI run, then approve/enqueue if it remains green. |
|
Correction: the reviewed current head is af975c2. The prior request-changes findings are resolved there; this remains held only for the current required CI run before approval/enqueue. |
|
Current source review is clean, but approval/enqueue is blocked by the required Contributor trust check, which is ACTION_REQUIRED and links to the existing superagent-security flagged-contributor notice. This sweep does not override required trust checks. |
Maintainer-side bring-current. This branch was reviewed clean on 2026-07-18 at head af975c2 and the author has not touched it since; main has advanced 241 commits underneath it, which is what turned it DIRTY. The staleness is maintainer-caused, so the port is ours. This commit keeps the contributor's `clear_post_collection_transients` hunk verbatim so the merge itself carries no judgement. The semantic resolution follows in the next commit.
…in's cast-payment-stamp authority Conflict resolution for the bring-current merge, isolated here so the judgement is reviewable on its own. This branch guarded `clear_post_collection_transients` so a spell still on the Stack kept `mana_spent_to_cast` / `colors_spent_to_cast`, because a GRANTED sunburst (Solar Array / Lux Artillery) enters AFTER the intervening cast-triggered grant resolves and that clear runs — wiping the color tally made it place zero counters (phase-rs#5337). While this branch sat, main fixed the same underlying seam independently for issue phase-rs#5943: `clear_post_collection_transients` now routes all five cast- payment stamps through `GameObject::clear_cast_payment_stamps()` and calls it ONLY for objects outside the Battlefield/Stack provenance zones. So `colors_spent_to_cast` — the field granted Sunburst actually reads, via `colors_spent_to_cast.distinct_colors()` — already survives on Stack objects under main's authority. The remaining unconditional line clears only the `mana_spent_to_cast` boolean, which main documents as a deliberate per-collection transient and which this feature does not read. Taking main's side therefore preserves this branch's behavior while keeping a single authority for payment-stamp lifetime, instead of layering a second, partly-redundant guard beside it. Verified, not assumed: with main's side taken, all 12 of this branch's own regressions pass — granted_sunburst_5337 (9) and granted_bloodthirst_5802 (3), including lux_artillery_grants_sunburst_two_colors_enters_with_two_p1p1 and solar_array_grants_sunburst_creature_three_colors_enters_with_three_p1p1, which are exactly the cases that fail if stack color provenance is wiped. Full `cargo test -p engine`: 21,496 passed, 0 failed. Co-authored-by: shin-core <153108882+shin-core@users.noreply.github.com>
📝 WalkthroughWalkthroughGranted Sunburst and Bloodthirst keywords now create virtual battlefield-entry counter replacements when granted to spells. Delayed grants rebind “that spell” correctly, while shared synthesis helpers and integration tests cover conditions, stacking, counter doubling, and replacement ordering. ChangesGranted as-enters keyword handling
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant SpellOnStack
participant ReplacementCandidates
participant GrantedEtbReplacement
participant Battlefield
SpellOnStack->>ReplacementCandidates: expose granted keyword ETB candidates
ReplacementCandidates->>GrantedEtbReplacement: apply matching virtual candidate
GrantedEtbReplacement->>SpellOnStack: add per-instance counter groups
SpellOnStack->>Battlefield: enter with counters
Suggested labels: Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Approved — current head 44e2119c11 (maintainer port of af975c2c4).
Clearing the stale CHANGES_REQUESTED state. Both request-changes reviews (the CR 616.1e ordering-materiality blocker on 07-14, and the generalize-beyond-Sunburst finding on 07-16) were confirmed resolved on af975c2c4 in the 2026-07-18 follow-up; every review after that point was a hold on pending CI, not on a code finding. The formal state simply never got cleared.
Staleness was ours, so the port was ours
This branch was reviewed clean on 2026-07-18 and @shin-core has not touched it since. main then advanced 241 commits underneath it, which is what turned it DIRTY. That is maintainer-caused staleness, so the maintainer-fixup size cap does not apply and the rebase is our work, not the author's. Pushed as two commits, with every one of @shin-core's commits keeping its original author identity:
d1e2a8f530—Merge branch 'main', taking the contributor's hunk verbatim so the merge itself carries no judgement.44e2119c11— the one conflict resolution, isolated for review, trailingCo-authored-by: @shin-core.
The conflict, and why it resolved this way. The single conflict was in clear_post_collection_transients (game/triggers.rs). This branch guarded that clear so a spell still on the Stack kept mana_spent_to_cast / colors_spent_to_cast, because a granted Sunburst enters after the intervening cast-triggered grant resolves and that clear runs. While the branch sat, main fixed the same seam independently for issue #5943: the clear now routes all five payment stamps through GameObject::clear_cast_payment_stamps() and calls it only for objects outside the Battlefield/Stack provenance zones. So colors_spent_to_cast — the field granted Sunburst actually reads, via distinct_colors() — already survives on Stack objects under main's authority. The remaining unconditional line clears only the mana_spent_to_cast boolean, which main documents as a deliberate per-collection transient and which this feature never reads. Taking main's side keeps one authority for payment-stamp lifetime instead of layering a second, partly-redundant guard beside it.
Verified, not assumed. With main's side taken, all 12 of this branch's own regressions pass — granted_sunburst_5337 (9) and granted_bloodthirst_5802 (3), including lux_artillery_grants_sunburst_two_colors_enters_with_two_p1p1 and solar_array_grants_sunburst_creature_three_colors_enters_with_three_p1p1, which are precisely the cases that fail if stack color provenance is wiped. Full cargo test -p engine in an isolated worktree: 21,496 passed, 0 failed. cargo fmt --all and git diff --check clean. mergeable has flipped CONFLICTING → MERGEABLE.
Security re-read (contributor is trust-flagged)
Contributor trust is ACTION_REQUIRED from the superagent-security app, so I re-read the complete current diff rather than relying on the earlier pass. It is not in main's branch-protection contexts (which require only Rust (fmt, clippy, test, coverage-gate) and Frontend (lint, type-check, test)) and not in the aggregator's needs: list, so it does not gate the queue. What I checked, explicitly:
- Paths — 8 files, all under
crates/engine/. No.github/workflows/, no.github/actions/, no.claude/, noCLAUDE.md/AGENTS.md/AI-CONTRIBUTOR.md, nopackage.json, nobuild.rs, no.gitignore, noCargo.toml. No binaries, no gitlinks/submodules. - Capability surface — grepped every added line for
std::env/env!,std::process/Command::new, any HTTP/socket client,std::fs/File::open/write_all/remove_file,unsafe,include_bytes!/include_str!, base64/decode, and token/secret/credential strings. Zero hits. The onlypanic!s are expected-shape assertions inside tests. - Prompt injection — no instructions addressed to a reviewing LLM, no fake
<system>tags, no attempts to redefine project rules in comments, docs, or fixtures. - Scope — the diff matches the stated scope. The one parser change (
oracle_effect/mod.rs) is narrow and on-topic: it lifts aParentTargetanaphor toTriggeringSourceforGenericEffectgrants inside aWhenNextEventbody, which is Solar Array's "when you next cast" form — the issue under fix. The sticky parse-diff reports no card-parse changes.
Nothing malicious or out-of-scope. The flag is not corroborated by the diff.
Architecture
Right seam and idiomatic. GrantedEtbKeyword { Sunburst, Bloodthirst } is a parameterized enum feeding one shared count/apply core (granted_keyword_etb_instances / apply_granted_keyword_etb_replacement) rather than two duplicated keyword paths — a future third as-enters keyword is one more reserved index. It reuses the existing virtual-ID protocol already shared by intrinsic shield/finality/compleated, routes conditions through the same evaluate_replacement_condition seam the printed path uses, and reads keywords through effective_off_zone_keywords (the single authority for stack objects, whose keywords are not yet materialized). CR annotations are present and specific throughout.
One non-blocking observation, recorded rather than actioned: Keyword::sums_across_instances() now covers two distinct reasons — parameter summation (Toxic, CR 702.164b) and instance-count multiplicity (Sunburst/Bloodthirst, CR 702.44d / 702.54c). The doc comment states this explicitly and both are served by the same "don't dedup identical instances" mechanic, so the behavior is right, but the name now under-describes the predicate. A rename to something like instances_must_coexist would fit better. I deliberately did not do it in this port: it touches call sites beyond the conflict and would widen the blast radius of a rebase that is already carrying 241 commits of drift.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
crates/engine/src/types/keywords.rs (1)
1749-1754: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
sums_across_instancesname no longer matches half its own semantics.The doc block itself distinguishes two disjoint reasons a keyword belongs here: Parameter-value summation (CR 702.164b): the aggregate reader sums each instance's parameter (a creature's total toxic value). Only Toxic sums today... Instance-count multiplicity (CR 702.44d + CR 702.54c): an as-enters static ability where "each instance works separately"... A GRANTED Sunburst... or GRANTED Bloodthirst... on top of an identical printed one must coexist so both the printed object-carried replacement AND the granted virtual replacement... fire. Sunburst/Bloodthirst don't "sum" anything — they coexist as separate, independently-firing replacement instances. The name only accurately describes the Toxic case; callers reading
sums_across_instances()at the single call site inlayers.rswill reasonably assume it always implies numeric summation.Consider renaming to something like
coexists_when_granted(or similar) to match the umbrella concept the doc already describes, since the function now serves two distinct rules purposes under one label.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/types/keywords.rs` around lines 1749 - 1754, Rename Keyword::sums_across_instances and its single caller in layers.rs to an umbrella name such as coexists_when_granted that covers both Toxic parameter aggregation and Sunburst/Bloodthirst instance multiplicity. Update the associated documentation and all references consistently, preserving the existing keyword matching behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/engine/src/database/synthesis.rs`:
- Around line 7226-7243: The granted Sunburst replacement path must derive its
object types from the printed/base snapshot rather than live `card_types`.
Update the logic around `sunburst_replacement_definition` and its
granted-keyword caller (`granted_sunburst_replacements`) to use
`base_card_types` or the appropriate face-specific base types, matching the
printed `synthesize_sunburst` behavior while preserving separate replacement
instances.
---
Nitpick comments:
In `@crates/engine/src/types/keywords.rs`:
- Around line 1749-1754: Rename Keyword::sums_across_instances and its single
caller in layers.rs to an umbrella name such as coexists_when_granted that
covers both Toxic parameter aggregation and Sunburst/Bloodthirst instance
multiplicity. Update the associated documentation and all references
consistently, preserving the existing keyword matching behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: 27c773b6-7c83-4368-b53b-3fdeb852417f
📒 Files selected for processing (8)
crates/engine/src/database/synthesis.rscrates/engine/src/game/replacement.rscrates/engine/src/parser/oracle_effect/mod.rscrates/engine/src/parser/oracle_effect/tests.rscrates/engine/src/types/keywords.rscrates/engine/tests/integration/granted_bloodthirst_5802.rscrates/engine/tests/integration/granted_sunburst_5337.rscrates/engine/tests/integration/main.rs
|
Not enqueued — blocked by a maintainer-side breakage on
That job is a Bringing this branch current necessarily inherited that breakage — before the port, this PR's base predated the release commit, so its Tauri check passed. Enqueuing now would just get the PR bounced and would misattribute a maintainer failure to your work, so I'm holding it here instead. State: approved, Local verification on this head, for the record: full |
…gate keyword sweep Maintainer review fixups on top of phase-rs#5802. CR 702.44a rules defect: `granted_etb_replacement_definitions` branched the Sunburst counter type on `obj.card_types`, the LIVE layer result. The rule says sunburst applies "if this object is entering as a creature, IGNORING ANY TYPE-CHANGING EFFECTS that would affect it", and Layer-6 type effects do reach stack objects (`remote_type_layer_recipients`) — which is the zone this check runs in. Branch on `base_card_types` instead, the same printed `card_face.card_type` the printed synthesizer (`synthesize_sunburst`) uses, so the granted and printed paths agree. Adds the first fixture whose printed and live core types DIVERGE; all 12 existing fixtures set `base_card_types == card_types`, so this arm was previously unexercised and the defect was invisible. Perf on the `find_applicable_replacements` hot path (states are cloned and replayed constantly under AI search): the `ZoneChange`→Battlefield gate ran a full off-zone keyword sweep per keyword family, each one a whole-game continuous-effect collect plus ordering and per-effect filter evaluation, and the guard was ordered expensive-term-first. Test the cheap `already_applied` set lookup first, and resolve the live keyword list at most once (lazily) and thread it through the family helpers so one sweep serves every family. Also renames `Keyword::sums_across_instances` to `instances_must_coexist` — all three call sites implement "don't dedup identical instances" and none sums a parameter, matching the predicate's own doc — and repoints a dangling doc reference to `granted_sunburst_replacements`, a symbol that does not exist. Co-authored-by: shin-core <153108882+shin-core@users.noreply.github.com>
|
Maintainer fixups pushed — 1. CR 702.44a — branch on PRINTED, not live, core types (rules defect)
// CR 702.44a: branch on the entering object's current core types.
.filter(|obj| obj.card_types.core_types.contains(&CoreType::Creature))
Now reads This also confirms CodeRabbit's inline finding at The fix needed a test to be real. All 12 existing fixtures set 2. Hot-path cost in
|
There was a problem hiding this comment.
🧹 Nitpick comments (1)
crates/engine/src/game/off_zone_characteristics.rs (1)
377-377: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUpdate the off-zone dedup comment
crates/engine/src/game/off_zone_characteristics.rs:371-377—instances_must_coexist()now keeps Toxic, Sunburst, and Bloodthirst instances, but this note still only describes Toxic’s summing behavior under CR 702.164b. Rewrite it to match the full contract and add a regression test for a granted non-Toxic coexisting keyword.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/engine/src/game/off_zone_characteristics.rs` at line 377, Update the comment immediately above the instances_must_coexist() check to document that Toxic, Sunburst, and Bloodthirst instances are retained, rather than describing only Toxic’s CR 702.164b summing behavior. Add a regression test covering a granted non-Toxic coexisting keyword, using the existing off-zone characteristics test helpers and asserting that its instances are preserved.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@crates/engine/src/game/off_zone_characteristics.rs`:
- Line 377: Update the comment immediately above the instances_must_coexist()
check to document that Toxic, Sunburst, and Bloodthirst instances are retained,
rather than describing only Toxic’s CR 702.164b summing behavior. Add a
regression test covering a granted non-Toxic coexisting keyword, using the
existing off-zone characteristics test helpers and asserting that its instances
are preserved.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro Plus
Run ID: c8416d27-94a7-403e-8635-61e41d0694d8
📒 Files selected for processing (6)
crates/engine/src/database/synthesis.rscrates/engine/src/game/layers.rscrates/engine/src/game/off_zone_characteristics.rscrates/engine/src/game/replacement.rscrates/engine/src/types/keywords.rscrates/engine/tests/integration/granted_sunburst_5337.rs
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/engine/src/database/synthesis.rs
- crates/engine/src/game/replacement.rs
- crates/engine/tests/integration/granted_sunburst_5337.rs
matthewevans
left a comment
matthewevans
left a comment
There was a problem hiding this comment.
Re-approving on f0b3f227 to refresh the approval onto the current head.
The earlier approval was recorded against 44e2119c (22:14). The maintainer fixup f0b3f227 (22:50) landed afterwards, and since auto-merge is armed, that left an approval covering a head that no longer exists. Approval freshness attaches to a head, not to a PR number, so this re-approval closes that gap rather than letting the stale one carry.
Verified independently before re-approving, not taken on trust from the fixup's own commit message:
- CR 702.44a is verbatim at
docs/MagicCompRules.txt:4336— "If this object is entering as a creature, ignoring any type-changing effects that would affect it…" The fixup's diagnosis holds: branching the counter type onobj.card_typesreads the live layer result, and Layer-6 type effects do reach stack objects viaremote_type_layer_recipients, which is the zone this check runs in.base_card_typesis the correct axis, and it makes the granted path agree withsynthesize_sunburst's printed-type read instead of diverging from it. - The new fixture is the one that matters. All 12 pre-existing fixtures set
base_card_types == card_types, so this arm was structurally unexercised — the defect could not have been observed by the existing suite regardless of coverage numbers. Adding the first fixture whose printed and live core types diverge is what turns this from an assertion into evidence. - The
sums_across_instances→instances_must_coexistrename is complete. Five occurrences acrosstypes/keywords.rs(definition + doc cross-reference),game/layers.rs(two call sites), andgame/off_zone_characteristics.rs(one); the commit's per-file line counts account for exactly those five. It is a method rename, so any missed call site is a compile error rather than a silent behavior change — the compiler is the census here. The new name is also the more accurate one: all three call sites implement "don't dedup identical instances" and none sums a parameter.
The perf change on find_applicable_replacements — testing the cheap already_applied set membership before the keyword sweep, and resolving the live keyword list lazily once instead of per keyword family — is on a genuinely hot path (states are cloned and replayed constantly under AI search) and is a reordering plus memoization, not a semantic change.
This stays behind the queue's own required checks; the re-approval is about the review record being accurate, not about bypassing CI.
…(Solar Array phase-rs#5337) (phase-rs#5802) * fix(engine): honor dynamically granted Sunburst at battlefield entry (Solar Array phase-rs#5337) "That spell gains sunburst" (Solar Array) and "it gains sunburst" (Lux Artillery) placed ZERO as-enters counters. Three stacked defects, fixed per layer: 1. Parser — the grant never landed (Solar Array). In a "when you next cast a <spell> this turn" delayed trigger, the subject-position "that spell" anaphor lowered to `affected: ParentTarget`; a `WhenNextEvent` delayed trigger has no parent target, so `register_transient_effect` bound the grant against the empty chain-tracked set and silently dropped it. A new lift (`lift_generic_effect_parent_target_to_triggering_source_in_ability`, mirroring the oracle_trigger.rs lift family) rebinds GenericEffect grants in a WhenNextEvent body to `TriggeringSource` — the just-cast spell is the event source (CR 608.2k). 2. Engine — a landed grant still placed no counters (the class bug, also breaking Lux Artillery). Printed sunburst is realized as an object-carried ETB `ReplacementDefinition` pre-synthesized from the card face's PRINTED keywords (`synthesize_sunburst`); a granted keyword adds no replacement and nothing consulted live keywords at entry. A granted-instance VIRTUAL replacement candidate now surfaces in `find_applicable_replacements` (alongside the shield/finality-counter virtual candidates), built from the same shared `sunburst_replacement_definition` authority the printed synthesis uses, so it participates in normal CR 616 replacement ordering (Doubling Season doubles it). Granted instances are counted as EFFECTIVE minus BASE keywords via `effective_off_zone_keywords` — the entering spell is still on the STACK when its entry pipeline runs, and a granted keyword exists only as a continuous effect at that moment (the materialized keyword list is empty), so the off-zone authority is the only correct read. The base subtraction keeps printed instances on their carried definitions — printed + granted apply separately, exactly CR 702.44d. 3. Engine — cast-trigger resolution wiped the color provenance. Resolving an intervening cast trigger (Lux Artillery's own grant trigger) cleared `colors_spent_to_cast` on objects still on the STACK, erasing the color count before the granted spell entered. The clear now preserves live cast provenance for stack objects (CR 601.2h), mirroring how cast_from_zone is preserved. Supporting changes: `Keyword` instance-coexistence predicate extended so a granted Sunburst survives next to an identical printed instance (CR 702.44d "each one works separately"; previously only Toxic's summation kept duplicates); `synthesize_sunburst`'s per-instance definition extracted into the shared `sunburst_replacement_definition` builder used by both the printed synthesis and the virtual candidate. Tests (integration, real activate/cast/trigger/replacement pipeline, verbatim Oracle texts): - Solar Array: creature cast for 3 colors -> 3 +1/+1; noncreature for 2 -> 2 charge; zero colored mana -> 0 counters (CR 702.44b). - Lux Artillery: 2 colors -> 2 +1/+1 (gap 2+3 canary). - Printed control: 3 colors -> 3 charge (carried-definition path untouched). - Printed + granted: 2 colors -> 4 charge (CR 702.44d separateness). - Counter doubling: granted sunburst under an AddCounter doubler -> 4 (CR 616 ordering through the virtual candidate). - Parser shape: the delayed grant lowers `affected: TriggeringSource` with the Sunburst AddKeyword (gap 1 canary). Full lib suite: 16549 passed, 0 failed. Integration: 3038 passed, 0 failed. Fixes phase-rs#5337. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * style: restore the Magus Lucea Kane doc comment to its function (clippy empty-line-after-doc) The phase-rs#5337 parser-shape test was inserted between an existing doc comment and its function; reattach the comment. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * fix(engine): classify granted sunburst as an additive counter-payload write (CR 616.1e) Review on phase-rs#5802: the virtual granted-sunburst candidate was classified `Disjoint` in `candidate_materiality`, but its applier appends to the same event counter payload a co-firing Count writer modifies — an append does not commute with a doubler ((0+N)*2 vs 0*2+N), so `Disjoint` silently suppressed the CR 616.1e affected-controller ordering choice. Reclassify as `Writes { field: Count, commute: Additive }`: two appenders still commute (no degenerate prompt), while an appender + any non-additive Count writer on one event now surfaces the ordering prompt. Integration regression (both fail on revert to `Disjoint` with "the CR 616.1e ordering prompt must surface"): granted sunburst + a same-event Moved-keyed `quantity_modification(DOUBLE)` writer co-fire on the entering spell's ZoneChange; the ordering prompt lists both candidates, and BOTH legal orderings are driven end-to-end to a clean entry with the granted payload intact. Empirical note recorded in the test doc: the two orderings currently converge (2 counters each) because a bare `quantity_modification` on a Moved-keyed definition has no ZoneChange counter-payload applier yet — the functioning Doubling Season path scales the downstream AddCounter placement instead (asserted at 4 by `granted_sunburst_participates_in_counter_ doubling`). The reclassification pins the ordering machinery so that if a ZoneChange payload applier lands later, only the expected totals change (4 sunburst-first vs 2 writer-first), not the choice surface. Full lib: 16549 passed. Integration: 3040 passed. Clippy clean. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> * feat(engine): generalize the granted-ETB-keyword path to Bloodthirst (Bloodlord of Vaasgoth, phase-rs#5802 review) matthewevans's [MED] review: the virtual ETB replacement handled only granted Sunburst, leaving the equivalent granted Bloodthirst path (Bloodlord of Vaasgoth: "Whenever you cast a Vampire creature spell, it gains bloodthirst 3") incorrect — a new special case beside an equally-broken keyword. Generalize the machinery into a keyword-agnostic granted-ETB-keyword path covering both Sunburst and Bloodthirst (and any future as-enters keyword): - `GrantedEtbKeyword { Sunburst, Bloodthirst }` with per-keyword reserved virtual-candidate ids (`GRANTED_SUNBURST_INDEX = MAX-7`, `GRANTED_BLOODTHIRST_INDEX = MAX-8`) feeding a shared count/apply core; `rid.index` recovers the keyword. - `granted_keyword_etb_instances` factors out the EFFECTIVE-minus-BASE keyword count (via `effective_off_zone_keywords`, since the entering spell is still on the stack); `granted_sunburst_instances` delegates to it, `granted_bloodthirst_instances` counts per distinct `BloodthirstValue` (mirroring `synthesize_bloodthirst`, CR 702.54c). - `bloodthirst_replacement_definition` extracted in synthesis.rs (mirroring `sunburst_replacement_definition`); both the printed synthesizer and the virtual applier build per-instance definitions from the same authority. - `apply_granted_keyword_etb_replacement` is keyword-agnostic and honors each definition's carried `condition` via `evaluate_replacement_condition` — Bloodthirst fixed-N only places counters when an opponent was dealt damage this turn (CR 702.54a); Sunburst has `condition: None` and always applies. `granted_etb_keyword_candidate_applies` re-checks the condition at registration so a condition-unmet grant raises no spurious CR 616.1e prompt. - `find_applicable_replacements` registers both keywords on `ZoneChange`→Battlefield; materiality stays `Writes { Count, Additive }`. - `keywords.rs`: Bloodthirst added to the instance-coexistence predicate so a granted instance survives beside an identical printed one (CR 702.54c). Also addresses Gemini's nit: the applier no longer rebuilds `ProposedEvent::ZoneChange` field-by-field — it mutates `enter_with_counters` in place via `if let ProposedEvent::ZoneChange { enter_with_counters, .. } = &mut event`, immune to new event fields. Rebased onto current main (resolved the `usize::MAX - 6` index collision with the new commander-return virtual candidate; `GRANTED_SUNBURST_INDEX` moved to MAX-7). Tests (integration, real cast/trigger/replacement pipeline): granted Bloodthirst 3 with an opponent damaged this turn → 3 +1/+1 counters; NO opponent damaged → 0 counters (condition revert-canary); end-to-end via Bloodlord's real "it gains bloodthirst 3" trigger → 3 counters. All 9 existing sunburst tests stay green. Full lib: 16959 passed. Integration: 3349 passed. Clippy clean. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> * fix(engine): fold the granted-Sunburst stack-provenance guard into main's cast-payment-stamp authority Conflict resolution for the bring-current merge, isolated here so the judgement is reviewable on its own. This branch guarded `clear_post_collection_transients` so a spell still on the Stack kept `mana_spent_to_cast` / `colors_spent_to_cast`, because a GRANTED sunburst (Solar Array / Lux Artillery) enters AFTER the intervening cast-triggered grant resolves and that clear runs — wiping the color tally made it place zero counters (phase-rs#5337). While this branch sat, main fixed the same underlying seam independently for issue phase-rs#5943: `clear_post_collection_transients` now routes all five cast- payment stamps through `GameObject::clear_cast_payment_stamps()` and calls it ONLY for objects outside the Battlefield/Stack provenance zones. So `colors_spent_to_cast` — the field granted Sunburst actually reads, via `colors_spent_to_cast.distinct_colors()` — already survives on Stack objects under main's authority. The remaining unconditional line clears only the `mana_spent_to_cast` boolean, which main documents as a deliberate per-collection transient and which this feature does not read. Taking main's side therefore preserves this branch's behavior while keeping a single authority for payment-stamp lifetime, instead of layering a second, partly-redundant guard beside it. Verified, not assumed: with main's side taken, all 12 of this branch's own regressions pass — granted_sunburst_5337 (9) and granted_bloodthirst_5802 (3), including lux_artillery_grants_sunburst_two_colors_enters_with_two_p1p1 and solar_array_grants_sunburst_creature_three_colors_enters_with_three_p1p1, which are exactly the cases that fail if stack color provenance is wiped. Full `cargo test -p engine`: 21,496 passed, 0 failed. Co-authored-by: shin-core <153108882+shin-core@users.noreply.github.com> * fix(engine): branch granted Sunburst on printed types; cut the entry-gate keyword sweep Maintainer review fixups on top of phase-rs#5802. CR 702.44a rules defect: `granted_etb_replacement_definitions` branched the Sunburst counter type on `obj.card_types`, the LIVE layer result. The rule says sunburst applies "if this object is entering as a creature, IGNORING ANY TYPE-CHANGING EFFECTS that would affect it", and Layer-6 type effects do reach stack objects (`remote_type_layer_recipients`) — which is the zone this check runs in. Branch on `base_card_types` instead, the same printed `card_face.card_type` the printed synthesizer (`synthesize_sunburst`) uses, so the granted and printed paths agree. Adds the first fixture whose printed and live core types DIVERGE; all 12 existing fixtures set `base_card_types == card_types`, so this arm was previously unexercised and the defect was invisible. Perf on the `find_applicable_replacements` hot path (states are cloned and replayed constantly under AI search): the `ZoneChange`→Battlefield gate ran a full off-zone keyword sweep per keyword family, each one a whole-game continuous-effect collect plus ordering and per-effect filter evaluation, and the guard was ordered expensive-term-first. Test the cheap `already_applied` set lookup first, and resolve the live keyword list at most once (lazily) and thread it through the family helpers so one sweep serves every family. Also renames `Keyword::sums_across_instances` to `instances_must_coexist` — all three call sites implement "don't dedup identical instances" and none sums a parameter, matching the predicate's own doc — and repoints a dangling doc reference to `granted_sunburst_replacements`, a symbol that does not exist. Co-authored-by: shin-core <153108882+shin-core@users.noreply.github.com> --------- Co-authored-by: Claude Fable 5 <noreply@anthropic.com> Co-authored-by: Matt Evans <matt.evans.dev@gmail.com> Co-authored-by: matthewevans <matthewevans@users.noreply.github.com>
2 participants
Fixes #5337 (claimed there with the full diagnosis). Verified Solar Array's Oracle text and CR 702.44a/b/d against
docs/MagicCompRules.txt, and reproduced end-to-end before building: a granted-sunburst artifact cast for three colors entered with 0 counters (printed-sunburst control: 3 — correct).Three stacked defects, one per layer
1. Parser — the grant never landed (Solar Array). "When you next cast an artifact spell this turn, that spell gains sunburst" lowered the subject-position anaphor to
affected: ParentTarget. AWhenNextEventdelayed trigger has no parent target, soregister_transient_effectbound the grant against the empty chain-tracked set and silently dropped it — the stack object never had Sunburst at all. Fixed with a scoped lift (mirroring thelift_parent_target_to_triggering_sourcefamily): GenericEffect grants in aWhenNextEventbody rebind toTriggeringSource— the just-cast spell IS the event source (CR 608.2k).2. Engine — a landed grant still did nothing (the class bug; Lux Artillery is equally broken today). Printed sunburst works via an object-carried ETB
ReplacementDefinitionpre-synthesized from the card face's printed keywords; a granted keyword adds no replacement, and nothing consulted live keywords at entry. Fix: a granted-instance virtual replacement candidate infind_applicable_replacements(alongside the existing shield/finality-counter virtual candidates), built from the same sharedsunburst_replacement_definitionauthority the printed synthesis uses — so it participates in normal CR 616 replacement ordering (a Doubling Season-class doubler doubles it, tested).The subtle part: granted instances are counted as effective minus base keywords via
effective_off_zone_keywords— the entering spell is still on the stack when its entry pipeline runs, and a granted keyword exists only as a continuous effect at that moment (the materialized keyword list is empty), so the off-zone authority is the only correct read. The base subtraction keeps printed instances on their carried definitions: printed + granted apply separately, exactly CR 702.44d ("each one works separately").3. Engine — cast-trigger resolution wiped the color provenance. Resolving an intervening cast trigger (Lux Artillery's own grant trigger) cleared
colors_spent_to_caston objects still on the stack — erasing the color count before the granted spell entered. The clear now preserves live cast provenance for stack objects (CR 601.2h), mirroring howcast_from_zoneis preserved.Supporting: the
Keywordinstance-coexistence predicate now also covers CR 702.44d instance-count multiplicity (previously only Toxic's parameter summation kept duplicate instances), so a granted Sunburst survives beside an identical printed one.Tests (integration, real activate/cast/trigger/replacement pipeline, verbatim Oracle texts)
affected: TriggeringSource+ Sunburst AddKeywordThe Solar Array tests drive the real "{T}: Add one mana of any color" activation including the
ChooseManaColorprompt.Class coverage
The granted-vs-printed gap is structural across the enters-with-counters keyword family that synthesis pre-bakes (Modular, Graft, Bloodthirst, Devour, Amplify, Fading, Vanishing, Read Ahead). Both actual granters today are fixed and tested in this PR:
af975c2cgeneralizes the virtual-candidate authority over aGrantedEtbKeyword{Sunburst, Bloodthirst}parameterization rather than adding a parallel Sunburst-shaped path, and carries its own integration coverage (granted_bloodthirst_5802.rs), including the CR 702.54a condition gate (an unmet damage condition contributes zero counters, routed through the sameevaluate_replacement_conditionseam the printed path uses).(An earlier revision of this description said Bloodthirst was deferred; that is no longer accurate — it ships here.)
Verification
Full engine lib: 16549 passed, 0 failed. Full integration: 3038 passed, 0 failed.
cargo fmtclean; parser combinator gate pass. All CR annotations (702.44a/b/d, 608.2k, 601.2h, 616.1, 604.1, 613.1f, 614.1c) grep-verified. (These counts predate the maintainer fixup commit below; that commit's evidence is the PR's own required CI run.)Maintainer fixups
Applied directly on this branch to avoid another contributor round-trip; the contributor is retained as co-author.
granted_etb_replacement_definitionsselected the Sunburst counter type fromobj.card_types, the live layer result. CR 702.44a says sunburst applies "if this object is entering as a creature, ignoring any type-changing effects that would affect it", and Layer-6 type effects do reach stack objects (remote_type_layer_recipients) — the very zone this check runs in. Now readsbase_card_types, seeded from the samecard_face.card_typethe printed synthesizer branches on, so the granted and printed paths agree. All 12 pre-existing fixtures setbase_card_types == card_types, leaving the divergent arm unexercised, so a fixture whose printed and live types diverge was added — it fails if the branch is reverted.find_applicable_replacements. TheZoneChange→Battlefield gate ran a full off-zone keyword sweep per keyword family — each a whole-game continuous-effect collect plus ordering and per-effect filter evaluation — and ordered the guard expensive-term-first. The cheapalready_appliedset lookup now short-circuits first, and the live keyword list is resolved at most once (lazily) and threaded through the family helpers, so one sweep serves every family.Keyword::sums_across_instances→instances_must_coexist. All three call sites implement "don't dedup identical instances"; none sums a parameter. The predicate's own doc already headlines "must COEXIST" — only the name disagreed.granted_sunburst_replacements(no such symbol) repointed at the realgranted_sunburst_instances→granted_etb_replacement_definitionschain.🤖 Generated with Claude Code
Summary by CodeRabbit
Bug Fixes
Tests